From 5f5860a636334dde18eb931394c0828c7c2937e3 Mon Sep 17 00:00:00 2001 From: zachaller Date: Mon, 3 Apr 2023 13:22:12 -0500 Subject: [PATCH 1/5] fix for dropping fields during removing routes Signed-off-by: zachaller --- rollout/trafficrouting/istio/istio.go | 14 +++++++------- rollout/trafficrouting/istio/istio_test.go | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index c13426782b..4d52f604b2 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -1391,8 +1391,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 { @@ -1406,7 +1406,7 @@ func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes for _, route := range httpRoutes { var found bool = false for _, managedRoute := range managedRoutes { - if route.Name == managedRoute.Name { + if route["name"] == managedRoute.Name { httpRoutesWithinManagedRoutes = append(httpRoutesWithinManagedRoutes, route) found = true break @@ -1423,11 +1423,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) } } @@ -1442,7 +1442,7 @@ func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1 // 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 { + if name == routeTyped["name"] || (name == "" && routeTyped["name"] == nil) { orderedInterfaceVSVCHTTPRoutes = append(orderedInterfaceVSVCHTTPRoutes, route) } } diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go index fa4330f363..b13d472944 100644 --- a/rollout/trafficrouting/istio/istio_test.go +++ b/rollout/trafficrouting/istio/istio_test.go @@ -883,6 +883,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) { From 6e89560cd58e76aed89778107965acb791bbdecf Mon Sep 17 00:00:00 2001 From: zachaller Date: Tue, 4 Apr 2023 09:29:16 -0500 Subject: [PATCH 2/5] add comment and rename Signed-off-by: zachaller --- rollout/trafficrouting/istio/istio.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index 4d52f604b2..76a58c56e6 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -1436,13 +1436,15 @@ 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"] || (name == "" && routeTyped["name"] == nil) { + // 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 == routeMap["name"] || (name == "" && routeMap["name"] == nil) { orderedInterfaceVSVCHTTPRoutes = append(orderedInterfaceVSVCHTTPRoutes, route) } } From a2d1b0de98f3d81bac681172c24ab28816d0585d Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 4 May 2023 09:42:37 -0500 Subject: [PATCH 3/5] add a light weight cast check Signed-off-by: zachaller --- rollout/trafficrouting/istio/istio.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index 76a58c56e6..ac6fdb846d 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -1406,7 +1406,9 @@ 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 + name, _ := route["name"].(string) + if name == managedRoute.Name { httpRoutesWithinManagedRoutes = append(httpRoutesWithinManagedRoutes, route) found = true break @@ -1440,7 +1442,7 @@ func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1 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 + // Not checking the cast success here is ok because it covers the case when the route has no name name, _ := r["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 From 0974afa05bbf98eacd90c1b834bafad4460f2e48 Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 4 May 2023 10:08:47 -0500 Subject: [PATCH 4/5] improve logic by adding type casting check Signed-off-by: zachaller --- rollout/trafficrouting/istio/istio.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index ac6fdb846d..7c1bc27dbf 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -1443,10 +1443,11 @@ func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1 r := route.(map[string]interface{}) // Not checking the cast success here is ok because it covers the case when the route has no name - name, _ := r["name"].(string) + 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 == routeMap["name"] || (name == "" && routeMap["name"] == nil) { + if name == routeMapName || (!rNameOK && !RMapNameOK) { orderedInterfaceVSVCHTTPRoutes = append(orderedInterfaceVSVCHTTPRoutes, route) } } From f4fadbcf8ecd28986bcb740bfab086aa6075bb2a Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 4 May 2023 10:58:37 -0500 Subject: [PATCH 5/5] add docs Signed-off-by: zachaller --- rollout/trafficrouting/istio/istio.go | 1 + 1 file changed, 1 insertion(+) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index 7c1bc27dbf..0449c62dff 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -1407,6 +1407,7 @@ func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes var found bool = false for _, managedRoute := range managedRoutes { // 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)