fix: workload rollout spec is invalid template is not empty (argoproj#1224)

Signed-off-by: Hui Kang <[email protected]>

Author: huikang

pkg/apis/rollouts/v1alpha1/types.go

@@ -83,6 +83,16 @@ func (s *RolloutSpec) SetResolvedTemplate(template corev1.PodTemplateSpec) {
 	s.Template = template
 }
 
+func (s *RolloutSpec) EmptyTemplate() bool {
+	if len(s.Template.Labels) > 0 {
+		return false
+	}
+	if len(s.Template.Annotations) > 0 {
+		return false
+	}
+	return true
+}
+
 func (s *RolloutSpec) MarshalJSON() ([]byte, error) {
 	type Alias RolloutSpec
 

pkg/apis/rollouts/validation/validation.go

@@ -83,6 +83,13 @@ func ValidateRolloutSpec(rollout *v1alpha1.Rollout, fldPath *field.Path) field.E
 		}
 	}
 
+	if !rollout.Spec.TemplateResolvedFromRef && (spec.WorkloadRef != nil && !spec.EmptyTemplate()) {
+		// WorkloadRef and template can not be set at the same time for lint plugin
+		// During reconciliation, TemplateResolvedFromRef is true and will not reach here
+		allErrs = append(allErrs, field.InternalError(fldPath.Child("template"),
+			fmt.Errorf("template must be empty for workload reference rollout")))
+	}
+
 	selector, err := metav1.LabelSelectorAsSelector(spec.Selector)
 	if err != nil {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "invalid label selector"))

pkg/apis/rollouts/validation/validation_test.go

@@ -312,3 +312,37 @@ func TestCanaryScaleDownDelaySeconds(t *testing.T) {
 	})
 
 }
+
+func TestWorkloadRefWithTemplate(t *testing.T) {
+	selector := &metav1.LabelSelector{
+		MatchLabels: map[string]string{"key": "value"},
+	}
+	ro := &v1alpha1.Rollout{
+		Spec: v1alpha1.RolloutSpec{
+			WorkloadRef: &v1alpha1.ObjectRef{
+				Name:       "my-deployment",
+				Kind:       "Deployment",
+				APIVersion: "apps/v1",
+			},
+			Selector: selector,
+			Strategy: v1alpha1.RolloutStrategy{
+				Canary: &v1alpha1.CanaryStrategy{
+					StableService: "stable",
+					CanaryService: "canary",
+				},
+			},
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: selector.MatchLabels,
+				},
+			},
+		},
+	}
+	t.Run("workload reference with template", func(t *testing.T) {
+		ro := ro.DeepCopy()
+		allErrs := ValidateRollout(ro)
+		assert.Equal(t, 2, len(allErrs))
+		assert.EqualError(t, allErrs[0], "spec.template: Internal error: template must be empty for workload reference rollout")
+		assert.EqualError(t, allErrs[1], "spec.template.spec.containers: Required value")
+	})
+}

rollout/temlateref.go

@@ -125,6 +125,12 @@ func (r *informerBasedTemplateResolver) Resolve(rollout *v1alpha1.Rollout) error
 		return nil
 	}
 
+	// When workloadRef is resolved for the first time, TemplateResolvedFromRef = false.
+	// In this case, template must not be set
+	if !rollout.Spec.TemplateResolvedFromRef && !rollout.Spec.EmptyTemplate() {
+		return fmt.Errorf("template must be empty for workload reference rollout")
+	}
+
 	gvk := schema.FromAPIVersionAndKind(rollout.Spec.WorkloadRef.APIVersion, rollout.Spec.WorkloadRef.Kind)
 
 	info, ok := infoByGroupKind[gvk.GroupKind()]

rollout/temlateref_test.go

@@ -406,3 +406,52 @@ func TestRemashalMapFails(t *testing.T) {
 	err := remarshalMap(nil, struct{}{})
 	assert.Error(t, err)
 }
+
+func TestResolve_WorkloadWithTemplate(t *testing.T) {
+	rollout := v1alpha1.Rollout{
+		ObjectMeta: v1.ObjectMeta{
+			Namespace: "default",
+		},
+		Spec: v1alpha1.RolloutSpec{
+			WorkloadRef: &v1alpha1.ObjectRef{
+				Name:       "my-deployment",
+				Kind:       "Deployment",
+				APIVersion: "apps/v1",
+			},
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"app": "deploy",
+					},
+				},
+			},
+		},
+	}
+
+	deployment := &appsv1.Deployment{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: appsv1.SchemeGroupVersion.String(),
+			Kind:       "Deployment",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-deployment",
+			Namespace: "default",
+		},
+		Spec: appsv1.DeploymentSpec{
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"test-label": "test-label-val"}},
+			},
+		},
+	}
+
+	discoveryClient := newFakeDiscoClient()
+	dynamicClient := dynamicfake.NewSimpleDynamicClient(scheme.Scheme, deployment)
+
+	resolver, cancel := newResolver(dynamicClient, discoveryClient, fake.NewSimpleClientset())
+	defer cancel()
+
+	err := resolver.Resolve(&rollout)
+
+	assert.Error(t, err)
+	assert.Equal(t, "template must be empty for workload reference rollout", err.Error())
+}

test/e2e/functional_test.go

@@ -1063,6 +1063,60 @@ spec:
 		ExpectActiveRevision("2")
 }
 
+func (s *FunctionalSuite) TestWorkloadRefTemplate() {
+	s.Given().
+		RolloutObjects(`
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app.kubernetes.io/instance: rollout-canary
+  name: rollout-ref-deployment
+spec:
+  replicas: 0
+  selector:
+    matchLabels:
+      app: rollout-ref-deployment
+  template:
+    metadata:
+      labels:
+        app: rollout-ref-deployment
+    spec:
+      containers:
+        - name: rollouts-demo
+          image: argoproj/rollouts-demo:green
+---
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: rollout-ref-deployment
+spec:
+  replicas: 1
+  workloadRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: rollout-ref-deployment
+  selector:
+    matchLabels:
+      app: rollout-demo-deploy
+  template:
+    metadata:
+      labels:
+        app: rollout-ref-deployment
+  strategy:
+    blueGreen: 
+      activeService: rollout-bluegreen-active
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Degraded").
+		Then().
+		ExpectRollout("error due to workload ref and template", func(r *v1alpha1.Rollout) bool {
+			return len(r.Status.Conditions) == 1 && strings.Contains(r.Status.Conditions[0].Message, "template must be empty for workload reference rollout")
+		})
+}
+
 func (s *FunctionalSuite) TestWorkloadRef() {
 	s.Given().
 		RolloutObjects(`