Repository: metal3-io/baremetal-operator
Pull request #1077 (2022-01-24 15:34:03)

Make bmc subscription validator less aggressive
Our intent is to make the subscription CR immutable; however, it doesn't
work if finalizers can't be added, or the spec updated.  Therefore, this
patch loosens the immutability requirement to the spec only.  This means
that any k8s bookkeeping can still happen, and we can prevent users from
changing existing subscriptions.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **fmuyassarov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @fmuyassarov` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["fmuyassarov"]} -->
bfournie: /lgtm

Patch:
diff --git a/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go b/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go
index 4e7cc579..9ddb2378 100644
--- a/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go
+++ b/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go
@@ -43,9 +43,22 @@ func (s *BMCEventSubscription) ValidateCreate() error {
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+//
+// We prevent updates to the spec.  All other updates (e.g. status, finalizers) are allowed.
 func (s *BMCEventSubscription) ValidateUpdate(old runtime.Object) error {
 	bmcsubscriptionlog.Info("validate update", "name", s.Name)
-	return fmt.Errorf("subscriptions cannot be updated, please recreate it")
+
+	bes, casted := old.(*BMCEventSubscription)
+	if !casted {
+		bmcsubscriptionlog.Error(fmt.Errorf("old object conversion error"), "validate update error")
+		return nil
+	}
+
+	if s.Spec != bes.Spec {
+		return fmt.Errorf("subscriptions cannot be updated, please recreate it")
+	}
+
+	return nil
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type


Test patch:
diff --git a/apis/metal3.io/v1alpha1/bmceventsubscription_webhook_test.go b/apis/metal3.io/v1alpha1/bmceventsubscription_webhook_test.go
new file mode 100644
index 00000000..78ef7c29
--- /dev/null
+++ b/apis/metal3.io/v1alpha1/bmceventsubscription_webhook_test.go
@@ -0,0 +1,156 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestBMCEventSubscriptionUpdate(t *testing.T) {
+	tests := []struct {
+		name      string
+		bes       *BMCEventSubscription
+		old       *BMCEventSubscription
+		wantedErr string
+	}{
+		{
+			// This is valid because the spec wasn't updated.
+			name: "valid",
+			bes: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+				Spec: BMCEventSubscriptionSpec{},
+			},
+			old: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+				Spec: BMCEventSubscriptionSpec{},
+			},
+			wantedErr: "",
+		},
+		{
+			name: "invalid",
+			bes: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+				Spec: BMCEventSubscriptionSpec{},
+			},
+			old: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+				Spec: BMCEventSubscriptionSpec{Context: "abc"},
+			},
+			wantedErr: "subscriptions cannot be updated, please recreate it",
+		},
+		{
+			// Status updates are valid
+			name: "status updated",
+			bes: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+				Spec: BMCEventSubscriptionSpec{},
+				Status: BMCEventSubscriptionStatus{
+					SubscriptionID: "some-id",
+				},
+			},
+			old: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+				Spec:   BMCEventSubscriptionSpec{},
+				Status: BMCEventSubscriptionStatus{},
+			},
+			wantedErr: "",
+		},
+		{
+			// Finalizer updates are valid
+			name: "finalizers updated",
+			bes: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test",
+					Namespace:  "test-namespace",
+					Finalizers: []string{BMCEventSubscriptionFinalizer},
+				},
+				Spec:   BMCEventSubscriptionSpec{},
+				Status: BMCEventSubscriptionStatus{},
+			},
+			old: &BMCEventSubscription{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "BMCEventSubscription",
+					APIVersion: "metal3.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test",
+					Namespace:  "test-namespace",
+					Finalizers: []string{},
+				},
+				Spec:   BMCEventSubscriptionSpec{},
+				Status: BMCEventSubscriptionStatus{},
+			},
+			wantedErr: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.bes.ValidateUpdate(tt.old); !errorContains(err, tt.wantedErr) {
+				t.Errorf("BMCEventSubscription.ValidateUpdate() error = %v, wantErr %v", err, tt.wantedErr)
+			}
+		})
+	}
+}