Merge pull request #176 from darkowlzz/policy-semver-range-invalid

policy: Handle failure due to invalid semver range
fluxcd · Oct 26, 2021 · 72bd994 · 72bd994
2 parents ef6e775 + 22e5ad1
commit 72bd994
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 6 deletions.
diff --git a/controllers/imagepolicy_controller.go b/controllers/imagepolicy_controller.go
@@ -48,6 +48,7 @@ import (
 const (
 	ImageRepositoryNotReadyReason = "ImageRepositoryNotReady"
 	AccessDeniedReason            = "AccessDenied"
+	ImagePolicyInvalidReason      = "InvalidPolicy"
 )
 
 const (
@@ -154,6 +155,17 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	policer, err := policy.PolicerFromSpec(pol.Spec.Policy)
+	if err != nil {
+		msg := fmt.Sprintf("invalid policy: %s", err.Error())
+		conditions.MarkFalse(
+			&pol,
+			meta.ReadyCondition,
+			ImagePolicyInvalidReason,
+			msg,
+		)
+		log.Error(err, "invalid policy")
+		return ctrl.Result{}, nil
+	}
 
 	var latest string
 	if policer != nil {

diff --git a/controllers/policy_test.go b/controllers/policy_test.go
@@ -40,6 +40,7 @@ func TestImagePolicyReconciler_calculateImageFromRepoTags(t *testing.T) {
 		versions     []string
 		policy       imagev1.ImagePolicyChoice
 		wantImageTag string
+		wantFailure  bool
 	}{
 		{
 			name:     "using SemVerPolicy",
@@ -51,6 +52,16 @@ func TestImagePolicyReconciler_calculateImageFromRepoTags(t *testing.T) {
 			},
 			wantImageTag: ":1.0.2",
 		},
+		{
+			name:     "using SemVerPolicy with invalid range",
+			versions: []string{"0.1.0", "0.1.1", "0.2.0", "1.0.0", "1.0.1", "1.0.2", "1.1.0-alpha"},
+			policy: imagev1.ImagePolicyChoice{
+				SemVer: &imagev1.SemVerPolicy{
+					Range: "*-*",
+				},
+			},
+			wantFailure: true,
+		},
 		{
 			name:     "using AlphabeticalPolicy",
 			versions: []string{"xenial", "yakkety", "zesty", "artful", "bionic"},
@@ -113,11 +124,20 @@ func TestImagePolicyReconciler_calculateImageFromRepoTags(t *testing.T) {
 
 			g.Expect(testEnv.Create(ctx, &pol)).To(Succeed())
 
-			g.Eventually(func() bool {
-				err := testEnv.Get(ctx, polName, &pol)
-				return err == nil && pol.Status.LatestImage != ""
-			}, timeout, interval).Should(BeTrue())
-			g.Expect(pol.Status.LatestImage).To(Equal(imgRepo + tt.wantImageTag))
+			if !tt.wantFailure {
+				g.Eventually(func() bool {
+					err := testEnv.Get(ctx, polName, &pol)
+					return err == nil && pol.Status.LatestImage != ""
+				}, timeout, interval).Should(BeTrue())
+				g.Expect(pol.Status.LatestImage).To(Equal(imgRepo + tt.wantImageTag))
+			} else {
+				g.Eventually(func() bool {
+					err := testEnv.Get(ctx, polName, &pol)
+					return err == nil && apimeta.IsStatusConditionFalse(pol.Status.Conditions, meta.ReadyCondition)
+				}, timeout, interval).Should(BeTrue())
+				ready := apimeta.FindStatusCondition(pol.Status.Conditions, meta.ReadyCondition)
+				g.Expect(ready.Message).To(ContainSubstring("invalid policy"))
+			}
 			g.Expect(testEnv.Delete(ctx, &pol)).To(Succeed())
 		})
 	}

diff --git a/internal/policy/factory.go b/internal/policy/factory.go
@@ -38,5 +38,8 @@ func PolicerFromSpec(choice imagev1.ImagePolicyChoice) (Policer, error) {
 		return nil, fmt.Errorf("given ImagePolicyChoice object is invalid")
 	}
 
-	return p, err
+	if err != nil {
+		return nil, err
+	}
+	return p, nil
 }
diff --git a/internal/policy/factory_test.go b/internal/policy/factory_test.go
@@ -40,4 +40,13 @@ func TestFactory_PolicerFromSpec(t *testing.T) {
 	if err != nil {
 		t.Error("should not return error")
 	}
+
+	// A nil checkable Policer for invalid policy.
+	p, err := PolicerFromSpec(imagev1.ImagePolicyChoice{SemVer: &imagev1.SemVerPolicy{Range: "*-*"}})
+	if err == nil {
+		t.Error("should return error")
+	}
+	if p != nil {
+		t.Error("should be nil")
+	}
 }