Refactor configureGWServiceType to configureGatewayService

knative · Jun 10, 2023 · ff1d262 · ff1d262
1 parent 566614e
commit ff1d262
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 50 deletions.
diff --git a/pkg/reconciler/knativeserving/ingress/ingress_test.go b/pkg/reconciler/knativeserving/ingress/ingress_test.go
@@ -58,7 +58,7 @@ func TestTransformers(t *testing.T) {
 				},
 			},
 		},
-		expected: 4,
+		expected: 3,
 	}, {
 		name: "Available contour ingress",
 		instance: servingv1beta1.KnativeServing{
@@ -94,7 +94,7 @@ func TestTransformers(t *testing.T) {
 				},
 			},
 		},
-		expected: 5,
+		expected: 4,
 	}}
 
 	for _, tt := range tests {

diff --git a/pkg/reconciler/knativeserving/ingress/kourier.go b/pkg/reconciler/knativeserving/ingress/kourier.go
@@ -40,16 +40,15 @@ var kourierControllerDeploymentNames = sets.NewString("3scale-kourier-control",
 
 func kourierTransformers(ctx context.Context, instance *v1beta1.KnativeServing) []mf.Transformer {
 	return []mf.Transformer{
-		replaceGWNamespace(),
-		configureGWServiceType(instance),
-		configureGWServiceLoadBalancerIP(instance),
+		replaceGatewayNamespace(),
+		configureGatewayService(instance),
 		configureBootstrapConfigMap(instance),
 	}
 }
 
-// replaceGWNamespace replace the environment variable KOURIER_GATEWAY_NAMESPACE with the
+// replaceGatewayNamespace replace the environment variable KOURIER_GATEWAY_NAMESPACE with the
 // namespace of the deployment its set on.
-func replaceGWNamespace() mf.Transformer {
+func replaceGatewayNamespace() mf.Transformer {
 	return func(u *unstructured.Unstructured) error {
 		if u.GetKind() == "Deployment" && kourierControllerDeploymentNames.Has(u.GetName()) {
 			deployment := &appsv1.Deployment{}
@@ -75,19 +74,19 @@ func replaceGWNamespace() mf.Transformer {
 	}
 }
 
-// configureGWServiceType configures Kourier GW's service type such as ClusterIP, LoadBalancer and NodePort.
-func configureGWServiceType(instance *v1beta1.KnativeServing) mf.Transformer {
+func configureGatewayService(instance *v1beta1.KnativeServing) mf.Transformer {
 	return func(u *unstructured.Unstructured) error {
-		if u.GetKind() == "Service" && u.GetName() == kourierGatewayServiceName {
-			if instance.Spec.Ingress.Kourier.ServiceType == "" {
-				// Do nothing if ServiceType is not configured.
-				return nil
-			}
-			svc := &v1.Service{}
-			if err := scheme.Scheme.Convert(u, svc, nil); err != nil {
-				return err
-			}
+		if !(u.GetKind() == "Service" && u.GetName() == kourierGatewayServiceName) {
+			return nil
+		}
 
+		svc := &v1.Service{}
+		if err := scheme.Scheme.Convert(u, svc, nil); err != nil {
+			return err
+		}
+
+		// First configure the Service Type.
+		if instance.Spec.Ingress.Kourier.ServiceType != "" {
 			serviceType := instance.Spec.Ingress.Kourier.ServiceType
 			switch serviceType {
 			case v1.ServiceTypeClusterIP, v1.ServiceTypeNodePort, v1.ServiceTypeLoadBalancer:
@@ -97,34 +96,18 @@ func configureGWServiceType(instance *v1beta1.KnativeServing) mf.Transformer {
 			default:
 				return fmt.Errorf("unknown service type %q", serviceType)
 			}
-
-			if err := scheme.Scheme.Convert(svc, u, nil); err != nil {
-				return err
-			}
 		}
-		return nil
-	}
-}
 
-// configureGWServiceLoadBalancerIP configures Kourier GW's service loadBalancerIP.
-func configureGWServiceLoadBalancerIP(instance *v1beta1.KnativeServing) mf.Transformer {
-	return func(u *unstructured.Unstructured) error {
-		if u.GetKind() == "Service" && u.GetName() == kourierGatewayServiceName {
-			if instance.Spec.Ingress.Kourier.ServiceLoadBalancerIP == "" {
-				// Do nothing if ServiceLoadBalancerIP is not configured.
-				return nil
-			}
-			svc := &v1.Service{}
-			if err := scheme.Scheme.Convert(u, svc, nil); err != nil {
-				return err
+		// Then configure LoadBalancerIP if set.
+		if instance.Spec.Ingress.Kourier.ServiceLoadBalancerIP != "" {
+			if svc.Spec.Type != v1.ServiceTypeLoadBalancer {
+				return fmt.Errorf("cannot configure LoadBalancerIP for service type %q", svc.Spec.Type)
 			}
-
 			svc.Spec.LoadBalancerIP = instance.Spec.Ingress.Kourier.ServiceLoadBalancerIP
-			if err := scheme.Scheme.Convert(svc, u, nil); err != nil {
-				return err
-			}
 		}
-		return nil
+
+		// Return any conversion error or nil
+		return scheme.Scheme.Convert(svc, u, nil)
 	}
 }
 

diff --git a/pkg/reconciler/knativeserving/ingress/kourier_test.go b/pkg/reconciler/knativeserving/ingress/kourier_test.go
@@ -83,22 +83,30 @@ func TestTransformKourierManifest(t *testing.T) {
 		expConfigMapName:         kourierDefaultVolumeName,
 		expServiceType:           "LoadBalancer",
 		expServiceLoadBalancerIP: "1.2.3.4",
+	}, {
+		name:                     "Use ServiceLoadBalancerIP with unsupported service type",
+		instance:                 servingInstance(servingNamespace, "ClusterIP", "", "1.2.3.4"),
+		expNamespace:             servingNamespace,
+		expConfigMapName:         kourierDefaultVolumeName,
+		expServiceType:           "ClusterIP",
+		expServiceLoadBalancerIP: "1.2.3.4",
+		expError:                 fmt.Errorf("cannot configure LoadBalancerIP for service type \"ClusterIP\""),
 	}, {
 		name:                     "Use unsupported service type",
 		instance:                 servingInstance(servingNamespace, "ExternalName", "", ""),
 		expNamespace:             servingNamespace,
 		expServiceType:           "ExternalName",
 		expConfigMapName:         kourierDefaultVolumeName,
-		expError:                 fmt.Errorf("unsupported service type \"ExternalName\""),
 		expServiceLoadBalancerIP: "",
+		expError:                 fmt.Errorf("unsupported service type \"ExternalName\""),
 	}, {
 		name:                     "Use unknown service type",
 		instance:                 servingInstance(servingNamespace, "Foo", "", ""),
 		expNamespace:             servingNamespace,
 		expServiceType:           "Foo",
 		expConfigMapName:         kourierDefaultVolumeName,
-		expError:                 fmt.Errorf("unknown service type \"Foo\""),
 		expServiceLoadBalancerIP: "",
+		expError:                 fmt.Errorf("unknown service type \"Foo\""),
 	}}
 
 	for _, tt := range tests {
@@ -109,12 +117,12 @@ func TestTransformKourierManifest(t *testing.T) {
 				t.Fatalf("Failed to read manifest: %v", err)
 			}
 
-			manifest, err = manifest.Transform(replaceGWNamespace())
+			manifest, err = manifest.Transform(replaceGatewayNamespace())
 			if err != nil {
 				t.Fatalf("Failed to transform manifest: %v", err)
 			}
 
-			manifest, err = manifest.Transform(configureGWServiceType(tt.instance))
+			manifest, err = manifest.Transform(configureGatewayService(tt.instance))
 			if err != nil {
 				util.AssertEqual(t, err.Error(), tt.expError.Error())
 			} else {
@@ -126,11 +134,6 @@ func TestTransformKourierManifest(t *testing.T) {
 				t.Fatalf("Failed to transform manifest: %v", err)
 			}
 
-			manifest, err = manifest.Transform(configureGWServiceLoadBalancerIP(tt.instance))
-			if err != nil {
-				t.Fatalf("Failed to transform manifest: %v", err)
-			}
-
 			for _, u := range manifest.Resources() {
 				verifyControllerNamespace(t, &u, tt.expNamespace)
 				verifyGatewayServiceType(t, &u, tt.expServiceType)