feat: retain TLS configuration for canary ingresses in the nginx inte…

…gration. Fixes argoproj#1134 (argoproj#2679) * Retain TLS configuration for canary ingresses in the nginx integration Signed-off-by: Pavels Fjodorovs <[email protected]> * Retain TLS configuration for canary ingresses in the nginx integration Signed-off-by: Pavels Fjodorovs <[email protected]> * fixed tests after multiple nginx ingress merge Signed-off-by: Pavels Fjodorovs <[email protected]> --------- Signed-off-by: Pavels Fjodorovs <[email protected]> Signed-off-by: Pavels Fjodorovs <[email protected]> Signed-off-by: Liming Liu <[email protected]>
andyliuliming · Jul 22, 2023 · 54a7c76 · 54a7c76
1 parent 839f05d
commit 54a7c76
Show file tree

Hide file tree

Showing 2 changed files with 211 additions and 4 deletions.
diff --git a/rollout/trafficrouting/nginx/nginx.go b/rollout/trafficrouting/nginx/nginx.go
@@ -66,8 +66,6 @@ func (r *Reconciler) buildCanaryIngress(stableIngress *networkingv1.Ingress, nam
 	canaryServiceName := r.cfg.Rollout.Spec.Strategy.Canary.CanaryService
 	annotationPrefix := defaults.GetCanaryIngressAnnotationPrefixOrDefault(r.cfg.Rollout)
 
-	// Set up canary ingress resource, we do *not* have to duplicate `spec.tls` in a canary, only
-	// `spec.rules`
 	desiredCanaryIngress := &networkingv1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        name,
@@ -78,6 +76,14 @@ func (r *Reconciler) buildCanaryIngress(stableIngress *networkingv1.Ingress, nam
 		},
 	}
 
+	// Preserve TLS from stable ingress
+	if stableIngress.Spec.TLS != nil {
+		desiredCanaryIngress.Spec.TLS = make([]networkingv1.IngressTLS, len(stableIngress.Spec.TLS))
+		for it := 0; it < len(stableIngress.Spec.TLS); it++ {
+			stableIngress.Spec.TLS[it].DeepCopyInto(&desiredCanaryIngress.Spec.TLS[it])
+		}
+	}
+
 	// Preserve ingressClassName from stable ingress
 	if stableIngress.Spec.IngressClassName != nil {
 		desiredCanaryIngress.Spec.IngressClassName = stableIngress.Spec.IngressClassName
@@ -136,8 +142,6 @@ func (r *Reconciler) buildLegacyCanaryIngress(stableIngress *extensionsv1beta1.I
 	canaryServiceName := r.cfg.Rollout.Spec.Strategy.Canary.CanaryService
 	annotationPrefix := defaults.GetCanaryIngressAnnotationPrefixOrDefault(r.cfg.Rollout)
 
-	// Set up canary ingress resource, we do *not* have to duplicate `spec.tls` in a canary, only
-	// `spec.rules`
 	desiredCanaryIngress := &extensionsv1beta1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        name,
@@ -148,6 +152,14 @@ func (r *Reconciler) buildLegacyCanaryIngress(stableIngress *extensionsv1beta1.I
 		},
 	}
 
+	// Preserve TLS from stable ingress
+	if stableIngress.Spec.TLS != nil {
+		desiredCanaryIngress.Spec.TLS = make([]extensionsv1beta1.IngressTLS, len(stableIngress.Spec.TLS))
+		for it := 0; it < len(stableIngress.Spec.TLS); it++ {
+			stableIngress.Spec.TLS[it].DeepCopyInto(&desiredCanaryIngress.Spec.TLS[it])
+		}
+	}
+
 	// Preserve ingressClassName from stable ingress
 	if stableIngress.Spec.IngressClassName != nil {
 		desiredCanaryIngress.Spec.IngressClassName = stableIngress.Spec.IngressClassName

diff --git a/rollout/trafficrouting/nginx/nginx_test.go b/rollout/trafficrouting/nginx/nginx_test.go
@@ -212,6 +212,48 @@ func checkBackendServiceLegacy(t *testing.T, ing *extensionsv1beta1.Ingress, ser
 	assert.Fail(t, msg)
 }
 
+func checkTLS(t *testing.T, ing *ingressutil.Ingress, hosts [][]string, secretNames []string) {
+	t.Helper()
+	switch ing.Mode() {
+	case ingressutil.IngressModeNetworking:
+		networkingIngress, err := ing.GetNetworkingIngress()
+		if err != nil {
+			t.Error(err)
+		}
+		checkIngressTLS(t, networkingIngress, hosts, secretNames)
+	case ingressutil.IngressModeExtensions:
+		extensionsIngress, err := ing.GetExtensionsIngress()
+		if err != nil {
+			t.Error(err)
+		}
+		checkTLSLegacy(t, extensionsIngress, hosts, secretNames)
+	}
+}
+
+func checkIngressTLS(t *testing.T, ing *networkingv1.Ingress, hosts [][]string, secretNames []string) {
+	t.Helper()
+	assert.Equal(t, len(hosts), len(ing.Spec.TLS), "Count of TLS rules differs")
+	for it := 0; it < len(ing.Spec.TLS); it++ {
+		assert.Equal(t, secretNames[it], ing.Spec.TLS[it].SecretName, "Secret name differs")
+		assert.Equal(t, len(hosts[it]), len(ing.Spec.TLS[it].Hosts), "Count of hosts differs")
+		for ih := 0; ih < len(ing.Spec.TLS[it].Hosts); ih++ {
+			assert.Equal(t, hosts[it][ih], ing.Spec.TLS[it].Hosts[ih])
+		}
+	}
+}
+
+func checkTLSLegacy(t *testing.T, ing *extensionsv1beta1.Ingress, hosts [][]string, secretNames []string) {
+	t.Helper()
+	assert.Equal(t, len(hosts), len(ing.Spec.TLS), "Count of TLS rules differs")
+	for it := 0; it < len(ing.Spec.TLS); it++ {
+		assert.Equal(t, secretNames[it], ing.Spec.TLS[it].SecretName, "Secret name differs")
+		assert.Equal(t, len(hosts[it]), len(ing.Spec.TLS[it].Hosts), "Count of hosts differs")
+		for ih := 0; ih < len(ing.Spec.TLS[it].Hosts); ih++ {
+			assert.Equal(t, hosts[it][ih], ing.Spec.TLS[it].Hosts[ih])
+		}
+	}
+}
+
 func TestCanaryIngressCreate(t *testing.T) {
 	tests := generateMultiIngressTestData()
 	for _, test := range tests {
@@ -347,6 +389,159 @@ func TestCanaryIngressRetainIngressClass(t *testing.T) {
 	}
 }
 
+func TestCanaryIngressRetainTLS(t *testing.T) {
+	r := Reconciler{
+		cfg: ReconcilerConfig{
+			Rollout: fakeRollout(stableService, canaryService, StableIngress, nil),
+		},
+	}
+	stable := networkingIngress(StableIngress, 80, stableService)
+	stable.Spec.TLS = []networkingv1.IngressTLS{
+		{
+			Hosts:      []string{"fakehost.example.com"},
+			SecretName: "tls-secret-name",
+		},
+		{
+			Hosts:      []string{"fakehost.example.com", "*.example.com"},
+			SecretName: "tls-secret-name-two",
+		},
+	}
+	stableIngress := ingressutil.NewIngress(stable)
+
+	desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), StableIngress), 15)
+	assert.Nil(t, err, "No error returned when calling canaryIngress")
+
+	checkTLS(t, desiredCanaryIngress, [][]string{
+		{"fakehost.example.com"},
+		{"fakehost.example.com", "*.example.com"},
+	}, []string{
+		"tls-secret-name",
+		"tls-secret-name-two",
+	})
+}
+
+func TestCanaryIngressRetainTLSWithMultipleStableIngresses(t *testing.T) {
+	r := Reconciler{
+		cfg: ReconcilerConfig{
+			Rollout: fakeRollout(stableService, canaryService, StableIngress, []string{StableIngresses}),
+		},
+	}
+
+	// main
+	stable := networkingIngress(StableIngress, 80, stableService)
+	stable.Spec.TLS = []networkingv1.IngressTLS{
+		{
+			Hosts:      []string{"fakehost.example.com"},
+			SecretName: "tls-secret-name",
+		},
+	}
+	stableIngress := ingressutil.NewIngress(stable)
+
+	desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), StableIngress), 15)
+	assert.Nil(t, err, "No error returned when calling canaryIngress")
+
+	checkTLS(t, desiredCanaryIngress, [][]string{
+		{"fakehost.example.com"},
+	}, []string{
+		"tls-secret-name",
+	})
+
+	// additional
+	stableAdditional := networkingIngress(StableIngresses, 80, stableService)
+	stableAdditional.Spec.TLS = []networkingv1.IngressTLS{
+		{
+			Hosts:      []string{"fakehost-additional.example.com"},
+			SecretName: "tls-secret-name-additional",
+		},
+	}
+	stableAdditionalIngress := ingressutil.NewIngress(stableAdditional)
+
+	desiredAdditionalCanaryIngress, err := r.canaryIngress(stableAdditionalIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), StableIngresses), 15)
+	assert.Nil(t, err, "No error returned when calling canaryIngress")
+
+	checkTLS(t, desiredAdditionalCanaryIngress, [][]string{
+		{"fakehost-additional.example.com"},
+	}, []string{
+		"tls-secret-name-additional",
+	})
+
+}
+
+func TestCanaryIngressRetainLegacyTLS(t *testing.T) {
+	r := Reconciler{
+		cfg: ReconcilerConfig{
+			Rollout: fakeRollout(stableService, canaryService, StableIngress, nil),
+		},
+	}
+	stable := extensionsIngress(StableIngress, 80, stableService)
+	stable.Spec.TLS = []extensionsv1beta1.IngressTLS{
+		{
+			Hosts:      []string{"fakehost.example.com"},
+			SecretName: "tls-secret-name",
+		},
+		{
+			Hosts:      []string{"fakehost.example.com", "*.example.com"},
+			SecretName: "tls-secret-name-two",
+		},
+	}
+	stableIngress := ingressutil.NewLegacyIngress(stable)
+
+	desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), StableIngress), 15)
+	assert.Nil(t, err, "No error returned when calling canaryIngress")
+
+	checkTLS(t, desiredCanaryIngress, [][]string{
+		{"fakehost.example.com"},
+		{"fakehost.example.com", "*.example.com"},
+	}, []string{
+		"tls-secret-name",
+		"tls-secret-name-two",
+	})
+}
+
+func TestCanaryIngressNotAddTLS(t *testing.T) {
+	r := Reconciler{
+		cfg: ReconcilerConfig{
+			Rollout: fakeRollout(stableService, canaryService, StableIngress, nil),
+		},
+	}
+	stable := networkingIngress(StableIngress, 80, stableService)
+
+	stableIngress := ingressutil.NewIngress(stable)
+
+	desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), StableIngress), 15)
+	assert.Nil(t, err, "No error returned when calling canaryIngress")
+
+	desired, err := desiredCanaryIngress.GetNetworkingIngress()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	assert.Nil(t, desired.Spec.TLS, "No TLS in the the canary ingress should be present")
+}
+
+func TestCanaryIngressNotAddLegacyTLS(t *testing.T) {
+	r := Reconciler{
+		cfg: ReconcilerConfig{
+			Rollout: fakeRollout(stableService, canaryService, StableIngress, nil),
+		},
+	}
+	stable := extensionsIngress(StableIngress, 80, stableService)
+
+	stableIngress := ingressutil.NewLegacyIngress(stable)
+
+	desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), StableIngress), 15)
+	assert.Nil(t, err, "No error returned when calling canaryIngress")
+
+	desired, err := desiredCanaryIngress.GetExtensionsIngress()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	assert.Nil(t, desired.Spec.TLS, "No TLS in the the canary ingress should be present")
+}
+
 func TestCanaryIngressAdditionalAnnotations(t *testing.T) {
 	tests := generateMultiIngressTestData()
 	for _, test := range tests {