fix: istio dropping fields during removing of managed routes (#2692)

* fix for dropping fields during removing routes Signed-off-by: zachaller <[email protected]> * add comment and rename Signed-off-by: zachaller <[email protected]> * add a light weight cast check Signed-off-by: zachaller <[email protected]> * improve logic by adding type casting check Signed-off-by: zachaller <[email protected]> * add docs Signed-off-by: zachaller <[email protected]> --------- Signed-off-by: zachaller <[email protected]>
argoproj · May 5, 2023 · ea3309a · ea3309a
1 parent 6cf76b3
commit ea3309a
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 10 deletions.
diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go
@@ -1263,8 +1263,8 @@ func (r *Reconciler) orderRoutes(istioVirtualService *unstructured.Unstructured)
 // splitManagedRoutesAndNonManagedRoutes This splits the routes from an istio virtual service into two slices
 // one slice contains all the routes that are also in the rollouts managedRoutes object and one that contains routes
 // that where only in the virtual service (aka routes that where manually added by user)
-func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes, httpRouteI []interface{}) (httpRoutesWithinManagedRoutes []VirtualServiceHTTPRoute, httpRoutesNotWithinManagedRoutes []VirtualServiceHTTPRoute, err error) {
-	var httpRoutes []VirtualServiceHTTPRoute
+func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes, httpRouteI []interface{}) (httpRoutesWithinManagedRoutes []map[string]interface{}, httpRoutesNotWithinManagedRoutes []map[string]interface{}, err error) {
+	var httpRoutes []map[string]interface{}
 
 	jsonHttpRoutes, err := json.Marshal(httpRouteI)
 	if err != nil {
@@ -1278,7 +1278,10 @@ func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes
 	for _, route := range httpRoutes {
 		var found bool = false
 		for _, managedRoute := range managedRoutes {
-			if route.Name == managedRoute.Name {
+			// Not checking the cast success here is ok because it covers the case when the route has no name
+			// when there is no name the cast return an empty string and will just not match the managed route
+			name, _ := route["name"].(string)
+			if name == managedRoute.Name {
 				httpRoutesWithinManagedRoutes = append(httpRoutesWithinManagedRoutes, route)
 				found = true
 				break
@@ -1295,11 +1298,11 @@ func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes
 // getOrderedVirtualServiceRoutes This returns an []interface{} of istio virtual routes where the routes are ordered based
 // on the rollouts managedRoutes field. We take the routes from the rollouts managedRoutes field order them and place them on top
 // of routes that are manually defined within the virtual service (aka. routes that users have defined manually)
-func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1alpha1.MangedRoutes, httpRoutesWithinManagedRoutes []VirtualServiceHTTPRoute, httpRoutesNotWithinManagedRoutes []VirtualServiceHTTPRoute) ([]interface{}, error) {
-	var orderedManagedRoutes []VirtualServiceHTTPRoute
+func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1alpha1.MangedRoutes, httpRoutesWithinManagedRoutes []map[string]interface{}, httpRoutesNotWithinManagedRoutes []map[string]interface{}) ([]interface{}, error) {
+	var orderedManagedRoutes []map[string]interface{}
 	for _, route := range managedRoutes {
 		for _, managedRoute := range httpRoutesWithinManagedRoutes {
-			if route.Name == managedRoute.Name {
+			if route.Name == managedRoute["name"] {
 				orderedManagedRoutes = append(orderedManagedRoutes, managedRoute)
 			}
 		}
@@ -1308,13 +1311,16 @@ func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1
 	orderedVirtualServiceHTTPRoutes := append(orderedManagedRoutes, httpRoutesNotWithinManagedRoutes...)
 
 	var orderedInterfaceVSVCHTTPRoutes []interface{}
-	for _, routeTyped := range orderedVirtualServiceHTTPRoutes {
+	for _, routeMap := range orderedVirtualServiceHTTPRoutes {
 		for _, route := range httpRouteI {
 			r := route.(map[string]interface{})
 
-			// No need to check if exist because the empty string returned on cast failure is good for this check
-			name, _ := r["name"].(string)
-			if name == routeTyped.Name {
+			// Not checking the cast success here is ok because it covers the case when the route has no name
+			name, rNameOK := r["name"].(string)
+			routeMapName, RMapNameOK := routeMap["name"].(string)
+			// The second or clause is for the case when we have a route that has no name set because this field
+			// is optional in istio virtual service if there is only one route
+			if name == routeMapName || (!rNameOK && !RMapNameOK) {
 				orderedInterfaceVSVCHTTPRoutes = append(orderedInterfaceVSVCHTTPRoutes, route)
 			}
 		}

diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go
@@ -675,6 +675,27 @@ func TestHttpReconcileHeaderRouteWithExtra(t *testing.T) {
 	_, found = r2["corsPolicy"]
 	assert.True(t, found)
 
+	r.RemoveManagedRoutes()
+	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)
+
+	routes, found, err = unstructured.NestedSlice(iVirtualService.Object, "spec", "http")
+	assert.NoError(t, err)
+	assert.True(t, found)
+
+	r0 = routes[0].(map[string]interface{})
+	route, found = r0["route"].([]interface{})
+	assert.True(t, found)
+
+	port1 = route[0].(map[string]interface{})["destination"].(map[string]interface{})["port"].(map[string]interface{})["number"]
+	assert.True(t, port1 == float64(8443))
+
+	r2 = routes[1].(map[string]interface{})
+	_, found = r2["retries"]
+	assert.True(t, found)
+	_, found = r2["corsPolicy"]
+	assert.True(t, found)
+
 }
 
 func TestReconcileUpdateHeader(t *testing.T) {