From e778e14c045c7269fd773fefc17f268cf7ca4a82 Mon Sep 17 00:00:00 2001 From: Atanas Dinov Date: Thu, 12 Sep 2024 00:27:41 +0300 Subject: [PATCH 1/6] Drop logger from webhooks Signed-off-by: Atanas Dinov --- api/v1alpha1/upgradeplan_webhook.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/api/v1alpha1/upgradeplan_webhook.go b/api/v1alpha1/upgradeplan_webhook.go index c9a4b7d..5027710 100644 --- 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,11 @@ 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 } // 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) @@ -97,6 +90,5 @@ 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 } From 9756b398488c0d5358cdf4d0f6eda13be348b305 Mon Sep 17 00:00:00 2001 From: Atanas Dinov Date: Thu, 12 Sep 2024 01:14:42 +0300 Subject: [PATCH 2/6] Validate a release version is specified on plan creation Signed-off-by: Atanas Dinov --- api/v1alpha1/upgradeplan_webhook.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/api/v1alpha1/upgradeplan_webhook.go b/api/v1alpha1/upgradeplan_webhook.go index 5027710..cbef29b 100644 --- a/api/v1alpha1/upgradeplan_webhook.go +++ b/api/v1alpha1/upgradeplan_webhook.go @@ -42,6 +42,15 @@ var _ webhook.Validator = &UpgradePlan{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (r *UpgradePlan) ValidateCreate() (admission.Warnings, error) { + if r.Spec.ReleaseVersion == "" { + return nil, fmt.Errorf("release version is required") + } + + _, err := version.ParseSemantic(r.Spec.ReleaseVersion) + if err != nil { + return nil, fmt.Errorf("'%s' is not a semantic version", r.Spec.ReleaseVersion) + } + return nil, nil } From cd131457d1f59c3a069ba88bd89749e4feb2b183 Mon Sep 17 00:00:00 2001 From: Atanas Dinov Date: Thu, 12 Sep 2024 10:01:48 +0300 Subject: [PATCH 3/6] Reorder update validations Signed-off-by: Atanas Dinov --- api/v1alpha1/upgradeplan_webhook.go | 51 ++++++++++++++++------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/api/v1alpha1/upgradeplan_webhook.go b/api/v1alpha1/upgradeplan_webhook.go index cbef29b..e6d2ec6 100644 --- a/api/v1alpha1/upgradeplan_webhook.go +++ b/api/v1alpha1/upgradeplan_webhook.go @@ -42,16 +42,8 @@ var _ webhook.Validator = &UpgradePlan{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (r *UpgradePlan) ValidateCreate() (admission.Warnings, error) { - if r.Spec.ReleaseVersion == "" { - return nil, fmt.Errorf("release version is required") - } - - _, err := version.ParseSemantic(r.Spec.ReleaseVersion) - if err != nil { - return nil, fmt.Errorf("'%s' is not a semantic version", r.Spec.ReleaseVersion) - } - - return nil, nil + _, err := validateReleaseVersion(r.Spec.ReleaseVersion) + return nil, err } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type @@ -67,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) @@ -82,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) } } @@ -101,3 +93,16 @@ func (r *UpgradePlan) ValidateUpdate(old runtime.Object) (admission.Warnings, er func (r *UpgradePlan) ValidateDelete() (admission.Warnings, error) { 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 +} From 056ab46762e80bb70572e8d7af91e9753e3c3696 Mon Sep 17 00:00:00 2001 From: Atanas Dinov Date: Thu, 12 Sep 2024 10:02:05 +0300 Subject: [PATCH 4/6] Add webhook tests Signed-off-by: Atanas Dinov --- api/v1alpha1/upgradeplan_webhook_test.go | 123 ++++++++++++++++++++++- 1 file changed, 118 insertions(+), 5 deletions(-) diff --git a/api/v1alpha1/upgradeplan_webhook_test.go b/api/v1alpha1/upgradeplan_webhook_test.go index 29d75df..ff9f49b 100644 --- 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()) }) }) From f1e88f8e45ee39ac0854be5f91533d0b4f3baa28 Mon Sep 17 00:00:00 2001 From: Atanas Dinov Date: Thu, 12 Sep 2024 10:03:56 +0300 Subject: [PATCH 5/6] Enable tests in CI Signed-off-by: Atanas Dinov --- .github/workflows/build_and_test.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 3120a78..268c824 100644 --- 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 From 5077bcb7d370c7cb835978510c9f5d924cbd0355 Mon Sep 17 00:00:00 2001 From: Atanas Dinov Date: Thu, 12 Sep 2024 10:16:52 +0300 Subject: [PATCH 6/6] Add arch tests Signed-off-by: Atanas Dinov --- api/v1alpha1/releasemanfest_types_test.go | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 api/v1alpha1/releasemanfest_types_test.go diff --git a/api/v1alpha1/releasemanfest_types_test.go b/api/v1alpha1/releasemanfest_types_test.go new file mode 100644 index 0000000..1efa367 --- /dev/null +++ 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") +}