From 23706d6732cfe0057e0dec7dee87a3bd37d0258e Mon Sep 17 00:00:00 2001 From: Dennis Nguyen Date: Wed, 25 Oct 2023 10:33:32 -0700 Subject: [PATCH 1/3] istio destionationrule subsets enforcement Signed-off-by: Dennis Nguyen --- rollout/trafficrouting/istio/istio.go | 17 +++++ rollout/trafficrouting/istio/istio_test.go | 85 ++++++++++++++++++++++ 2 files changed, 102 insertions(+) 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..3cf911915f 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,66 @@ 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 TestHttpReconcileHeaderRouteHostBased(t *testing.T) { ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"}) obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvc) From b4603bd8fde89b73487d94b5c81115589ab470f9 Mon Sep 17 00:00:00 2001 From: Dennis Nguyen Date: Wed, 25 Oct 2023 14:16:38 -0700 Subject: [PATCH 2/3] add users Signed-off-by: Dennis Nguyen --- USERS.md | 1 + 1 file changed, 1 insertion(+) 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) From 8402330130ba38f02beeb35667b4a0e0b6b3fae2 Mon Sep 17 00:00:00 2001 From: Dennis Nguyen Date: Tue, 31 Oct 2023 21:29:40 -0700 Subject: [PATCH 3/3] add test case for failure cases Signed-off-by: Dennis Nguyen --- rollout/trafficrouting/istio/istio_test.go | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go index 3cf911915f..ead1b7e1ee 100644 --- a/rollout/trafficrouting/istio/istio_test.go +++ b/rollout/trafficrouting/istio/istio_test.go @@ -780,6 +780,31 @@ spec: 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)