feat: Multiple VirtualService objects support for canary strategy of …

…Argo Rollouts. Addressed all the review comments fixes #1100
argoproj · Aug 3, 2021 · 9f0125f · 9f0125f
1 parent ec64953
commit 9f0125f
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
diff --git 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,41 +144,31 @@ 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)
 		fldPath := field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio")
 		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
@@ -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
@@ -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