fix(expose): override public service port (#210)

* fix(expose): override service port if 'expose.service.overrides.{service}.publicPort' is defined.

* fix(expose): avoid to override patches.

* fix(expose): cleanup code.

* fix(expose): add unit test.

* fix(expose): add unit test.

* fix(expose): add unit test.

pkg/deploy/spindeploy/expose_service/testdata/spinsvc_expose.yml

@@ -21,7 +21,6 @@ spec:
     type: service
     service:
       type: LoadBalancer
-      publicPort: 80
       annotations:
         "service.beta.kubernetes.io/aws-load-balancer-backend-protocol": "http"
       overrides: {}

pkg/deploy/spindeploy/expose_service/testdata/spinsvc_expose_publicPort.yml

@@ -0,0 +1,30 @@
+apiVersion: spinnaker.io/v1alpha2
+kind: SpinnakerService
+metadata:
+  name: spinnaker
+  namespace: ns1
+spec:
+  spinnakerConfig:
+    config:
+      version: 1.15.1
+      persistentStorage:
+        persistentStoreType: s3
+        s3:
+          bucket: my-bucket
+          region: us-west-2
+          rootFolder: front50
+    profiles:
+      gate:
+        default:
+          apiPort: 8085
+  expose:
+    type: service
+    service:
+      type: LoadBalancer
+      publicPort: 80
+      annotations:
+        "service.beta.kubernetes.io/aws-load-balancer-backend-protocol": "http"
+      overrides: {}
+status:
+  apiUrl: http://acme.com
+  uiUrl: http://acme.com

pkg/deploy/spindeploy/expose_service/testdata/spinsvc_override_port.yml

@@ -0,0 +1,34 @@
+apiVersion: spinnaker.io/v1alpha2
+kind: SpinnakerService
+metadata:
+  name: spinnaker
+  namespace: ns1
+spec:
+  spinnakerConfig:
+    config:
+      version: 1.23.3
+      persistentStorage:
+        persistentStoreType: s3
+        s3:
+          bucket: my-bucket
+          region: us-west-2
+          rootFolder: front50
+      security:
+        apiSecurity:
+          overrideBaseUrl: http://acme.com
+          ssl:
+            enabled: false
+        uiSecurity:
+          overrideBaseUrl: http://acme.com
+          ssl:
+            enabled: false
+  expose:
+    type: service
+    service:
+      type: LoadBalancer
+      annotations:
+        service.beta.kubernetes.io/aws-load-balancer-backend-protocol: http
+      overrides:
+        gate:
+          type: ClusterIP
+          publicPort: 8089

pkg/deploy/spindeploy/expose_service/transform.go

@@ -40,10 +40,10 @@ func (t *exposeTransformer) TransformManifests(ctx context.Context, gen *generat
 	if !applies(t.svc) {
 		return nil
 	}
-	if err := t.transformServiceManifest(ctx, "deck", util.DeckOverrideBaseUrlProp, gen.Config["deck"].Service); err != nil {
+	if err := t.transformServiceManifest(ctx, "deck", gen.Config["deck"].Service); err != nil {
 		return err
 	}
-	return t.transformServiceManifest(ctx, "gate", util.GateOverrideBaseUrlProp, gen.Config["gate"].Service)
+	return t.transformServiceManifest(ctx, "gate", gen.Config["gate"].Service)
 }
 
 func (t *exposeTransformer) TransformConfig(ctx context.Context) error {
@@ -124,12 +124,12 @@ func (t *exposeTransformer) generateOverrideUrl(ctx context.Context, serviceName
 	return util.BuildUrl(parsedLbUrl.Scheme, parsedLbUrl.Hostname(), desiredPort), nil
 }
 
-func (t *exposeTransformer) transformServiceManifest(ctx context.Context, svcName, overrideUrlKeyName string, svc *corev1.Service) error {
+func (t *exposeTransformer) transformServiceManifest(ctx context.Context, svcName string, svc *corev1.Service) error {
 	if svc == nil {
 		return nil
 	}
 	defaultPort := util.GetDesiredExposePort(ctx, svcName, int32(80), t.svc)
-	if err := t.applyPortChanges(ctx, fmt.Sprintf("%s-tcp", svcName), defaultPort, overrideUrlKeyName, svc); err != nil {
+	if err := t.applyPortChanges(ctx, fmt.Sprintf("%s-tcp", svcName), defaultPort, svc); err != nil {
 		return err
 	}
 	ApplyExposeServiceConfig(t.svc.GetExposeConfig(), svc, svcName)
@@ -148,10 +148,9 @@ func ApplyExposeServiceConfig(exp *interfaces.ExposeConfig, svc *corev1.Service,
 	svc.Annotations = exp.GetAggregatedAnnotations(serviceName)
 }
 
-func (t *exposeTransformer) applyPortChanges(ctx context.Context, portName string, portDefault int32, overrideUrlName string, svc *corev1.Service) error {
+func (t *exposeTransformer) applyPortChanges(ctx context.Context, portName string, portDefault int32, svc *corev1.Service) error {
 	if len(svc.Spec.Ports) > 0 {
-		overrideUrl, _ := t.svc.GetSpinnakerConfig().GetHalConfigPropString(ctx, overrideUrlName)
-		svc.Spec.Ports[0].Port = util.GetPort(overrideUrl, portDefault)
+		svc.Spec.Ports[0].Port = portDefault
 		svc.Spec.Ports[0].Name = portName
 		if strings.Contains(portName, "gate") {
 			// ignore error, property may be missing

pkg/deploy/spindeploy/expose_service/transform_test.go

@@ -39,6 +39,20 @@ func TestTransformManifests_ExposedWithOverrideUrlChangingPort(t *testing.T) {
 	assert.Equal(t, expected, gen.Config["gate"].Service)
 }
 
+func TestTransformManifests_ExposedWithOverridePort(t *testing.T) {
+	tr, spinSvc := transformertest.SetupTransformerFromSpinFile(&TransformerGenerator{}, "testdata/spinsvc_expose_publicPort.yml", t)
+	gen := &generated.SpinnakerGeneratedConfig{}
+	test.AddServiceToGenConfig(gen, "gate", "testdata/input_service.yml", t)
+	err := spinSvc.GetSpinnakerConfig().SetHalConfigProp("security.apiSecurity.overrideBaseUrl", "https://my-api.spin.com")
+
+	err = tr.TransformManifests(context.TODO(), gen)
+	assert.Nil(t, err)
+
+	expected := &corev1.Service{}
+	test.ReadYamlFile("testdata/output_service_lb.yml", expected, t)
+	assert.Equal(t, expected, gen.Config["gate"].Service)
+}
+
 func TestTransformManifests_ExposedAggregatedAnnotations(t *testing.T) {
 	s := `
 apiVersion: spinnaker.io/v1alpha2
@@ -282,3 +296,17 @@ func TestTransformHalconfig_ExposedPortRemovedFromConfig(t *testing.T) {
 	assert.Equal(t, "http://abc.com", actualOverrideUrl)
 	assert.Equal(t, "http://abc.com", (tr.(*exposeTransformer)).svc.GetStatus().APIUrl)
 }
+
+// Input: existing services running on custom port, then spin config override port
+func TestTransformHalconfig_ExposedPortOverridedFromConfig(t *testing.T) {
+	gateSvc := &corev1.Service{}
+	test.ReadYamlFile("testdata/output_service_lb.yml", gateSvc, t)
+	tr, spinSvc := transformertest.SetupTransformerFromSpinFile(&TransformerGenerator{}, "testdata/spinsvc_override_port.yml", t, gateSvc)
+	gen := &generated.SpinnakerGeneratedConfig{}
+	test.AddServiceToGenConfig(gen, "gate", "testdata/input_service.yml", t)
+	spinSvc.GetExposeConfig().Service.PublicPort = 0
+
+	err := tr.TransformManifests(context.TODO(), gen)
+	assert.Nil(t, err)
+	assert.Equal(t, int32(8089), gen.Config["gate"].Service.Spec.Ports[0].Port)
+}

pkg/util/runtime.go

@@ -128,14 +128,6 @@ func GetPort(aUrl string, defaultPort int32) int32 {
 // GetDesiredExposePort returns the expected public port to have for the given service, according to halyard and expose configurations
 func GetDesiredExposePort(ctx context.Context, svcNameWithoutPrefix string, defaultPort int32, spinSvc interfaces.SpinnakerService) int32 {
 	desiredPort := defaultPort
-	exp := spinSvc.GetExposeConfig()
-	if c, ok := exp.Service.Overrides[svcNameWithoutPrefix]; ok {
-		if c.PublicPort != 0 {
-			desiredPort = c.PublicPort
-		}
-	} else if exp.Service.PublicPort != 0 {
-		desiredPort = exp.Service.PublicPort
-	}
 
 	// Get port from overrideBaseUrl, if any
 	propName := ""
@@ -150,7 +142,18 @@ func GetDesiredExposePort(ctx context.Context, svcNameWithoutPrefix string, defa
 		// ignore error, prop may be missing
 		overrideBaseUrl, _ = spinSvc.GetSpinnakerConfig().GetHalConfigPropString(ctx, propName)
 	}
-	return GetPort(overrideBaseUrl, desiredPort)
+	desiredPort = GetPort(overrideBaseUrl, desiredPort)
+
+	exp := spinSvc.GetExposeConfig()
+	if c, ok := exp.Service.Overrides[svcNameWithoutPrefix]; ok {
+		if c.PublicPort != 0 {
+			desiredPort = c.PublicPort
+		}
+	} else if exp.Service.PublicPort != 0 {
+		desiredPort = exp.Service.PublicPort
+	}
+
+	return desiredPort
 }
 
 func CreateOrUpdateService(svc *corev1.Service, rawClient *kubernetes.Clientset) error {