fix: istio destionationrule subsets enforcement (argoproj#3126)

* istio destionationrule subsets enforcement Signed-off-by: Dennis Nguyen <[email protected]> * add users Signed-off-by: Dennis Nguyen <[email protected]> * add test case for failure cases Signed-off-by: Dennis Nguyen <[email protected]> --------- Signed-off-by: Dennis Nguyen <[email protected]>
zachaller · Nov 1, 2023 · 04e1119 · 04e1119
1 parent e1b6d05
commit 04e1119
Show file tree

Hide file tree

Showing 3 changed files with 128 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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)