fix: get new httpRoutesI after removeRoute() to avoid duplicates. Fixes

#2769 (#2887) fix: get new httpRoutesI after removeRoute() to avoid duplicates Signed-off-by: xinpureZhu <[email protected]>
argoproj · Jul 14, 2023 · 3ddb9ef · 3ddb9ef
1 parent c8cd968
commit 3ddb9ef
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 5 deletions.
diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go
@@ -712,11 +712,6 @@ func (r *Reconciler) getVirtualService(namespace string, vsvcName string, client
 }
 
 func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1.IstioVirtualService, obj *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute) error {
-	// HTTP Routes
-	httpRoutesI, err := GetHttpRoutesI(obj)
-	if err != nil {
-		return err
-	}
 	destRuleHost, err := r.getDestinationRuleHost()
 	if err != nil {
 		return err
@@ -746,6 +741,12 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1
 		return fmt.Errorf("[reconcileVirtualServiceHeaderRoutes] failed to remove http route from virtual service: %w", err)
 	}
 
+	// HTTP Routes
+	httpRoutesI, err := GetHttpRoutesI(obj)
+	if err != nil {
+		return err
+	}
+
 	httpRoutesI = append(httpRoutesI, createHeaderRoute(virtualService, obj, headerRouting, canarySvc, canarySubset))
 
 	err = unstructured.SetNestedSlice(obj.Object, httpRoutesI, "spec", Http)

diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go
@@ -2857,3 +2857,46 @@ func AssertReconcileUpdateMirror(t *testing.T, vsvc string, ro *v1alpha1.Rollout
 	assert.Equal(t, "update", actions[0].GetVerb())
 	return client
 }
+
+func TestReconcileHeaderRouteAvoidDuplicates(t *testing.T) {
+	ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"})
+	obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvc)
+	client := testutil.NewFakeDynamicClient(obj)
+	vsvcLister, druleLister := getIstioListers(client)
+	r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister)
+	client.ClearActions()
+
+	const headerName = "test-header-route"
+	r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = append(r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes, []v1alpha1.MangedRoutes{{
+		Name: headerName,
+	},
+	}...)
+
+	var setHeader = &v1alpha1.SetHeaderRoute{
+		Name: headerName,
+		Match: []v1alpha1.HeaderRoutingMatch{
+			{
+				HeaderName: "browser",
+				HeaderValue: &v1alpha1.StringMatch{
+					Prefix: "Firefox",
+				},
+			},
+		},
+	}
+
+	err := r.SetHeaderRoute(setHeader)
+	assert.Nil(t, err)
+
+	err = r.SetHeaderRoute(setHeader)
+	assert.Nil(t, err)
+
+	iVirtualService, err := client.Resource(istioutil.GetIstioVirtualServiceGVR()).Namespace(r.rollout.Namespace).Get(context.TODO(), ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name, metav1.GetOptions{})
+	assert.NoError(t, err)
+	// HTTP Routes
+	httpRoutes := extractHttpRoutes(t, iVirtualService)
+
+	// Assertions
+	assert.Equal(t, httpRoutes[0].Name, headerName)
+	assert.Equal(t, httpRoutes[1].Name, "primary")
+	assert.Equal(t, httpRoutes[2].Name, "secondary")
+}