fix: background analysis refs were not verified. requeue InvalidSpec rollouts

pkg/apis/rollouts/validation/validation.go

@@ -224,14 +224,12 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat
 			}
 		}
 
-		if analysisRunArgs != nil {
-			for _, arg := range analysisRunArgs {
-				if arg.ValueFrom != nil {
-					if arg.ValueFrom.FieldRef != nil {
-						_, err := fieldpath.ExtractFieldPathAsString(rollout, arg.ValueFrom.FieldRef.FieldPath)
-						if err != nil {
-							allErrs = append(allErrs, field.Invalid(stepFldPath.Child("analyses"), analysisRunArgs, InvalidAnalysisArgsMessage))
-						}
+		for _, arg := range analysisRunArgs {
+			if arg.ValueFrom != nil {
+				if arg.ValueFrom.FieldRef != nil {
+					_, err := fieldpath.ExtractFieldPathAsString(rollout, arg.ValueFrom.FieldRef.FieldPath)
+					if err != nil {
+						allErrs = append(allErrs, field.Invalid(stepFldPath.Child("analyses"), analysisRunArgs, InvalidAnalysisArgsMessage))
 					}
 				}
 			}

pkg/apis/rollouts/validation/validation_references.go

@@ -24,15 +24,16 @@ type AnalysisTemplateType string
 const (
 	PrePromotionAnalysis  AnalysisTemplateType = "PrePromotionAnalysis"
 	PostPromotionAnalysis AnalysisTemplateType = "PostPromotionAnalysis"
-	CanaryStep            AnalysisTemplateType = "CanaryStep"
+	InlineAnalysis        AnalysisTemplateType = "InlineAnalysis"
+	BackgroundAnalysis    AnalysisTemplateType = "BackgroundAnalysis"
 )
 
 type AnalysisTemplateWithType struct {
 	AnalysisTemplate        *v1alpha1.AnalysisTemplate
 	ClusterAnalysisTemplate *v1alpha1.ClusterAnalysisTemplate
 	TemplateType            AnalysisTemplateType
 	AnalysisIndex           int
-	// Used only for CanaryStep
+	// Used only for InlineAnalysis
 	CanaryStepIndex int
 }
 
@@ -57,7 +58,7 @@ type ReferencedResources struct {
 	VirtualServices          []unstructured.Unstructured
 }
 
-func ValidateRolloutReferencedResources(rollout *v1alpha1.Rollout, referencedResources ReferencedResources) field.ErrorList { //field.ErrorList {
+func ValidateRolloutReferencedResources(rollout *v1alpha1.Rollout, referencedResources ReferencedResources) field.ErrorList {
 	allErrs := field.ErrorList{}
 	for _, service := range referencedResources.ServiceWithType {
 		allErrs = append(allErrs, ValidateService(service, rollout)...)
@@ -104,11 +105,13 @@ func ValidateAnalysisTemplateWithType(template AnalysisTemplateWithType) field.E
 	} else if template.AnalysisTemplate != nil {
 		templateName, templateSpec = template.AnalysisTemplate.Name, template.AnalysisTemplate.Spec
 	}
-	for _, metric := range templateSpec.Metrics {
-		effectiveCount := metric.EffectiveCount()
-		if effectiveCount == nil {
-			msg := fmt.Sprintf("AnalysisTemplate %s has metric %s which runs indefinitely", templateName, metric.Name)
-			allErrs = append(allErrs, field.Invalid(fldPath, templateName, msg))
+	if template.TemplateType != BackgroundAnalysis {
+		for _, metric := range templateSpec.Metrics {
+			effectiveCount := metric.EffectiveCount()
+			if effectiveCount == nil {
+				msg := fmt.Sprintf("AnalysisTemplate %s has metric %s which runs indefinitely", templateName, metric.Name)
+				allErrs = append(allErrs, field.Invalid(fldPath, templateName, msg))
+			}
 		}
 	}
 	return allErrs
@@ -188,8 +191,10 @@ func GetAnalysisTemplateWithTypeFieldPath(templateType AnalysisTemplateType, ana
 		fldPath = fldPath.Child("blueGreen", "prePromotionAnalysis", "templates")
 	case PostPromotionAnalysis:
 		fldPath = fldPath.Child("blueGreen", "postPromotionAnalysis", "templates")
-	case CanaryStep:
+	case InlineAnalysis:
 		fldPath = fldPath.Child("canary", "steps").Index(canaryStepIndex).Child("analysis", "templates")
+	case BackgroundAnalysis:
+		fldPath = fldPath.Child("canary", "analysis", "templates")
 	default:
 		// No path specified
 		return nil

pkg/apis/rollouts/validation/validation_references_test.go

@@ -84,7 +84,7 @@ func getAnalysisTemplateWithType() AnalysisTemplateWithType {
 				}},
 			},
 		},
-		TemplateType:    CanaryStep,
+		TemplateType:    InlineAnalysis,
 		AnalysisIndex:   0,
 		CanaryStepIndex: 0,
 	}
@@ -160,14 +160,23 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) {
 		assert.Empty(t, allErrs)
 	})
 
-	t.Run("validate analysisTemplate - failure", func(t *testing.T) {
+	t.Run("validate inline analysisTemplate - failure", func(t *testing.T) {
 		template := getAnalysisTemplateWithType()
 		template.AnalysisTemplate.Spec.Metrics[0].Count = 0
 		allErrs := ValidateAnalysisTemplateWithType(template)
 		assert.Len(t, allErrs, 1)
 		expectedError := field.Invalid(GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.AnalysisIndex, template.CanaryStepIndex), template.AnalysisTemplate.Name, "AnalysisTemplate analysis-template-name has metric metric-name which runs indefinitely")
 		assert.Equal(t, expectedError.Error(), allErrs[0].Error())
 	})
+
+	// verify background analysis does not care about a metric that runs indefinitely
+	t.Run("validate background analysisTemplate - success", func(t *testing.T) {
+		template := getAnalysisTemplateWithType()
+		template.TemplateType = BackgroundAnalysis
+		template.AnalysisTemplate.Spec.Metrics[0].Count = 0
+		allErrs := ValidateAnalysisTemplateWithType(template)
+		assert.Empty(t, allErrs)
+	})
 }
 
 func TestValidateIngress(t *testing.T) {
@@ -256,7 +265,7 @@ func TestGetAnalysisTemplateWithTypeFieldPath(t *testing.T) {
 	})
 
 	t.Run("get fieldPath for analysisTemplateType CanaryStep", func(t *testing.T) {
-		fldPath := GetAnalysisTemplateWithTypeFieldPath(CanaryStep, 0, 0)
+		fldPath := GetAnalysisTemplateWithTypeFieldPath(InlineAnalysis, 0, 0)
 		expectedFldPath := field.NewPath("spec", "strategy", "canary", "steps").Index(0).Child("analysis", "templates").Index(0).Child("templateName")
 		assert.Equal(t, expectedFldPath.String(), fldPath.String())
 	})

rollout/analysis_test.go

@@ -1,12 +1,14 @@
 package rollout
 
 import (
+	"encoding/json"
 	"fmt"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/kubernetes/pkg/controller"
@@ -285,43 +287,37 @@ func TestCreateBackgroundAnalysisRunWithClusterTemplates(t *testing.T) {
 	assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName, expectedArName)), patch)
 }
 
-func TestCreateBackgroundAnalysisRunErrorWithMissingClusterTemplates(t *testing.T) {
+func TestInvalidSpecMissingClusterTemplatesBackgroundAnalysis(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
 
-	steps := []v1alpha1.CanaryStep{{
-		SetWeight: int32Ptr(10),
-	}}
-	cat := clusterAnalysisTemplate("bar")
-	r1 := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
-	r2 := bumpVersion(r1)
-	r2.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
+	r := newCanaryRollout("foo", 10, nil, nil, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
 		RolloutAnalysis: v1alpha1.RolloutAnalysis{
 			Templates: []v1alpha1.RolloutAnalysisTemplate{{
 				TemplateName: "missing",
 				ClusterScope: true,
 			}},
 		},
 	}
+	f.rolloutLister = append(f.rolloutLister, r)
+	f.objects = append(f.objects, r)
 
-	rs1 := newReplicaSetWithStatus(r1, 10, 10)
-	rs2 := newReplicaSetWithStatus(r2, 0, 0)
-	f.kubeobjects = append(f.kubeobjects, rs1, rs2)
-	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
-	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
-
-	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 0, 10, false)
-	progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs2, "")
-	conditions.SetRolloutCondition(&r2.Status, progressingCondition)
-	availableCondition, _ := newAvailableCondition(true)
-	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+	patchIndex := f.expectPatchRolloutAction(r)
+	f.run(getKey(r, t))
 
-	f.rolloutLister = append(f.rolloutLister, r2)
-	f.clusterAnalysisTemplateLister = append(f.clusterAnalysisTemplateLister, cat)
-	f.objects = append(f.objects, r2, cat)
+	expectedPatchWithoutSub := `{
+		"status": {
+			"conditions": [%s,%s]
+		}
+	}`
+	_, progressingCond := newProgressingCondition(conditions.ReplicaSetUpdatedReason, r, "")
+	invalidSpecCond := conditions.NewRolloutCondition(v1alpha1.InvalidSpec, corev1.ConditionTrue, conditions.InvalidSpecReason, "The Rollout \"foo\" is invalid: spec.strategy.canary.analysis.templates[0].templateName: Invalid value: \"missing\": clusteranalysistemplate.argoproj.io \"missing\" not found")
+	invalidSpecBytes, _ := json.Marshal(invalidSpecCond)
+	expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes))
 
-	c, i, k8sI := f.newController(noResyncPeriodFunc)
-	f.runController(getKey(r2, t), true, true, c, i, k8sI)
+	patch := f.getPatchedRollout(patchIndex)
+	assert.Equal(t, calculatePatch(r, expectedPatch), patch)
 }
 
 func TestCreateBackgroundAnalysisRunWithClusterTemplatesAndTemplate(t *testing.T) {
@@ -646,43 +642,6 @@ func TestFailCreateBackgroundAnalysisRunIfInvalidTemplateRef(t *testing.T) {
 	f.runExpectError(getKey(r2, t), true)
 }
 
-func TestFailCreateBackgroundAnalysisRunIfInvalidTemplateRefWithTemplates(t *testing.T) {
-	f := newFixture(t)
-	defer f.Close()
-
-	steps := []v1alpha1.CanaryStep{{
-		SetWeight: pointer.Int32Ptr(10),
-	}}
-
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
-	r2 := bumpVersion(r1)
-	r2.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
-		RolloutAnalysis: v1alpha1.RolloutAnalysis{
-			Templates: []v1alpha1.RolloutAnalysisTemplate{{
-				TemplateName: "invalid-template-ref",
-			}},
-		},
-	}
-
-	rs1 := newReplicaSetWithStatus(r1, 1, 1)
-	rs2 := newReplicaSetWithStatus(r2, 0, 0)
-	f.kubeobjects = append(f.kubeobjects, rs1, rs2)
-	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
-	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
-
-	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 1, 0, 1, false)
-
-	progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs2, "")
-	conditions.SetRolloutCondition(&r2.Status, progressingCondition)
-	availableCondition, _ := newAvailableCondition(true)
-	conditions.SetRolloutCondition(&r2.Status, availableCondition)
-
-	f.rolloutLister = append(f.rolloutLister, r2)
-	f.objects = append(f.objects, r2)
-
-	f.runExpectError(getKey(r2, t), true)
-}
-
 func TestFailCreateBackgroundAnalysisRunIfMetricRepeated(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()

rollout/context.go

@@ -1,6 +1,8 @@
 package rollout
 
 import (
+	"time"
+
 	log "github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -45,6 +47,9 @@ func (c *rolloutContext) reconcile() error {
 	err := c.getRolloutValidationErrors()
 	if err != nil {
 		if vErr, ok := err.(*field.Error); ok {
+			// We want to frequently requeue rollouts with InvalidSpec errors, because the error
+			// condition might be timing related (e.g. the Rollout was applied before the Service).
+			c.enqueueRolloutAfter(c.rollout, 20*time.Second)
 			return c.createInvalidRolloutCondition(vErr, c.rollout)
 		}
 		return err

rollout/controller.go

@@ -640,20 +640,24 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT
 			}
 			analysisTemplates = append(analysisTemplates, templates...)
 		}
-		// Don't need to check for background AnalysisRuns since RO controls and can terminate them
 	} else if c.rollout.Spec.Strategy.Canary != nil {
 		canary := c.rollout.Spec.Strategy.Canary
-		if canary.Steps != nil {
-			for i, step := range canary.Steps {
-				if step.Analysis != nil {
-					templates, err := c.getReferencedAnalysisTemplates(c.rollout, step.Analysis, validation.CanaryStep, i)
-					if err != nil {
-						return nil, err
-					}
-					analysisTemplates = append(analysisTemplates, templates...)
+		for i, step := range canary.Steps {
+			if step.Analysis != nil {
+				templates, err := c.getReferencedAnalysisTemplates(c.rollout, step.Analysis, validation.InlineAnalysis, i)
+				if err != nil {
+					return nil, err
 				}
+				analysisTemplates = append(analysisTemplates, templates...)
 			}
 		}
+		if canary.Analysis != nil {
+			templates, err := c.getReferencedAnalysisTemplates(c.rollout, &canary.Analysis.RolloutAnalysis, validation.BackgroundAnalysis, 0)
+			if err != nil {
+				return nil, err
+			}
+			analysisTemplates = append(analysisTemplates, templates...)
+		}
 	}
 	return &analysisTemplates, nil
 }

rollout/controller_test.go

@@ -356,7 +356,10 @@ func calculatePatch(ro *v1alpha1.Rollout, patch string) string {
 	newObservedGen := conditions.ComputeGenerationHash(newRO.Spec)
 
 	newPatch := make(map[string]interface{})
-	json.Unmarshal([]byte(patch), &newPatch)
+	err = json.Unmarshal([]byte(patch), &newPatch)
+	if err != nil {
+		panic(err)
+	}
 	newStatus := newPatch["status"].(map[string]interface{})
 	newStatus["observedGeneration"] = newObservedGen
 	newPatch["status"] = newStatus

test/e2e/functional_test.go

@@ -435,3 +435,59 @@ spec:
 		ExpectReplicaCounts(2, 2, 1, 2, 2). // desired, current, updated, ready, available
 		ExpectServiceSelector("bluegreen-to-canary", map[string]string{"app": "bluegreen-to-canary"})
 }
+
+// TestFixInvalidSpec verifies we recover from an InvalidSpec after applying
+func (s *FunctionalSuite) TestFixInvalidSpec() {
+	s.Given().
+		RolloutObjects(`
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: fix-invalid-spec
+spec:
+  replicas: 0
+  strategy:
+    canary:
+      analysis:
+        templates:
+        - templateName: doesnt-exist-yet
+  selector:
+    matchLabels:
+      app: fix-invalid-spec
+  template:
+    metadata:
+      labels:
+        app: fix-invalid-spec
+    spec:
+      containers:
+      - name: fix-invalid-spec
+        image: nginx:1.19-alpine
+        resources:
+          requests:
+            memory: 16Mi
+            cpu: 1m
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Degraded").
+		Then().
+		Given().
+		RolloutObjects(`
+apiVersion: argoproj.io/v1alpha1
+kind: AnalysisTemplate
+metadata:
+  name: doesnt-exist-yet
+spec:
+  metrics:
+  - name: web
+    interval: 5s
+    successCondition: result.major == '1'
+    provider:
+      web:
+        url: https://kubernetes.default.svc/version
+        insecure: true
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy")
+}

test/fixtures/then.go

@@ -255,3 +255,9 @@ func (t *Then) When() *When {
 		Common: t.Common,
 	}
 }
+
+func (t *Then) Given() *Given {
+	return &Given{
+		Common: t.Common,
+	}
+}