feat(trafficrouting): use values array for multiple accepted values u…

…nder same header name (#2974) * feat(headername): allow traffic forward condition string to append to matching headername conditions Signed-off-by: Dylan Schlager <[email protected]> * chore: remove unnecessary comments from tests Signed-off-by: Dylan Schlager <[email protected]> * chore: fix linting failures Signed-off-by: Dylan Schlager <[email protected]> --------- Signed-off-by: Dylan Schlager <[email protected]>
argoproj · Aug 25, 2023 · f28d70c · f28d70c
1 parent 5e2249f
commit f28d70c
Show file tree

Hide file tree

Showing 2 changed files with 121 additions and 8 deletions.
diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go
@@ -515,17 +515,28 @@ func getTrafficForwardActionString(r *v1alpha1.Rollout, port int32) (string, err
 	return string(bytes), nil
 }
 
+// Two exact matches with the same header name should be merged into the values array of the same condition
+func upsertCondition(res []ingressutil.ALBCondition, match v1alpha1.HeaderRoutingMatch) []ingressutil.ALBCondition {
+	for i, condition := range res {
+		if condition.HttpHeaderConfig.HttpHeaderName == match.HeaderName {
+			res[i].HttpHeaderConfig.Values = append(res[i].HttpHeaderConfig.Values, match.HeaderValue.Exact)
+			return res
+		}
+	}
+	condition := ingressutil.ALBCondition{
+		Field: "http-header",
+		HttpHeaderConfig: ingressutil.HttpHeaderConfig{
+			HttpHeaderName: match.HeaderName,
+			Values:         []string{match.HeaderValue.Exact},
+		},
+	}
+	return append(res, condition)
+}
+
 func getTrafficForwardConditionString(headerRoute *v1alpha1.SetHeaderRoute) (string, error) {
 	var res []ingressutil.ALBCondition
 	for _, match := range headerRoute.Match {
-		condition := ingressutil.ALBCondition{
-			Field: "http-header",
-			HttpHeaderConfig: ingressutil.HttpHeaderConfig{
-				HttpHeaderName: match.HeaderName,
-				Values:         []string{match.HeaderValue.Exact},
-			},
-		}
-		res = append(res, condition)
+		res = upsertCondition(res, match)
 	}
 	bytes := jsonutil.MustMarshal(res)
 	return string(bytes), nil

diff --git a/rollout/trafficrouting/alb/alb_test.go b/rollout/trafficrouting/alb/alb_test.go
@@ -1866,6 +1866,108 @@ func TestSetHeaderRoute(t *testing.T) {
 	assert.Len(t, client.Actions(), 1)
 }
 
+func TestSetHeaderRouteWithDifferentHeaderNames(t *testing.T) {
+	ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443)
+	ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{
+		{Name: "header-route"},
+	}
+
+	i := ingress("ingress", STABLE_SVC, CANARY_SVC, "action1", 443, 10, ro.Name, false)
+	client := fake.NewSimpleClientset(i)
+	k8sI := kubeinformers.NewSharedInformerFactory(client, 0)
+	k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i)
+
+	ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	r, err := NewReconciler(ReconcilerConfig{
+		Rollout:        ro,
+		Client:         client,
+		Recorder:       record.NewFakeEventRecorder(),
+		ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"},
+		IngressWrapper: ingressWrapper,
+	})
+	assert.NoError(t, err)
+
+	err = r.SetHeaderRoute(&v1alpha1.SetHeaderRoute{
+		Name: "header-route",
+		Match: []v1alpha1.HeaderRoutingMatch{
+			{
+				HeaderName: "origin",
+				HeaderValue: &v1alpha1.StringMatch{
+					Exact: "https://www.fake-origin1.com",
+				},
+			},
+			{
+				HeaderName: "Agent",
+				HeaderValue: &v1alpha1.StringMatch{
+					Exact: "Chrome",
+				},
+			},
+		},
+	})
+	assert.Nil(t, err)
+	assert.Len(t, client.Actions(), 1)
+
+	// no managed routes, no changes expected
+	err = r.RemoveManagedRoutes()
+	assert.Nil(t, err)
+	assert.Len(t, client.Actions(), 1)
+}
+
+func TestSetHeaderRouteWithDuplicateHeaderNameMatches(t *testing.T) {
+	ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443)
+	ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{
+		{Name: "header-route"},
+	}
+
+	i := ingress("ingress", STABLE_SVC, CANARY_SVC, "action1", 443, 10, ro.Name, false)
+	client := fake.NewSimpleClientset(i)
+	k8sI := kubeinformers.NewSharedInformerFactory(client, 0)
+	k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i)
+
+	ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	r, err := NewReconciler(ReconcilerConfig{
+		Rollout:        ro,
+		Client:         client,
+		Recorder:       record.NewFakeEventRecorder(),
+		ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"},
+		IngressWrapper: ingressWrapper,
+	})
+	assert.NoError(t, err)
+
+	err = r.SetHeaderRoute(&v1alpha1.SetHeaderRoute{
+		Name: "header-route",
+		Match: []v1alpha1.HeaderRoutingMatch{
+			{
+				HeaderName: "origin",
+				HeaderValue: &v1alpha1.StringMatch{
+					Exact: "https://www.fake-origin1.com",
+				},
+			},
+			{
+				HeaderName: "origin",
+				HeaderValue: &v1alpha1.StringMatch{
+					Exact: "https://www.fake-origin2.com",
+				},
+			},
+		},
+	})
+	assert.Nil(t, err)
+	assert.Len(t, client.Actions(), 1)
+
+	// no managed routes, no changes expected
+	err = r.RemoveManagedRoutes()
+	assert.Nil(t, err)
+	assert.Len(t, client.Actions(), 1)
+}
+
 func TestSetHeaderRouteMultiIngress(t *testing.T) {
 	ro := fakeRolloutWithMultiIngress(STABLE_SVC, CANARY_SVC, nil, []string{"ingress", "multi-ingress"}, 443)
 	ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{