fix: respect namespace in structured local-gateways config (#2217)

* fix: respect namespace in structured local-gateways config

Signed-off-by: DCchoudhury15 <divyanshuchoudhury3@gmail.com>

* Update pkg/reconciler/knativeserving/ingress/ingress_service.go

Co-authored-by: Vincent <shou73@bloomberg.net>

* refactor: breakdown UpdateNamespace logic as requested

---------

Signed-off-by: DCchoudhury15 <divyanshuchoudhury3@gmail.com>
Co-authored-by: Vincent <shou73@bloomberg.net>

Author: DCchoudhury15

pkg/reconciler/knativeserving/ingress/ingress_service.go

@@ -23,19 +23,28 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/kubernetes/scheme"
 	"knative.dev/operator/pkg/apis/operator/base"
+	"sigs.k8s.io/yaml"
 
 	mf "github.com/manifestival/manifestival"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"knative.dev/operator/pkg/apis/operator/v1beta1"
 )
 
+// localGateway defines the structure for the entries in the 'local-gateways' array.
+type localGatewayConfig struct {
+	Name      string `json:"name"`
+	Namespace string `json:"namespace"`
+	Service   string `json:"service"`
+}
+
 // IngressServiceTransform pins the namespace to istio-system for the service named knative-local-gateway.
 // It also removes the OwnerReference to the operator, as they are in different namespaces, which is
 // invalid in Kubernetes 1.20+.
 func IngressServiceTransform(ks *v1beta1.KnativeServing) mf.Transformer {
 	return func(u *unstructured.Unstructured) error {
 		if u.GetAPIVersion() == "v1" && u.GetKind() == "Service" {
 			if u.GetName() == "knative-local-gateway" {
+				// Default to istio-system, then override if config exists
 				u.SetNamespace("istio-system")
 				u.SetOwnerReferences(nil)
 				config := ks.GetSpec().GetConfig()
@@ -74,15 +83,58 @@ func updateIstioService(ks *v1beta1.KnativeServing, u *unstructured.Unstructured
 	return nil
 }
 
+var (
+	knativeLocalGateway = "knative-local-gateway"
+	localGateways       = "local-gateways"
+)
+
 // UpdateNamespace set correct namespace of istio to the service knative-local-gateway
-func UpdateNamespace(u *unstructured.Unstructured, data map[string]string, ns string) {
-	key := fmt.Sprintf("%s.%s.%s", "local-gateway", ns, "knative-local-gateway")
-	if val, ok := data[key]; ok {
-		fields := strings.Split(val, ".")
-		// The value is in the format of knative-local-gateway.{istio-namespace}.svc.cluster.local
-		// The second item is the istio namespace
-		if len(fields) >= 2 {
-			u.SetNamespace(fields[1])
+func UpdateNamespace(u *unstructured.Unstructured, data map[string]string, namespace string) {
+	if ns := resolveGatewayNamespace(data, namespace); ns != "" {
+		u.SetNamespace(ns)
+	}
+}
+
+func resolveGatewayNamespace(data map[string]string, ns string) string {
+	if ns := fromStructuredGateways(data); ns != "" {
+		return ns
+	}
+
+	return fromLegacyGateway(data, ns)
+}
+
+func fromStructuredGateways(data map[string]string) string {
+	raw, ok := data[localGateways]
+	if !ok {
+		return ""
+	}
+
+	var gateways []localGatewayConfig
+	if err := yaml.Unmarshal([]byte(raw), &gateways); err != nil {
+		return ""
+	}
+
+	for _, gw := range gateways {
+		if gw.Name == knativeLocalGateway {
+			return gw.Namespace
 		}
 	}
+
+	return ""
+}
+
+func fromLegacyGateway(data map[string]string, ns string) string {
+	key := fmt.Sprintf("local-gateway.%s.%s", ns, knativeLocalGateway)
+
+	raw, ok := data[key]
+	if !ok {
+		return ""
+	}
+
+	fields := strings.Split(raw, ".")
+	if len(fields) < 2 {
+		return ""
+	}
+
+	return fields[1]
 }

pkg/reconciler/knativeserving/ingress/ingress_service_test.go

@@ -88,6 +88,49 @@ func TestIngressServiceTransformIstioSelector(t *testing.T) {
 			},
 		},
 		expected: true,
+	}, {
+		name:              "Structured local-gateways config",
+		namespace:         "test-namespace",
+		serviceName:       "knative-local-gateway",
+		expectedNamespace: "custom-gateway-ns",
+		instance: &servingv1beta1.KnativeServing{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-instance",
+				Namespace: "test-namespace",
+			},
+			Spec: servingv1beta1.KnativeServingSpec{
+				CommonSpec: base.CommonSpec{
+					Config: map[string]map[string]string{
+						"istio": {
+							"local-gateways": "- name: knative-local-gateway\n  namespace: custom-gateway-ns\n  service: knative-local-gateway.custom-gateway-ns.svc.cluster.local",
+						},
+					},
+				},
+			},
+		},
+		expected: true,
+	}, {
+		name:              "Structured config precedence",
+		namespace:         "test-namespace",
+		serviceName:       "knative-local-gateway",
+		expectedNamespace: "modern-ns",
+		instance: &servingv1beta1.KnativeServing{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-instance",
+				Namespace: "test-namespace",
+			},
+			Spec: servingv1beta1.KnativeServingSpec{
+				CommonSpec: base.CommonSpec{
+					Config: map[string]map[string]string{
+						"istio": {
+							"local-gateways": "- name: knative-local-gateway\n  namespace: modern-ns",
+							"local-gateway.test-namespace.knative-local-gateway": "knative-local-gateway.legacy-ns.svc.cluster.local",
+						},
+					},
+				},
+			},
+		},
+		expected: true,
 	}, {
 		name:              "IstioNotUnderDefaultNS with both istio and config-istio",
 		namespace:         "test-namespace",
@@ -137,14 +180,12 @@ func TestIngressServiceTransformIstioSelector(t *testing.T) {
 
 func TestIngressServiceTransform(t *testing.T) {
 	tests := []struct {
-		name                  string
-		namespace             string
-		serviceName           string
-		expectedNamespace     string
-		knativeIngressGateway *base.IstioGatewayOverride
-		clusterLocalGateway   *base.IstioGatewayOverride
-		instance              *servingv1beta1.KnativeServing
-		expected              map[string]string
+		name              string
+		namespace         string
+		serviceName       string
+		expectedNamespace string
+		instance          *servingv1beta1.KnativeServing
+		expected          map[string]string
 	}{{
 		name:              "Istio ingress selector configuration for local gateway service",
 		namespace:         "test-namespace",
@@ -158,8 +199,10 @@ func TestIngressServiceTransform(t *testing.T) {
 			Spec: servingv1beta1.KnativeServingSpec{
 				Ingress: &servingv1beta1.IngressConfigs{
 					Istio: base.IstioIngressConfiguration{
-						Enabled:             true,
-						KnativeLocalGateway: gatewayOverride(map[string]string{"istio": "cluster-local-1"}, nil),
+						Enabled: true,
+						KnativeLocalGateway: &base.IstioGatewayOverride{
+							Selector: map[string]string{"istio": "cluster-local-1"},
+						},
 					},
 				},
 			},
@@ -180,8 +223,10 @@ func TestIngressServiceTransform(t *testing.T) {
 			Spec: servingv1beta1.KnativeServingSpec{
 				Ingress: &servingv1beta1.IngressConfigs{
 					Istio: base.IstioIngressConfiguration{
-						Enabled:               true,
-						KnativeIngressGateway: gatewayOverride(map[string]string{"istio": "ingress-gateway-1"}, nil),
+						Enabled: true,
+						KnativeIngressGateway: &base.IstioGatewayOverride{
+							Selector: map[string]string{"istio": "ingress-gateway-1"},
+						},
 					},
 				},
 			},