Add webhook tests (#80)

* Drop logger from webhooks Signed-off-by: Atanas Dinov <[email protected]> * Validate a release version is specified on plan creation Signed-off-by: Atanas Dinov <[email protected]> * Reorder update validations Signed-off-by: Atanas Dinov <[email protected]> * Add webhook tests Signed-off-by: Atanas Dinov <[email protected]> * Enable tests in CI Signed-off-by: Atanas Dinov <[email protected]> * Add arch tests Signed-off-by: Atanas Dinov <[email protected]> --------- Signed-off-by: Atanas Dinov <[email protected]>
suse-edge · Sep 12, 2024 · 23f05cc · 23f05cc
1 parent 3ef7610
commit 23f05cc
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 30 deletions.
diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
@@ -12,6 +12,5 @@ jobs:
           go-version: '1.22'
       - name: Build
         run: go build -v ./...
-#      TODO: Fix tests and enable step
-#      - name: Test
-#        run: go test -v ./... -tags integration
+      - name: Test
+        run: make test
diff --git a/api/v1alpha1/releasemanfest_types_test.go b/api/v1alpha1/releasemanfest_types_test.go
@@ -0,0 +1,29 @@
+package v1alpha1
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestArch_Short(t *testing.T) {
+	assert.Equal(t, "amd64", ArchTypeX86.Short())
+	assert.Equal(t, "arm64", ArchTypeARM.Short())
+	assert.PanicsWithValue(t, "unknown arch: abc", func() {
+		arch := Arch("abc")
+		arch.Short()
+	})
+}
+
+func TestSupportedArchitectures(t *testing.T) {
+	archs := []Arch{ArchTypeARM, ArchTypeX86}
+
+	supported := SupportedArchitectures(archs)
+	require.Len(t, supported, 4)
+
+	assert.Contains(t, supported, "x86_64")
+	assert.Contains(t, supported, "aarch64")
+	assert.Contains(t, supported, "amd64")
+	assert.Contains(t, supported, "arm64")
+}
diff --git a/api/v1alpha1/upgradeplan_webhook.go b/api/v1alpha1/upgradeplan_webhook.go
@@ -23,14 +23,10 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/version"
 	ctrl "sigs.k8s.io/controller-runtime"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
-// log is for logging in this package.
-var upgradeplanlog = logf.Log.WithName("upgradeplan-resource")
-
 // SetupWebhookWithManager will setup the manager to manage the webhooks
 func (r *UpgradePlan) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
@@ -46,14 +42,12 @@ var _ webhook.Validator = &UpgradePlan{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *UpgradePlan) ValidateCreate() (admission.Warnings, error) {
-	upgradeplanlog.Info("validate create", "name", r.Name)
-	return nil, nil
+	_, err := validateReleaseVersion(r.Spec.ReleaseVersion)
+	return nil, err
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *UpgradePlan) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	upgradeplanlog.Info("validate update", "name", r.Name)
-
 	oldPlan, ok := old.(*UpgradePlan)
 	if !ok {
 		return nil, fmt.Errorf("unexpected object type: %T", old)
@@ -65,12 +59,20 @@ func (r *UpgradePlan) ValidateUpdate(old runtime.Object) (admission.Warnings, er
 		return nil, nil
 	}
 
-	if oldPlan.Status.LastSuccessfulReleaseVersion != "" {
-		newReleaseVersion, err := version.ParseSemantic(r.Spec.ReleaseVersion)
-		if err != nil {
-			return nil, fmt.Errorf("'%s' is not a semantic version", r.Spec.ReleaseVersion)
+	disallowingUpdateStates := []string{UpgradeInProgress, UpgradePending, UpgradeError}
+
+	for _, condition := range r.Status.Conditions {
+		if slices.Contains(disallowingUpdateStates, condition.Reason) {
+			return nil, fmt.Errorf("upgrade plan cannot be edited while condition '%s' is in '%s' state", condition.Type, condition.Reason)
 		}
+	}
 
+	newReleaseVersion, err := validateReleaseVersion(r.Spec.ReleaseVersion)
+	if err != nil {
+		return nil, err
+	}
+
+	if oldPlan.Status.LastSuccessfulReleaseVersion != "" {
 		indicator, err := newReleaseVersion.Compare(oldPlan.Status.LastSuccessfulReleaseVersion)
 		if err != nil {
 			return nil, fmt.Errorf("comparing versions: %w", err)
@@ -80,15 +82,7 @@ func (r *UpgradePlan) ValidateUpdate(old runtime.Object) (admission.Warnings, er
 		case 0:
 			return nil, fmt.Errorf("any edits over '%s' must come with an increment of the releaseVersion", r.Name)
 		case -1:
-			return nil, fmt.Errorf("new releaseVersion '%s' must be greater than the currently applied '%s' releaseVersion", r.Spec.ReleaseVersion, oldPlan.Status.LastSuccessfulReleaseVersion)
-		}
-	}
-
-	disallowingUpdateStates := []string{UpgradeInProgress, UpgradePending, UpgradeError}
-
-	for _, condition := range r.Status.Conditions {
-		if slices.Contains(disallowingUpdateStates, condition.Reason) {
-			return nil, fmt.Errorf("upgrade plan cannot be edited while condition '%s' is in '%s' state", condition.Type, condition.Reason)
+			return nil, fmt.Errorf("new releaseVersion must be greater than the currently applied one ('%s')", oldPlan.Status.LastSuccessfulReleaseVersion)
 		}
 	}
 
@@ -97,6 +91,18 @@ func (r *UpgradePlan) ValidateUpdate(old runtime.Object) (admission.Warnings, er
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *UpgradePlan) ValidateDelete() (admission.Warnings, error) {
-	upgradeplanlog.Info("validate delete", "name", r.Name)
 	return nil, nil
 }
+
+func validateReleaseVersion(releaseVersion string) (*version.Version, error) {
+	if releaseVersion == "" {
+		return nil, fmt.Errorf("release version is required")
+	}
+
+	v, err := version.ParseSemantic(releaseVersion)
+	if err != nil {
+		return nil, fmt.Errorf("'%s' is not a semantic version", releaseVersion)
+	}
+
+	return v, nil
+}
diff --git a/api/v1alpha1/upgradeplan_webhook_test.go b/api/v1alpha1/upgradeplan_webhook_test.go
@@ -18,21 +18,134 @@ package v1alpha1
 
 import (
 	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 var _ = Describe("UpgradePlan Webhook", func() {
+	Context("When creating UpgradePlans under Validating Webhook", func() {
+		It("Should be denied if release version is not specified", func() {
+			plan := &UpgradePlan{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "plan1",
+					Namespace: "default",
+				},
+			}
+
+			err := k8sClient.Create(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("release version is required")))
+		})
+
+		It("Should be denied if release version is not in semantic format", func() {
+			plan := &UpgradePlan{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "plan1",
+					Namespace: "default",
+				},
+				Spec: UpgradePlanSpec{
+					ReleaseVersion: "v1",
+				},
+			}
 
-	Context("When creating UpgradePlan under Validating Webhook", func() {
-		It("Should deny if a required field is empty", func() {
+			err := k8sClient.Create(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("'v1' is not a semantic version")))
+		})
+	})
 
-			// TODO(user): Add your logic here
+	Context("When updating UpgradePlan under Validating Webhook", Ordered, func() {
+		plan := &UpgradePlan{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "plan1",
+				Namespace: "default",
+			},
+			Spec: UpgradePlanSpec{
+				ReleaseVersion: "3.1.0",
+			},
+		}
+
+		BeforeAll(func() {
+			By("Creating the plan")
+			Expect(k8sClient.Create(ctx, plan)).To(Succeed())
+		})
 
+		AfterEach(func() {
+			By("Cleaning up status conditions")
+			plan.Status.Conditions = nil
+			Expect(k8sClient.Status().Update(ctx, plan)).To(Succeed())
 		})
 
-		It("Should admit if all required fields are provided", func() {
+		It("Should be denied when an upgrade is pending", func() {
+			condition := metav1.Condition{Type: KubernetesUpgradedCondition, Status: metav1.ConditionFalse, Reason: UpgradePending}
+
+			meta.SetStatusCondition(&plan.Status.Conditions, condition)
+			Expect(k8sClient.Status().Update(ctx, plan)).To(Succeed())
+
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("upgrade plan cannot be edited while condition 'KubernetesUpgraded' is in 'Pending' state")))
+		})
+
+		It("Should be denied when an upgrade is in progress", func() {
+			condition := metav1.Condition{Type: KubernetesUpgradedCondition, Status: metav1.ConditionFalse, Reason: UpgradeInProgress}
+
+			meta.SetStatusCondition(&plan.Status.Conditions, condition)
+			Expect(k8sClient.Status().Update(ctx, plan)).To(Succeed())
+
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("upgrade plan cannot be edited while condition 'KubernetesUpgraded' is in 'InProgress' state")))
+		})
 
-			// TODO(user): Add your logic here
+		It("Should be denied when an upgrade is experiencing a transient error", func() {
+			condition := metav1.Condition{Type: KubernetesUpgradedCondition, Status: metav1.ConditionFalse, Reason: UpgradeError}
+
+			meta.SetStatusCondition(&plan.Status.Conditions, condition)
+			Expect(k8sClient.Status().Update(ctx, plan)).To(Succeed())
+
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("upgrade plan cannot be edited while condition 'KubernetesUpgraded' is in 'Error' state")))
+		})
+
+		It("Should be denied if release version is not specified", func() {
+			plan.Spec.ReleaseVersion = ""
+
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("release version is required")))
+		})
+
+		It("Should be denied if release version is not in semantic format", func() {
+			plan.Spec.ReleaseVersion = "v1"
+
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("'v1' is not a semantic version")))
+		})
+
+		It("Should be denied if the new release version is the same as the last applied one", func() {
+			plan.Status.LastSuccessfulReleaseVersion = "3.1.0"
+			Expect(k8sClient.Status().Update(ctx, plan)).To(Succeed())
+
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("any edits over 'plan1' must come with an increment of the releaseVersion")))
+		})
+
+		It("Should be denied if the new release version is lesser than the last applied one", func() {
+			plan.Spec.ReleaseVersion = "3.0.2"
+			err := k8sClient.Update(ctx, plan)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError(ContainSubstring("new releaseVersion must be greater than the currently applied one ('3.1.0')")))
+		})
 
+		It("Should pass if the new release version is greater than the last applied one", func() {
+			plan.Spec.ReleaseVersion = "3.1.1"
+			Expect(k8sClient.Update(ctx, plan)).To(Succeed())
 		})
 	})