diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index 545b87e7af..8e3578675c 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -273,6 +273,15 @@ func ValidateVirtualService(rollout *v1alpha1.Rollout, obj unstructured.Unstruct allErrs := field.ErrorList{} newObj := obj.DeepCopy() + if rollout.Spec.Strategy.Canary == nil || + rollout.Spec.Strategy.Canary.TrafficRouting == nil || + rollout.Spec.Strategy.Canary.TrafficRouting.Istio == nil { + + msg := "Rollout object is not configured with Istio traffic routing" + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio"), rollout.Name, msg)) + return allErrs + } + if istioutil.MultipleVirtualServiceConfigured(rollout) { fldPath = field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio", "virtualServices", "name") virtualServices = rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualServices diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go index ac8e962cf2..75e7a59c92 100644 --- a/pkg/apis/rollouts/validation/validation_references_test.go +++ b/pkg/apis/rollouts/validation/validation_references_test.go @@ -558,6 +558,18 @@ func TestValidateVirtualService(t *testing.T) { }, } + roWithoutIstio := &v1alpha1.Rollout{ + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + StableService: "stable", + CanaryService: "canary", + TrafficRouting: &v1alpha1.RolloutTrafficRouting{}, + }, + }, + }, + } + t.Run("validate virtualService HTTP routes - success", func(t *testing.T) { vsvc := unstructured.StrToUnstructuredUnsafe(successCaseVsvc) allErrs := ValidateVirtualService(ro, *vsvc) @@ -624,13 +636,21 @@ func TestValidateVirtualService(t *testing.T) { assert.Equal(t, expectedErr.Error(), allErrs[0].Error()) }) - t.Run("validate virtualService invalid tcp routes - failure", func(t *testing.T) { + t.Run("validate virtualService invalid rollout missing istio", func(t *testing.T) { vsvc := unstructured.StrToUnstructuredUnsafe(failCaseInvalidTcpRoutesVsvc) allErrs := ValidateVirtualService(ro, *vsvc) assert.Len(t, allErrs, 1) expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio", "virtualService", "name"), "istio-vsvc", "Unable to get TCP routes for Istio VirtualService") assert.Equal(t, expectedErr.Error(), allErrs[0].Error()) }) + + t.Run("validate virtualService invalid tcp routes - failure", func(t *testing.T) { + vsvc := unstructured.StrToUnstructuredUnsafe(successCaseTcpVsvc) + allErrs := ValidateVirtualService(roWithoutIstio, *vsvc) + assert.Len(t, allErrs, 1) + expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "istio"), roWithoutIstio.Name, "Rollout object is not configured with Istio traffic routing") + assert.Equal(t, expectedErr.Error(), allErrs[0].Error()) + }) } func TestValidateVirtualServices(t *testing.T) { diff --git a/pkg/kubectl-argo-rollouts/cmd/lint/lint.go b/pkg/kubectl-argo-rollouts/cmd/lint/lint.go index 66e28318eb..c7cb091a0a 100644 --- a/pkg/kubectl-argo-rollouts/cmd/lint/lint.go +++ b/pkg/kubectl-argo-rollouts/cmd/lint/lint.go @@ -294,6 +294,9 @@ func setIngressManagedAnnotation(rollouts []v1alpha1.Rollout, refResource valida func setVirtualServiceManagedAnnotation(ro []v1alpha1.Rollout, refResource validation.ReferencedResources) { for _, rollout := range ro { for i := range refResource.VirtualServices { + if rollout.Spec.Strategy.Canary == nil || rollout.Spec.Strategy.Canary.TrafficRouting == nil || rollout.Spec.Strategy.Canary.TrafficRouting.Istio == nil { + return + } if rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService != nil && rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name == refResource.VirtualServices[i].GetName() { annotations := refResource.VirtualServices[i].GetAnnotations() if annotations == nil { diff --git a/pkg/kubectl-argo-rollouts/cmd/lint/lint_test.go b/pkg/kubectl-argo-rollouts/cmd/lint/lint_test.go index 326015bbb3..e9b66fd2ed 100644 --- a/pkg/kubectl-argo-rollouts/cmd/lint/lint_test.go +++ b/pkg/kubectl-argo-rollouts/cmd/lint/lint_test.go @@ -29,6 +29,7 @@ func TestLintValidRollout(t *testing.T) { "testdata/valid-nginx-canary.yml", "testdata/valid-nginx-basic-canary.yml", "testdata/valid-istio-v1beta1-mulitiple-virtualsvcs.yml", + "testdata/valid-nginx-smi-with-vsvc.yaml", } for _, filename := range tests { @@ -54,6 +55,10 @@ func TestLintInvalidRollout(t *testing.T) { "testdata/invalid.yml", "Error: spec.strategy.maxSurge: Invalid value: intstr.IntOrString{Type:0, IntVal:0, StrVal:\"\"}: MaxSurge and MaxUnavailable both can not be zero\n", }, + { + "testdata/invalid-empty-rollout-vsvc.yml", + "Error: spec.selector: Required value: Rollout has missing field '.spec.selector'\n", + }, { "testdata/invalid.json", "Error: spec.strategy.maxSurge: Invalid value: intstr.IntOrString{Type:0, IntVal:0, StrVal:\"\"}: MaxSurge and MaxUnavailable both can not be zero\n", diff --git a/pkg/kubectl-argo-rollouts/cmd/lint/testdata/invalid-empty-rollout-vsvc.yml b/pkg/kubectl-argo-rollouts/cmd/lint/testdata/invalid-empty-rollout-vsvc.yml new file mode 100644 index 0000000000..bf184253b9 --- /dev/null +++ b/pkg/kubectl-argo-rollouts/cmd/lint/testdata/invalid-empty-rollout-vsvc.yml @@ -0,0 +1,5 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +--- +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService \ No newline at end of file diff --git a/pkg/kubectl-argo-rollouts/cmd/lint/testdata/valid-nginx-smi-with-vsvc.yaml b/pkg/kubectl-argo-rollouts/cmd/lint/testdata/valid-nginx-smi-with-vsvc.yaml new file mode 100644 index 0000000000..0acb23d62f --- /dev/null +++ b/pkg/kubectl-argo-rollouts/cmd/lint/testdata/valid-nginx-smi-with-vsvc.yaml @@ -0,0 +1,74 @@ +apiVersion: networking.istio.io/v1beta1 +kind: VirtualService +metadata: + name: istio-host-split-vsvc +spec: + hosts: + - istio-host-split.com + gateways: + - istio-host-split-gateway + http: + - name: primary + route: + - destination: + host: istio-host-split-stable + weight: 100 + - destination: + host: istio-host-split-canary + weight: 0 +--- +apiVersion: networking.istio.io/v1beta1 +kind: VirtualService +metadata: + name: istio-host-split-vsvc-1 +spec: + hosts: + - istio-host-split.com + gateways: + - istio-host-split-gateway + http: + - name: primary + route: + - destination: + host: istio-host-split-stable + weight: 100 + - destination: + host: istio-host-split-canary + weight: 0 +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: nginx-rollout +spec: + selector: + matchLabels: + app: nginx-rollout + template: + metadata: + labels: + app: nginx-rollout + spec: + containers: + - name: nginx-rollout + image: argoproj/rollouts-demo:blue + ports: + - name: http + containerPort: 80 + protocol: TCP + resources: + requests: + memory: 16Mi + cpu: 5m + strategy: + canary: + canaryService: nginx-rollout-canary + stableService: nginx-rollout-stable + trafficRouting: + smi: + trafficSplitName: rollout-smi-experiment-split + rootService: rollout-smi-experiment-root + nginx: + stableIngress: nginx-rollout-ingress + steps: + - setWeight: 10