fix(cli): nil pointer while linting (argoproj#2324)

* fix nil pointer on lint Signed-off-by: zachaller <[email protected]> * github trigger re-run Signed-off-by: zachaller <[email protected]> * add test for having a virtual service but not using it in rollout Signed-off-by: zachaller <[email protected]> * change logic Signed-off-by: zachaller <[email protected]> * improve test cov Signed-off-by: zachaller <[email protected]> * remove unreachable code Signed-off-by: zachaller <[email protected]> * github trigger re-run Signed-off-by: zachaller <[email protected]> * github trigger re-run Signed-off-by: zachaller <[email protected]> * Add test coverage Signed-off-by: zachaller <[email protected]> * add more test coverage Signed-off-by: zachaller <[email protected]> * Standardize msg Signed-off-by: zachaller <[email protected]> * lint Signed-off-by: zachaller <[email protected]> Signed-off-by: zachaller <[email protected]>
jenciso · Oct 25, 2022 · 1132371 · 1132371
1 parent 5660a10
commit 1132371
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,5 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: VirtualService
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
@@ -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