feat: Multiple VirtualService objects support for canary strategy of …

…Argo Rollouts. Addressing few minor comments and lint issues. fixes #1100
argoproj · Aug 27, 2021 · 8d9cffd · 8d9cffd
1 parent 5cdb12e
commit 8d9cffd
Show file tree

Hide file tree

Showing 9 changed files with 57 additions and 15 deletions.
diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml
@@ -567,6 +567,18 @@ spec:
                                       items:
                                         type: string
                                       type: array
+                                    tlsRoutes:
+                                      items:
+                                        properties:
+                                          port:
+                                            format: int64
+                                            type: integer
+                                          sniHosts:
+                                            items:
+                                              type: string
+                                            type: array
+                                        type: object
+                                      type: array
                                   required:
                                   - name
                                   type: object

diff --git a/manifests/install.yaml b/manifests/install.yaml
@@ -10265,6 +10265,18 @@ spec:
                                       items:
                                         type: string
                                       type: array
+                                    tlsRoutes:
+                                      items:
+                                        properties:
+                                          port:
+                                            format: int64
+                                            type: integer
+                                          sniHosts:
+                                            items:
+                                              type: string
+                                            type: array
+                                        type: object
+                                      type: array
                                   required:
                                   - name
                                   type: object

diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml
@@ -10265,6 +10265,18 @@ spec:
                                       items:
                                         type: string
                                       type: array
+                                    tlsRoutes:
+                                      items:
+                                        properties:
+                                          port:
+                                            format: int64
+                                            type: integer
+                                          sniHosts:
+                                            items:
+                                              type: string
+                                            type: array
+                                        type: object
+                                      type: array
                                   required:
                                   - name
                                   type: object

diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go
@@ -213,9 +213,10 @@ func ValidateIngress(rollout *v1alpha1.Rollout, ingress v1beta1.Ingress) field.E
 	return allErrs
 }
 
+// ValidateRolloutVirtualServicesConfig checks either VirtualService or VirtualServices configured
+// It returns an error if both VirtualService and VirtualServices are configured.
+// Also, returns an error if both are not configured.
 func ValidateRolloutVirtualServicesConfig(r *v1alpha1.Rollout) error {
-	//Either VirtualService or VirtualServices must be configured.
-	//If both configured then it is an invalid configuration
 	var fldPath *field.Path
 	fldPath = field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio")
 	errorString := "either VirtualService or VirtualServices must be configured"

diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go
@@ -420,7 +420,7 @@ func TestValidateVirtualServices(t *testing.T) {
 	})
 }
 
-func TestValidateMultipleVirtualServicesInvalidConfig(t *testing.T) {
+func TestValidateRolloutVirtualServicesConfig(t *testing.T) {
 	ro := v1alpha1.Rollout{
 		Spec: v1alpha1.RolloutSpec{
 			Strategy: v1alpha1.RolloutStrategy{
@@ -450,12 +450,26 @@ func TestValidateMultipleVirtualServicesInvalidConfig(t *testing.T) {
 	}
 
 	// Test when both virtualService and  virtualServices are  configured
-	t.Run("get referenced virtualService - fail", func(t *testing.T) {
+	t.Run("validate both virtualService configured - fail", func(t *testing.T) {
 		err := ValidateRolloutVirtualServicesConfig(&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())
 	})
+
+	ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
+		Istio: &v1alpha1.IstioTrafficRouting{
+			VirtualService: v1alpha1.IstioVirtualService{
+				Name: "istio-vsvc1-name",
+			},
+		},
+	}
+
+	// Successful case where either virtualService or  virtualServices configured
+	t.Run("validate either virtualService or  virtualServices configured - pass", func(t *testing.T) {
+		err := ValidateRolloutVirtualServicesConfig(&ro)
+		assert.Empty(t, err)
+	})
 }
 
 func TestGetAnalysisTemplateWithTypeFieldPath(t *testing.T) {

diff --git a/rollout/controller.go b/rollout/controller.go
@@ -472,7 +472,6 @@ func (c *rolloutContext) getRolloutValidationErrors() error {
 		return rolloutValidationErrors[0]
 	}
 
-
 	refResources, err := c.getRolloutReferencedResources()
 	if err != nil {
 		return err

diff --git a/rollout/trafficrouting/istio/controller_test.go b/rollout/trafficrouting/istio/controller_test.go
@@ -90,14 +90,6 @@ spec:
   - istio-rollout.dev.argoproj.io
   http:
   - name: primary
-    route:
-    - destination:
-        host: 'stable'
-      weight: 100
-    - destination:
-        host: canary
-      weight: 0
-  - name: secondary
     route:
     - destination:
         host: 'stable'

diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go
@@ -753,4 +753,4 @@ func validateDestinationRule(dRule *v1alpha1.IstioDestinationRule, hasCanarySubs
 		}
 	}
 	return nil
-}
+}
diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go
@@ -1492,4 +1492,4 @@ func TestMultipleVirtualServiceReconcileInferredSingleRoute(t *testing.T) {
 		// Assertions
 		assertHttpRouteWeightChanges(t, httpRoutes[0], "", 10, 90)
 	}
-}
+}
-Original file line number
+Diff line change
@@ Expand Up @@
     		}
     	}
     	return nil
-    }
+    }