From 04e11195fb4046e897574478c51fb14692a07578 Mon Sep 17 00:00:00 2001 From: Dennis Nguyen <1750375+dnguy078@users.noreply.github.com> Date: Wed, 1 Nov 2023 07:03:46 -0700 Subject: [PATCH] fix: istio destionationrule subsets enforcement (#3126) * istio destionationrule subsets enforcement Signed-off-by: Dennis Nguyen * add users Signed-off-by: Dennis Nguyen * add test case for failure cases Signed-off-by: Dennis Nguyen --------- Signed-off-by: Dennis Nguyen --- USERS.md | 1 + rollout/trafficrouting/istio/istio.go | 17 ++++ rollout/trafficrouting/istio/istio_test.go | 110 +++++++++++++++++++++ 3 files changed, 128 insertions(+) diff --git a/USERS.md b/USERS.md index 4ad9fb29bf..5ca758b084 100644 --- a/USERS.md +++ b/USERS.md @@ -22,6 +22,7 @@ Organizations below are **officially** using Argo Rollouts. Please send a PR wit 1. [Ibotta](https://home.ibotta.com/) 1. [Intuit](https://www.intuit.com/) 1. [New Relic](https://newrelic.com/) +1. [Nextdoor](https://nextdoor.com/) 1. [Nitro](https://www.gonitro.com) 1. [Nozzle](https://nozzle.io) 1. [Opensurvey Inc.](https://opensurvey.co.kr) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index dd44da7b25..8a47a1011d 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -255,6 +255,23 @@ func (r *Reconciler) reconcileVirtualService(obj *unstructured.Unstructured, vsv if err = ValidateHTTPRoutes(r.rollout, vsvcRouteNames, httpRoutes); err != nil { return nil, false, err } + + host, err := r.getDestinationRuleHost() + if err != nil { + return nil, false, err + } + + if host != "" { + var routeDestinations []VirtualServiceRouteDestination + for i, routes := range httpRoutes { + for _, r := range routes.Route { + if r.Destination.Host == host { + routeDestinations = append(routeDestinations, VirtualServiceRouteDestination{Destination: r.Destination, Weight: r.Weight}) + } + } + httpRoutes[i].Route = routeDestinations + } + } } // TLS Routes diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go index c18cdedae5..ead1b7e1ee 100644 --- a/rollout/trafficrouting/istio/istio_test.go +++ b/rollout/trafficrouting/istio/istio_test.go @@ -465,6 +465,31 @@ spec: subset: 'canary-subset' weight: 0` +const singleRouteSubsetMultipleDestRuleVsvc = `apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: vsvc + namespace: default +spec: + gateways: + - istio-rollout-gateway + hosts: + - istio-rollout.dev.argoproj.io + http: + - route: + - destination: + host: rollout-service + subset: stable + weight: 100 + - destination: + host: rollout-service + subset: canary + weight: 0 + - destination: + host: additional-service + subset: stable-subset + weight: 20` + const singleRouteTlsVsvc = `apiVersion: networking.istio.io/v1alpha3 kind: VirtualService metadata: @@ -695,6 +720,91 @@ func TestHttpReconcileWeightsBaseCase(t *testing.T) { } } +func TestHttpReconcileMultipleDestRule(t *testing.T) { + ro := rolloutWithDestinationRule() + ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name = "vsvc" + ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes = nil + + dRule1 := unstructuredutil.StrToUnstructuredUnsafe(` +apiVersion: networking.istio.io/v1alpha3 +kind: DestinationRule +metadata: + name: istio-destrule + namespace: default +spec: + host: rollout-service + subsets: + - name: stable + - name: canary +`) + dRule2 := unstructuredutil.StrToUnstructuredUnsafe(` +apiVersion: networking.istio.io/v1alpha3 +kind: DestinationRule +metadata: + name: additional-istio-destrule + namespace: default +spec: + host: additional-service + subsets: + - name: stable-subset +`) + + obj := unstructuredutil.StrToUnstructuredUnsafe(singleRouteSubsetMultipleDestRuleVsvc) + client := testutil.NewFakeDynamicClient(obj, dRule1, dRule2) + vsvcLister, druleLister := getIstioListers(client) + r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister) + client.ClearActions() + + vsvcRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes + vsvcTLSRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.TLSRoutes + vsvcTCPRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.TCPRoutes + modifiedObj, _, err := r.reconcileVirtualService(obj, vsvcRoutes, vsvcTLSRoutes, vsvcTCPRoutes, 10) + assert.Nil(t, err) + assert.NotNil(t, modifiedObj) + + httpRoutes := extractHttpRoutes(t, modifiedObj) + + // Assertions + assert.Equal(t, httpRoutes[0].Route[0].Destination.Host, "rollout-service") + assert.Equal(t, httpRoutes[0].Route[1].Destination.Host, "rollout-service") + if httpRoutes[0].Route[0].Destination.Subset == "stable" || httpRoutes[0].Route[1].Destination.Subset == "canary" { + assert.Equal(t, httpRoutes[0].Route[0].Weight, int64(90)) + assert.Equal(t, httpRoutes[0].Route[1].Weight, int64(10)) + } else { + assert.Equal(t, httpRoutes[0].Route[0].Weight, int64(10)) + assert.Equal(t, httpRoutes[0].Route[1].Weight, int64(90)) + } + + assert.Equal(t, httpRoutes[0].Route[2].Destination.Host, "additional-service") + assert.Equal(t, httpRoutes[0].Route[2].Destination.Subset, "stable-subset") + assert.Equal(t, httpRoutes[0].Route[2].Weight, int64(20)) +} + +func TestHttpReconcileInvalidMultipleDestRule(t *testing.T) { + ro := rolloutWithDestinationRule() + // set to invalid destination rule + ro.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule = &v1alpha1.IstioDestinationRule{ + Name: "invalid-destination-rule", + CanarySubsetName: "canary", + StableSubsetName: "stable", + } + + ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name = "vsvc" + ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes = nil + + obj := unstructuredutil.StrToUnstructuredUnsafe(singleRouteSubsetMultipleDestRuleVsvc) + client := testutil.NewFakeDynamicClient(obj) + vsvcLister, druleLister := getIstioListers(client) + r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister) + client.ClearActions() + + vsvcRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes + vsvcTLSRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.TLSRoutes + vsvcTCPRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.TCPRoutes + _, _, err := r.reconcileVirtualService(obj, vsvcRoutes, vsvcTLSRoutes, vsvcTCPRoutes, 10) + assert.Error(t, err, "expected destination rule not found") +} + func TestHttpReconcileHeaderRouteHostBased(t *testing.T) { ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"}) obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvc)