From 173148e298109aa5fdcea5008cc24a4850e10d89 Mon Sep 17 00:00:00 2001
From: Sunny <darkowlzz@protonmail.com>
Date: Wed, 6 Oct 2021 00:32:48 +0530
Subject: [PATCH 1/2] policy: Handle failure due to invalid semver range

This adds error check after creating a Policer.
For alphabetical and numerical policies, the k8s API validates the
input data. But for semver policy, there aren't predefined valid
values.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
---
 controllers/imagepolicy_controller.go | 12 +++++++++++
 controllers/policy_test.go            | 30 ++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/controllers/imagepolicy_controller.go b/controllers/imagepolicy_controller.go
index 3e029476..7de9a2d2 100644
--- 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
index d644d62c..2c129a47 100644
--- 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())
 		})
 	}

From 22e5ad16d54d259951c034be9909a9ff61392154 Mon Sep 17 00:00:00 2001
From: Sunny <darkowlzz@protonmail.com>
Date: Wed, 6 Oct 2021 00:33:55 +0530
Subject: [PATCH 2/2] PolicyFromSpec: return nil on error

Returning nil wrapped in Policy interface along with error returns an
interface with a nil value. This can't be used to perform a nil check.
To make it safe, return nil value on error.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
---
 internal/policy/factory.go      | 5 ++++-
 internal/policy/factory_test.go | 9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/internal/policy/factory.go b/internal/policy/factory.go
index 40be13d8..07e939f2 100644
--- 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
index dc988b4f..2005781e 100644
--- 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")
+	}
 }