diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index 5afa45988f..48be7f3360 100644 --- 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 index a196c34355..07add3e3be 100644 --- 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 index 7e85b00937..c1bf930ee3 100644 --- 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 index 2f84493010..d64fca7b70 100644 --- 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 index 9b7749e530..ec09513bc6 100644 --- 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{