feat: Multiple VirtualService objects support for canary strategy of …

…Argo Rollouts. Addressed few minor review comments fixes #1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>
argoproj · Aug 5, 2021 · 58d3595 · 58d3595
1 parent b355ffb
commit 58d3595
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 65 deletions.
diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go
@@ -213,6 +213,30 @@ func ValidateIngress(rollout *v1alpha1.Rollout, ingress v1beta1.Ingress) field.E
 	return allErrs
 }
 
+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"
+
+	if r.Spec.Strategy.Canary != nil {
+		canary := r.Spec.Strategy.Canary
+		if canary.TrafficRouting != nil && canary.TrafficRouting.Istio != nil {
+			if istioutil.MultipleVirtualServiceConfigured(r) {
+				if r.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name != "" {
+					return field.InternalError(fldPath, fmt.Errorf(errorString))
+				}
+			} else {
+				if r.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name == "" {
+					return field.InternalError(fldPath, fmt.Errorf(errorString))
+				}
+			}
+		}
+	}
+	return nil
+}
+
 func ValidateVirtualService(rollout *v1alpha1.Rollout, obj unstructured.Unstructured) field.ErrorList {
 	var fldPath *field.Path
 	var virtualServices []v1alpha1.IstioVirtualService

diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go
@@ -420,6 +420,44 @@ func TestValidateVirtualServices(t *testing.T) {
 	})
 }
 
+func TestValidateMultipleVirtualServicesInvalidConfig(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{},
+	}
+
+	// Test when both virtualService and  virtualServices are not configured
+	t.Run("validate No 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",
+			},
+			VirtualServices: []v1alpha1.IstioVirtualService{{Name: "istio-vsvc1-name", Routes: nil}},
+		},
+	}
+
+	// Test when both virtualService and  virtualServices are  configured
+	t.Run("get referenced virtualService - 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())
+	})
+}
+
 func TestGetAnalysisTemplateWithTypeFieldPath(t *testing.T) {
 	t.Run("get fieldPath for analysisTemplateType PrePromotionAnalysis", func(t *testing.T) {
 		fldPath := GetAnalysisTemplateWithTypeFieldPath(PrePromotionAnalysis, 0)

diff --git a/rollout/controller.go b/rollout/controller.go
@@ -471,6 +471,7 @@ func (c *rolloutContext) getRolloutValidationErrors() error {
 		return rolloutValidationErrors[0]
 	}
 
+
 	refResources, err := c.getRolloutReferencedResources()
 	if err != nil {
 		return err
@@ -526,6 +527,12 @@ func (c *rolloutContext) getRolloutReferencedResources() (*validation.Referenced
 	}
 	refResources.Ingresses = *ingresses
 
+	// Validate Rollout virtualServices before referencing
+	err = validation.ValidateRolloutVirtualServicesConfig(c.rollout)
+	if err != nil {
+		return nil, err
+	}
+
 	virtualServices, err := c.IstioController.GetReferencedVirtualServices(c.rollout)
 	if err != nil {
 		return nil, err

diff --git a/rollout/trafficrouting/istio/controller.go b/rollout/trafficrouting/istio/controller.go
@@ -174,22 +174,6 @@ func (c *IstioController) EnqueueRolloutFromIstioVirtualService(vsvc interface{}
 	}
 }
 
-func (c *IstioController) validateRolloutVirtualServicesConfig(r *v1alpha1.Rollout) error {
-	//Either VirtualService or VirtualServices must be configured.
-	//If both configured then it is an invalid configuration
-	errorString := "either VirtualService or VirtualServices must be configured"
-	if istioutil.MultipleVirtualServiceConfigured(r) {
-		if r.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name != "" {
-			return fmt.Errorf(errorString)
-		}
-	} else {
-		if r.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name == "" {
-			return fmt.Errorf(errorString)
-		}
-	}
-	return nil
-}
-
 func (c *IstioController) GetReferencedVirtualServices(ro *v1alpha1.Rollout) (*[]unstructured.Unstructured, error) {
 	var fldPath *field.Path
 	fldPath = field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio")
@@ -202,11 +186,6 @@ func (c *IstioController) GetReferencedVirtualServices(ro *v1alpha1.Rollout) (*[
 			var err error
 			var vsvcs []v1alpha1.IstioVirtualService
 
-			err = c.validateRolloutVirtualServicesConfig(ro)
-			if err != nil {
-				return nil, field.InternalError(fldPath, err)
-			}
-
 			if istioutil.MultipleVirtualServiceConfigured(ro) {
 				vsvcs = canary.TrafficRouting.Istio.VirtualServices
 				fldPath = field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio", "virtualServices", "name")

diff --git a/rollout/trafficrouting/istio/controller_test.go b/rollout/trafficrouting/istio/controller_test.go
@@ -2,7 +2,6 @@ package istio
 
 import (
 	"context"
-	"fmt"
 	"testing"
 	"time"
 
@@ -135,49 +134,6 @@ func TestGetReferencedMultipleVirtualServices(t *testing.T) {
 	})
 }
 
-func TestGetReferencedMultipleVirtualServicesInvalidConfig(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{},
-	}
-	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())
-	})
-
-	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}},
-		},
-	}
-
-	// 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)
-		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 TestSyncDestinationRule(t *testing.T) {
 	ro := &v1alpha1.Rollout{
 		ObjectMeta: metav1.ObjectMeta{