chore: Fix various MR comments

knative-extensions · Feb 29, 2024 · 7809617 · 7809617
1 parent 00596c1
commit 7809617
Show file tree

Hide file tree

Showing 5 changed files with 156 additions and 154 deletions.
diff --git a/pkg/reconciler/ingress/config/istio.go b/pkg/reconciler/ingress/config/istio.go
@@ -231,6 +231,14 @@ func parseNewFormat(configMap *corev1.ConfigMap) (*Istio, error) {
 		ret.LocalGateways = localGateways
 	}
 
+	if len(ret.DefaultExternalGateways()) > 1 {
+		return ret, fmt.Errorf("only one external gateway with no selector can be defined, here: %v", ret.DefaultExternalGateways())
+	}
+
+	if len(ret.DefaultLocalGateways()) > 1 {
+		return ret, fmt.Errorf("only one local gateway with no selector can be defined, here: %v", ret.DefaultLocalGateways())
+	}
+
 	return ret, nil
 }
 

diff --git a/pkg/reconciler/ingress/config/istio_test.go b/pkg/reconciler/ingress/config/istio_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package config
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -233,14 +234,14 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 	}, {
@@ -252,14 +253,14 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 				"local-gateway.custom-namespace.custom-local-gateway": "istio-ingressbackroad.istio-system.svc.cluster.local",
 				"gateway.custom-namespace.custom-gateway":             "istio-ingressfreeway.istio-system.svc.cluster.local",
 			},
@@ -273,14 +274,14 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "_invalid"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "_invalid"`),
 			},
 		},
 	}, {
@@ -292,14 +293,14 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  ""`,
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  ""`),
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 	}, {
@@ -311,14 +312,14 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: ""
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: ""
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 	}, {
@@ -330,14 +331,14 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
-				"local-gateways": `
-                  - namespace: ""
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
+				"local-gateways": replaceTabs(`
+				- namespace: ""
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 	}, {
@@ -349,10 +350,10 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
 				"local-gateways": "notYAML",
 			},
 		},
@@ -366,10 +367,10 @@ func TestGatewayConfiguration(t *testing.T) {
 			},
 			Data: map[string]string{
 				"external-gateways": "notYAML",
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 	}, {
@@ -380,10 +381,10 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service:  "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service:  "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 		wantIstio: &Istio{
@@ -402,10 +403,10 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service:  "istio-ingressbackroad.istio-system.svc.cluster.local"`),
 			},
 		},
 		wantIstio: &Istio{
@@ -424,20 +425,20 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"external-gateways": `
-                  - namespace: "namespace"
-                    name: "gateway"
-                    service: "istio-gateway.istio-system.svc.cluster.local"
-                    labelSelector:
-                      matchExpressions:
-                      - key: "key"
-                        operator: "In"
-                        values: ["value"]`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace"
+				  name: "gateway"
+				  service: "istio-gateway.istio-system.svc.cluster.local"
+				  labelSelector:
+					matchExpressions:
+					- key: "key"
+					  operator: "In"
+					  values: ["value"]`),
 
-				"local-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service: "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service: "istio-local-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 		wantIstio: &Istio{
@@ -469,23 +470,23 @@ func TestGatewayConfiguration(t *testing.T) {
 				Name:      IstioConfigName,
 			},
 			Data: map[string]string{
-				"local-gateways": `
-                  - namespace: "namespace"
-                    name: "gateway"
-                    service: "istio-local.istio-system.svc.cluster.local"
-                    labelSelector:
-                      matchExpressions:
-                      - key: "key"
-                        operator: "In"
-                        values: ["value"]
-                  - namespace: "namespace1"
-                    name: "gateway1"
-                    service: "istio-local-gateway.istio-system.svc.cluster.local"`,
+				"local-gateways": replaceTabs(`
+				- namespace: "namespace"
+				  name: "gateway"
+				  service: "istio-local.istio-system.svc.cluster.local"
+				  labelSelector:
+					matchExpressions:
+					- key: "key"
+					  operator: "In"
+					  values: ["value"]
+				- namespace: "namespace1"
+				  name: "gateway1"
+				  service: "istio-local-gateway.istio-system.svc.cluster.local"`),
 
-				"external-gateways": `
-                  - namespace: "namespace2"
-                    name: "gateway2"
-                    service: "istio-gateway.istio-system.svc.cluster.local"`,
+				"external-gateways": replaceTabs(`
+				- namespace: "namespace2"
+				  name: "gateway2"
+				  service: "istio-gateway.istio-system.svc.cluster.local"`),
 			},
 		},
 		wantIstio: &Istio{
@@ -531,3 +532,7 @@ func TestGatewayConfiguration(t *testing.T) {
 		})
 	}
 }
+
+func replaceTabs(s string) string {
+	return strings.ReplaceAll(s, "\t", "    ")
+}
diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go
@@ -113,14 +113,19 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
 	ing.Status.InitializeConditions()
 	logger.Infof("Reconciling ingress: %#v", ing)
 
-	defaultGateways, err := resources.QualifiedGatewayNamesFromContext(ctx, ing)
+	defaultGateways, err := resources.GatewaysFromContext(ctx, ing)
 	if err != nil {
 		return err
 	}
 
-	gatewayNames := map[v1alpha1.IngressVisibility]sets.Set[string]{}
-	gatewayNames[v1alpha1.IngressVisibilityClusterLocal] = defaultGateways[v1alpha1.IngressVisibilityClusterLocal]
-	gatewayNames[v1alpha1.IngressVisibilityExternalIP] = sets.New[string]()
+	gatewayNames := map[v1alpha1.IngressVisibility]sets.Set[string]{
+		v1alpha1.IngressVisibilityClusterLocal: sets.New[string](),
+		v1alpha1.IngressVisibilityExternalIP:   sets.New[string](),
+	}
+
+	for _, gateway := range defaultGateways[v1alpha1.IngressVisibilityClusterLocal] {
+		gatewayNames[v1alpha1.IngressVisibilityClusterLocal].Insert(gateway.QualifiedName())
+	}
 
 	externalIngressGateways := []*v1beta1.Gateway{}
 	if shouldReconcileExternalDomainTLS(ing) {
@@ -206,7 +211,11 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
 	} else {
 		// Otherwise, we fall back to the default global Gateways for HTTP behavior.
 		// We need this for the backward compatibility.
-		gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(sets.List(defaultGateways[v1alpha1.IngressVisibilityExternalIP])...)
+		defaultGlobalHTTPGateways := defaultGateways[v1alpha1.IngressVisibilityExternalIP]
+
+		for _, gateway := range defaultGlobalHTTPGateways {
+			gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(gateway.QualifiedName())
+		}
 	}
 
 	if err := r.reconcileIngressGateways(ctx, externalIngressGateways); err != nil {
@@ -262,17 +271,11 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
 	}
 
 	if ready {
-		publicGateways, err := resources.PublicGatewayServiceURLFromContext(ctx, ing)
-		if err != nil {
-			return fmt.Errorf("failed to get public gateways: %w", err)
-		}
-		publicLbs := getLBStatus(publicGateways)
+		publicGatewayURL := gatewayServiceURL(defaultGateways[v1alpha1.IngressVisibilityExternalIP])
+		publicLbs := getLBStatus(publicGatewayURL)
 
-		privateGateways, err := resources.PrivateGatewayServiceURLFromContext(ctx, ing)
-		if err != nil {
-			return fmt.Errorf("failed to get private gateways: %w", err)
-		}
-		privateLbs := getLBStatus(privateGateways)
+		privateGatewayURL := gatewayServiceURL(defaultGateways[v1alpha1.IngressVisibilityClusterLocal])
+		privateLbs := getLBStatus(privateGatewayURL)
 
 		ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs)
 	} else {
@@ -532,6 +535,14 @@ func (r *Reconciler) GetDestinationRuleLister() istiolisters.DestinationRuleList
 	return r.destinationRuleLister
 }
 
+func gatewayServiceURL(gateways []config.Gateway) string {
+	if len(gateways) == 0 {
+		return ""
+	}
+
+	return gateways[0].ServiceURL
+}
+
 // getLBStatus gets the LB Status.
 func getLBStatus(gatewayServiceURL string) []v1alpha1.LoadBalancerIngressStatus {
 	// The Ingress isn't load-balanced by any particular