From b0b95cade830d8a70624fcaaf8e92e1fe2ecd355 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Thu, 29 Sep 2022 09:16:47 -0500 Subject: [PATCH] fixes(controller): istio dropping fields not defined in type (#2268) * fixes #2266 Signed-off-by: zachaller * typo Signed-off-by: zachaller * test after calling set mirror Signed-off-by: zachaller * Keep port on user defined service by adding to internal types Signed-off-by: zachaller * github trigger re-run Signed-off-by: zachaller * Drop port on virtual service to be consistent with added routes Drop the port on the virtual service with the statment that rollouts only supports one port services. Signed-off-by: zachaller * cleanup test to just test extra fields and add a port check on destination as well Signed-off-by: zachaller * keep port for both mirroring and header routes Signed-off-by: zachaller * github trigger re-run Signed-off-by: zachaller * github trigger re-run Signed-off-by: zachaller * add function comment Signed-off-by: zachaller Signed-off-by: zachaller --- rollout/trafficrouting/istio/istio.go | 87 +++++++--- rollout/trafficrouting/istio/istio_test.go | 181 ++++++++++++++++++++ rollout/trafficrouting/istio/istio_types.go | 5 + 3 files changed, 251 insertions(+), 22 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index f001c585a3..c03bc41f41 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -652,7 +652,7 @@ func (r *Reconciler) getVirtualService(namespace string, vsvcName string, client return vsvc, err } -func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(obj *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute) error { +func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1.IstioVirtualService, obj *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute) error { // HTTP Routes httpRoutesI, err := GetHttpRoutesI(obj) if err != nil { @@ -687,7 +687,7 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(obj *unstructured.Unstr return fmt.Errorf("[reconcileVirtualServiceHeaderRoutes] failed to remove http route from virtual service: %w", err) } - httpRoutesI = append(httpRoutesI, createHeaderRoute(headerRouting, canarySvc, canarySubset)) + httpRoutesI = append(httpRoutesI, createHeaderRoute(virtualService, obj, headerRouting, canarySvc, canarySubset)) err = unstructured.SetNestedSlice(obj.Object, httpRoutesI, "spec", Http) if err != nil { @@ -712,7 +712,7 @@ func (r *Reconciler) SetHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute) erro return fmt.Errorf("[SetHeaderRoute] failed to get istio virtual service: %w", err) } - err = r.reconcileVirtualServiceHeaderRoutes(vsvc, headerRouting) + err = r.reconcileVirtualServiceHeaderRoutes(virtualService, vsvc, headerRouting) if err != nil { return fmt.Errorf("[SetHeaderRoute] failed to reconcile header routes: %w", err) } @@ -765,12 +765,19 @@ func (r *Reconciler) getDestinationRule(dRuleSpec *v1alpha1.IstioDestinationRule return origBytes, dRule, dRuleNew, nil } -func createHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute, host string, subset string) map[string]interface{} { +func createHeaderRoute(virtualService v1alpha1.IstioVirtualService, unVsvc *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute, host string, subset string) map[string]interface{} { var routeMatches []interface{} for _, hrm := range headerRouting.Match { routeMatches = append(routeMatches, createHeaderRouteMatch(hrm)) } - canaryDestination := routeDestination(host, subset, 100) + + port, err := getVirtualServiceCanaryPort(unVsvc, virtualService) + if err != nil { + port = Port{Number: 0} + } + + canaryDestination := routeDestination(host, port.Number, subset, 100) + return map[string]interface{}{ "name": headerRouting.Name, "match": routeMatches, @@ -795,10 +802,13 @@ func setMapValueIfNotEmpty(m map[string]interface{}, key string, value string) { } } -func routeDestination(host, subset string, weight int64) map[string]interface{} { +func routeDestination(host string, port uint32, subset string, weight int64) map[string]interface{} { dest := map[string]interface{}{ "host": host, } + if port > 0 { + dest["port"] = map[string]interface{}{"number": int64(port)} + } if subset != "" { dest["subset"] = subset } @@ -1141,14 +1151,20 @@ func createMirrorRoute(virtualService v1alpha1.IstioVirtualService, httpRoutes [ }) } + mirrorDestinations := VirtualServiceDestination{ + Host: canarySvc, + Subset: subset, + } + if len(route) >= 0 && route[0].Destination.Port != nil { + // We try to pull the port from any of the routes destinations that are supposed to be updated via SetWeight + mirrorDestinations.Port = &Port{Number: route[0].Destination.Port.Number} + } + mirrorRoute := map[string]interface{}{ - "name": mirrorRouting.Name, - "match": istioMatch, - "route": route, - "mirror": VirtualServiceDestination{ - Host: canarySvc, - Subset: subset, - }, + "name": mirrorRouting.Name, + "match": istioMatch, + "route": route, + "mirror": mirrorDestinations, "mirrorPercentage": map[string]interface{}{"value": float64(percent)}, } @@ -1232,7 +1248,7 @@ func (r *Reconciler) orderRoutes(istioVirtualService *unstructured.Unstructured) return fmt.Errorf("[orderRoutes] could not split routes between managed and non managed: %w", err) } - finalRoutes, err := getOrderedVirtualServiceRoutes(managedRoutes, httpRoutesWithinManagedRoutes, httpRoutesNotWithinManagedRoutes) + finalRoutes, err := getOrderedVirtualServiceRoutes(httpRouteI, managedRoutes, httpRoutesWithinManagedRoutes, httpRoutesNotWithinManagedRoutes) if err != nil { return fmt.Errorf("[orderRoutes] could not get ordered virtual service routes: %w", err) } @@ -1279,7 +1295,7 @@ 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(managedRoutes []v1alpha1.MangedRoutes, httpRoutesWithinManagedRoutes []VirtualServiceHTTPRoute, httpRoutesNotWithinManagedRoutes []VirtualServiceHTTPRoute) ([]interface{}, error) { +func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1alpha1.MangedRoutes, httpRoutesWithinManagedRoutes []VirtualServiceHTTPRoute, httpRoutesNotWithinManagedRoutes []VirtualServiceHTTPRoute) ([]interface{}, error) { var orderedManagedRoutes []VirtualServiceHTTPRoute for _, route := range managedRoutes { for _, managedRoute := range httpRoutesWithinManagedRoutes { @@ -1289,18 +1305,45 @@ func getOrderedVirtualServiceRoutes(managedRoutes []v1alpha1.MangedRoutes, httpR } } - allIstioRoutes := append(orderedManagedRoutes, httpRoutesNotWithinManagedRoutes...) + orderedVirtualServiceHTTPRoutes := append(orderedManagedRoutes, httpRoutesNotWithinManagedRoutes...) - jsonAllIstioRoutes, err := json.Marshal(allIstioRoutes) + var orderedInterfaceVSVCHTTPRoutes []interface{} + for _, routeTyped := 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 { + orderedInterfaceVSVCHTTPRoutes = append(orderedInterfaceVSVCHTTPRoutes, route) + } + } + } + + return orderedInterfaceVSVCHTTPRoutes, nil +} + +// getVirtualServiceCanaryPort This function returns the port that the canary service is running on. It does this by looking at the +// istio Virtual Service and finding any port from a destination that is suppose to be update via SetWeight. +func getVirtualServiceCanaryPort(unVsvc *unstructured.Unstructured, virtualService v1alpha1.IstioVirtualService) (Port, error) { + httpRoutes, _, err := getVirtualServiceHttpRoutes(unVsvc) if err != nil { - return nil, fmt.Errorf("[getOrderedVirtualServiceRoutes] failed to marsharl istio routes: %w", err) + return Port{}, fmt.Errorf("[getVirtualServiceCanaryPort] failed to get virtual service http routes: %w", err) } - var orderedRoutes []interface{} - if err := json.Unmarshal(jsonAllIstioRoutes, &orderedRoutes); err != nil { - return nil, fmt.Errorf("[getOrderedVirtualServiceRoutes] failed to unmarsharl istio routes: %w", err) + + route, err := getVirtualServiceSetWeightRoute(virtualService.Routes, httpRoutes) + if err != nil { + return Port{}, fmt.Errorf("[getVirtualServiceCanaryPort] failed to get virtual service set weight route: %w", err) + } + + var port uint32 = 0 + if len(route) > 0 && route[0].Destination.Port != nil { + port = route[0].Destination.Port.Number } - return orderedRoutes, nil + return Port{ + Number: port, + }, nil } // RemoveManagedRoutes this removes all the routes in all the istio virtual services rollouts is managing by getting two slices diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go index 3c57dcfbdb..bde05ff034 100644 --- a/rollout/trafficrouting/istio/istio_test.go +++ b/rollout/trafficrouting/istio/istio_test.go @@ -603,6 +603,80 @@ spec: assert.Equal(t, httpRoutes[0].Route[0].Destination.Subset, "canary-subset") } +func TestHttpReconcileHeaderRouteWithExtra(t *testing.T) { + ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"}) + obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvcWithExtra) + 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, + }, + }...) + + // Test for both the HTTP VS & Mixed VS + hr := &v1alpha1.SetHeaderRoute{ + Name: headerName, + Match: []v1alpha1.HeaderRoutingMatch{ + { + HeaderName: "agent", + HeaderValue: &v1alpha1.StringMatch{Exact: "firefox"}, + }, + }, + } + + err := r.SetHeaderRoute(hr) + 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) + checkDestination(t, httpRoutes[0].Route, "canary", 100) + assert.Equal(t, len(httpRoutes[0].Route), 1) + assert.Equal(t, httpRoutes[1].Name, "primary") + checkDestination(t, httpRoutes[1].Route, "stable", 100) + assert.Equal(t, httpRoutes[2].Name, "secondary") + + 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") + + 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 == int64(8443)) + + r1 := routes[1].(map[string]interface{}) + _, found = r1["retries"] + assert.True(t, found) + + r2 := routes[2].(map[string]interface{}) + _, found = r2["retries"] + assert.True(t, found) + _, found = r2["corsPolicy"] + assert.True(t, found) + +} + func TestReconcileUpdateHeader(t *testing.T) { ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"}) ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = append(ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes, v1alpha1.MangedRoutes{ @@ -610,6 +684,7 @@ func TestReconcileUpdateHeader(t *testing.T) { }) AssertReconcileUpdateHeader(t, regularVsvc, ro) } + func AssertReconcileUpdateHeader(t *testing.T, vsvc string, ro *v1alpha1.Rollout) *dynamicfake.FakeDynamicClient { obj := unstructuredutil.StrToUnstructuredUnsafe(vsvc) client := testutil.NewFakeDynamicClient(obj) @@ -1642,6 +1717,60 @@ spec: host: canary weight: 0` +const regularVsvcWithExtra = `apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: vsvc + namespace: default +spec: + gateways: + - istio-rollout-gateway + hosts: + - istio-rollout.dev.argoproj.io + http: + - name: primary + retries: + attempts: 3 + perTryTimeout: 10s + retryOn: 'gateway-error,connect-failure,refused-stream' + route: + - destination: + host: 'stable' + port: + number: 8443 + weight: 100 + - destination: + host: canary + port: + number: 8443 + weight: 0 + - name: secondary + retries: + attempts: 3 + perTryTimeout: 10s + retryOn: 'gateway-error,connect-failure,refused-stream' + corsPolicy: + allowOrigins: + - exact: https://example.com + allowMethods: + - POST + - GET + allowCredentials: false + allowHeaders: + - X-Foo-Bar + maxAge: "24h" + route: + - destination: + host: 'stable' + port: + number: 8443 + weight: 100 + - destination: + host: canary + port: + number: 8443 + weight: 0` + func TestMultipleVirtualServiceConfigured(t *testing.T) { multipleVirtualService := []v1alpha1.IstioVirtualService{{Name: "vsvc1", Routes: []string{"primary", "secondary"}}, {Name: "vsvc2", Routes: []string{"blue-green"}}} ro := multiVsRollout("stable", "canary", multipleVirtualService) @@ -1874,6 +2003,58 @@ func TestHttpReconcileMirrorRoute(t *testing.T) { } +func TestHttpReconcileMirrorRouteWithExtraFields(t *testing.T) { + ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"}) + obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvcWithExtra) + client := testutil.NewFakeDynamicClient(obj) + vsvcLister, druleLister := getIstioListers(client) + r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister) + client.ClearActions() + + // Test for both the HTTP VS & Mixed VS + setMirror1 := &v1alpha1.SetMirrorRoute{ + Name: "test-mirror-1", + Match: []v1alpha1.RouteMatch{{ + Method: &v1alpha1.StringMatch{ + Exact: "GET", + }, + }}, + } + r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = append(r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes, []v1alpha1.MangedRoutes{{ + Name: "test-mirror-1", + }, + }...) + + err := r.SetMirrorRoute(setMirror1) + 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) + + routes, found, err := unstructured.NestedSlice(iVirtualService.Object, "spec", "http") + assert.NoError(t, err) + assert.True(t, found) + + r0 := routes[0].(map[string]interface{}) + mirrorRoute, found := r0["route"].([]interface{}) + assert.True(t, found) + + port1 := mirrorRoute[0].(map[string]interface{})["destination"].(map[string]interface{})["port"].(map[string]interface{})["number"] + port2 := mirrorRoute[1].(map[string]interface{})["destination"].(map[string]interface{})["port"].(map[string]interface{})["number"] + assert.True(t, port1 == float64(8443)) + assert.True(t, port2 == float64(8443)) + + r1 := routes[1].(map[string]interface{}) + _, found = r1["retries"] + assert.True(t, found) + + r2 := routes[2].(map[string]interface{}) + _, found = r2["retries"] + assert.True(t, found) + _, found = r2["corsPolicy"] + assert.True(t, found) + +} + func TestHttpReconcileMirrorRouteOrder(t *testing.T) { ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary", "secondary"}) obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvc) diff --git a/rollout/trafficrouting/istio/istio_types.go b/rollout/trafficrouting/istio/istio_types.go index ce714757d2..bb09693177 100644 --- a/rollout/trafficrouting/istio/istio_types.go +++ b/rollout/trafficrouting/istio/istio_types.go @@ -70,6 +70,11 @@ type VirtualServiceRouteDestination struct { type VirtualServiceDestination struct { Host string `json:"host,omitempty"` Subset string `json:"subset,omitempty"` + Port *Port `json:"port,omitempty"` +} + +type Port struct { + Number uint32 `json:"number,omitempty"` } // DestinationRule is an Istio DestinationRule containing only the fields which we care about