From 8d9cffd9277972c73c65ca4866ddee8f011391c3 Mon Sep 17 00:00:00 2001 From: Darshan Hassan Shashikumar <62122269+darshanhs09@users.noreply.github.com> Date: Fri, 27 Aug 2021 14:21:18 -0700 Subject: [PATCH] feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts. Addressing few minor comments and lint issues. fixes #1100 --- manifests/crds/rollout-crd.yaml | 12 ++++++++++++ manifests/install.yaml | 12 ++++++++++++ manifests/namespace-install.yaml | 12 ++++++++++++ .../validation/validation_references.go | 5 +++-- .../validation/validation_references_test.go | 18 ++++++++++++++++-- rollout/controller.go | 1 - .../trafficrouting/istio/controller_test.go | 8 -------- rollout/trafficrouting/istio/istio.go | 2 +- rollout/trafficrouting/istio/istio_test.go | 2 +- 9 files changed, 57 insertions(+), 15 deletions(-) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index cff1cb3d90..9b922a8713 100644 --- 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 index fa7873ce18..128e361f82 100644 --- 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 index 3faa69c9ad..7396f9c446 100644 --- 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 index 48be7f3360..7c7e73c5e6 100644 --- 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 index 07add3e3be..079b52c3aa 100644 --- 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 index cafa60c012..4602946e12 100644 --- 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 index ec09513bc6..7b50d18e31 100644 --- 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 index c049fb56df..7a758b9c25 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -753,4 +753,4 @@ func validateDestinationRule(dRule *v1alpha1.IstioDestinationRule, hasCanarySubs } } return nil -} \ No newline at end of file +} diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go index e16d9f00aa..ac1c7bc6eb 100644 --- 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) } -} \ No newline at end of file +}