From 9f0125fc660b7fb6e84d47167816f86d8309b370 Mon Sep 17 00:00:00 2001 From: Darshan Hassan Shashikumar Date: Tue, 3 Aug 2021 13:36:51 -0700 Subject: [PATCH] feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts. Addressed all the review comments fixes #1100 --- docs/features/specification.md | 9 ++++++ docs/getting-started/istio/index.md | 18 ++++++++++++ .../istio/multipleVirtualsvc.yaml | 28 +++++++++++------- .../trafficrouting/istio/controller_test.go | 29 +++++-------------- rollout/trafficrouting/istio/istio.go | 4 +-- utils/istio/istio.go | 5 +--- 6 files changed, 56 insertions(+), 37 deletions(-) diff --git a/docs/features/specification.md b/docs/features/specification.md index 60acd6cb21..cf9d47d368 100644 --- a/docs/features/specification.md +++ b/docs/features/specification.md @@ -275,10 +275,19 @@ spec: # Istio traffic routing configuration istio: + # Either virtualService or virtualServices can be configured. virtualService: name: rollout-vsvc # required routes: - primary # optional if there is a single route in VirtualService, required otherwise + virtualServices: + # One or more virtualServices can be configured + - name: rollouts-vsvc1 # required + routes: + - primary # optional if there is a single route in VirtualService, required otherwise + - name: rollouts-vsvc2 # required + routes: + - secondary # optional if there is a single route in VirtualService, required otherwise # NGINX Ingress Controller routing configuration nginx: diff --git a/docs/getting-started/istio/index.md b/docs/getting-started/istio/index.md index 79e0cb2831..143552cc92 100644 --- a/docs/getting-started/istio/index.md +++ b/docs/getting-started/istio/index.md @@ -47,6 +47,8 @@ The VirtualService and route referenced in either `trafficRouting.istio.virtualS `trafficRouting.istio.virtualServices`. `trafficRouting.istio.virtualServices` helps in adding one or more virtualServices unlike `trafficRouting.istio.virtualService` where only single virtualService can be added. This is required to have a HTTP route which splits between the stable and canary Services, referenced in the rollout. +In this example, primary route splits the http route with port number 15372 between stable and canary services; +secondary route splits the http route with port number 15373 between stable and canary services. In this guide, those Services are named: `rollouts-demo-stable` and `rollouts-demo-canary` respectively. The weight values for these services used should be initially set to 100% stable, and 0% on the canary. During an update, these values will be modified by the controller. @@ -68,9 +70,13 @@ spec: route: - destination: host: rollouts-demo-stable # Should match spec.strategy.canary.stableService + port: + number: 15372 weight: 100 - destination: host: rollouts-demo-canary # Should match spec.strategy.canary.canaryService + port: + number: 15372 weight: 0 ``` @@ -90,9 +96,13 @@ spec: route: - destination: host: rollouts-demo-stable # Should match spec.strategy.canary.stableService + port: + number: 15373 weight: 100 - destination: host: rollouts-demo-canary # Should match spec.strategy.canary.canaryService + port: + number: 15373 weight: 0 ``` @@ -173,9 +183,13 @@ spec: route: - destination: host: rollouts-demo-stable + port: + number: 15372 weight: 95 - destination: host: rollouts-demo-canary + port: + number: 15372 weight: 5 ``` @@ -195,9 +209,13 @@ spec: route: - destination: host: rollouts-demo-stable + port: + number: 15373 weight: 95 - destination: host: rollouts-demo-canary + port: + number: 15373 weight: 5 ``` diff --git a/docs/getting-started/istio/multipleVirtualsvc.yaml b/docs/getting-started/istio/multipleVirtualsvc.yaml index 1b59774d11..567e9d8a18 100644 --- a/docs/getting-started/istio/multipleVirtualsvc.yaml +++ b/docs/getting-started/istio/multipleVirtualsvc.yaml @@ -12,9 +12,13 @@ spec: route: - destination: host: rollouts-demo-stable + port: + number: 15372 weight: 100 - destination: host: rollouts-demo-canary + port: + number: 15372 weight: 0 --- @@ -24,15 +28,19 @@ metadata: name: rollouts-demo-vsvc2 spec: gateways: - - rollouts-demo-gateway + - rollouts-demo-gateway hosts: - - rollouts-demo-vsvc2.local + - rollouts-demo-vsvc2.local http: - - name: primary - route: - - destination: - host: rollouts-demo-stable - weight: 100 - - destination: - host: rollouts-demo-canary - weight: 0 + - name: secondary + route: + - destination: + host: rollouts-demo-stable + port: + number: 15373 + weight: 100 + - destination: + host: rollouts-demo-canary + port: + number: 15373 + weight: 0 \ No newline at end of file diff --git a/rollout/trafficrouting/istio/controller_test.go b/rollout/trafficrouting/istio/controller_test.go index ed8121e45a..9b7749e530 100644 --- a/rollout/trafficrouting/istio/controller_test.go +++ b/rollout/trafficrouting/istio/controller_test.go @@ -136,9 +136,6 @@ func TestGetReferencedMultipleVirtualServices(t *testing.T) { } func TestGetReferencedMultipleVirtualServicesInvalidConfig(t *testing.T) { - - multipleVirtualService := []v1alpha1.IstioVirtualService{{Name: "istio-vsvc1-name", Routes: nil}} - ro := v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ Strategy: v1alpha1.RolloutStrategy{ @@ -147,17 +144,13 @@ func TestGetReferencedMultipleVirtualServicesInvalidConfig(t *testing.T) { }, } ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ - Istio: &v1alpha1.IstioTrafficRouting{ - VirtualService: v1alpha1.IstioVirtualService{ - Name: "istio-vsvc1-name", - }, - VirtualServices: multipleVirtualService, - }, + Istio: &v1alpha1.IstioTrafficRouting{}, } ro.Namespace = metav1.NamespaceDefault vService := unstructuredutil.StrToUnstructuredUnsafe(istiovsvc1) + // Test when both virtualService and virtualServices are not configured t.Run("get referenced virtualService - fail", func(t *testing.T) { c := NewFakeIstioController(vService) _, err := c.GetReferencedVirtualServices(&ro) @@ -165,23 +158,17 @@ func TestGetReferencedMultipleVirtualServicesInvalidConfig(t *testing.T) { expected := fmt.Sprintf("%s: Internal error: either VirtualService or VirtualServices must be configured", fldPath) assert.Equal(t, expected, err.Error()) }) -} -func TestGetReferencedNoVirtualServicesConfig(t *testing.T) { - ro := v1alpha1.Rollout{ - Spec: v1alpha1.RolloutSpec{ - Strategy: v1alpha1.RolloutStrategy{ - Canary: &v1alpha1.CanaryStrategy{}, + ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ + Istio: &v1alpha1.IstioTrafficRouting{ + VirtualService: v1alpha1.IstioVirtualService{ + Name: "istio-vsvc1-name", }, + VirtualServices: []v1alpha1.IstioVirtualService{{Name: "istio-vsvc1-name", Routes: nil}}, }, } - ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ - Istio: &v1alpha1.IstioTrafficRouting{}, - } - ro.Namespace = metav1.NamespaceDefault - - vService := unstructuredutil.StrToUnstructuredUnsafe(istiovsvc1) + // Test when both virtualService and virtualServices are configured t.Run("get referenced virtualService - fail", func(t *testing.T) { c := NewFakeIstioController(vService) _, err := c.GetReferencedVirtualServices(&ro) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index 2f09f7d624..2e0a9dc7fa 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -472,8 +472,8 @@ func (r *Reconciler) SetWeight(desiredWeight int32) error { } _, err = client.Update(ctx, modifiedVirtualService, metav1.UpdateOptions{}) if err == nil { - r.log.Debugf("UpdatedVirtualService: %s", modifiedVirtualService) - r.recorder.Eventf(r.rollout, record.EventOptions{EventReason: "UpdatedVirtualService"}, "VirtualService `%s` set to desiredWeight '%d'", vsvcName, desiredWeight) + r.log.Debugf("Updated VirtualService: %s", modifiedVirtualService) + r.recorder.Eventf(r.rollout, record.EventOptions{EventReason: "Updated VirtualService"}, "VirtualService `%s` set to desiredWeight '%d'", vsvcName, desiredWeight) } } return err diff --git a/utils/istio/istio.go b/utils/istio/istio.go index be09ee8d15..252197ba9c 100644 --- a/utils/istio/istio.go +++ b/utils/istio/istio.go @@ -65,10 +65,7 @@ func GetVirtualServiceNamespaceName(vsv string) (string, string) { } func MultipleVirtualServiceConfigured(rollout *v1alpha1.Rollout) bool { - if rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualServices != nil { - return true - } - return false + return rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualServices != nil } // GetRolloutVirtualServiceKeys gets the referenced VirtualService and its namespace from a Rollout