From 09e24a9dcb0ae7807596721ca224468f9cae33e9 Mon Sep 17 00:00:00 2001 From: Eric Weber Date: Wed, 3 Jan 2024 13:18:53 -0600 Subject: [PATCH] Allow deletion of node finalizer without passing checks Longhorn 7475 Signed-off-by: Eric Weber --- webhook/common/common.go | 48 +++++++++++ webhook/common/common_test.go | 121 ++++++++++++++++++++++++++++ webhook/resources/node/validator.go | 12 ++- 3 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 webhook/common/common_test.go diff --git a/webhook/common/common.go b/webhook/common/common.go index 59871aa632..45d6ee5cd8 100644 --- a/webhook/common/common.go +++ b/webhook/common/common.go @@ -108,3 +108,51 @@ func ValidateRequiredDataEngineEnabled(ds *datastore.DataStore, dataEngine longh } return nil } + +func IsRemovingLonghornFinalizer(oldObj runtime.Object, newObj runtime.Object) (bool, error) { + oldMeta, err := meta.Accessor(oldObj) + if err != nil { + return false, err + } + newMeta, err := meta.Accessor(newObj) + if err != nil { + return false, err + } + oldFinalizers := oldMeta.GetFinalizers() + newFinalizers := newMeta.GetFinalizers() + + if oldMeta.GetDeletionTimestamp() == nil { + // The object is not being deleted. + return false, nil + } + + if len(newFinalizers) != len(oldFinalizers)-1 { + // More or less than one finalizer is being removed. + return false, nil + } + + hadFinalizer := false + for _, finalizer := range oldFinalizers { + if finalizer == longhornFinalizerKey { + hadFinalizer = true + break + } + } + if !hadFinalizer { + // The old object didn't have the longhorn.io finalizer. + return false, nil + } + + hasFinalizer := false + for _, finalizer := range newFinalizers { + if finalizer == longhornFinalizerKey { + hasFinalizer = true + } + } + if hasFinalizer { + // The new object still has the longhorn.io finalizer. + return false, nil + } + + return true, nil +} diff --git a/webhook/common/common_test.go b/webhook/common/common_test.go new file mode 100644 index 0000000000..55b7adbd09 --- /dev/null +++ b/webhook/common/common_test.go @@ -0,0 +1,121 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + + longhorn "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2" +) + +func TestIsRemovingLonghornFinalizer(t *testing.T) { + assert := assert.New(t) + now := v1.Now() + + tests := map[string]struct { + oldObj runtime.Object + newObj runtime.Object + want bool + wantErr bool + }{ + "notBeingDeleted": { + oldObj: &longhorn.Node{}, + newObj: &longhorn.Node{}, + want: false, + wantErr: false, + }, + "lessThanOneFinalizerRemoved": { + oldObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{longhornFinalizerKey, "otherFinalizer"}, + }, + }, + newObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{longhornFinalizerKey, "otherFinalizer"}, + }, + }, + want: false, + wantErr: false, + }, + "moreThanOneFinalizerRemoved": { + oldObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{longhornFinalizerKey, "otherFinalizer"}, + }, + }, + newObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + }, + }, + want: false, + wantErr: false, + }, + "noLonghornFinalizerInOld": { + oldObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{"otherFinalizer"}, + }, + }, + newObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{}, + }, + }, + want: false, + wantErr: false, + }, + "longhornFinalizerInNew": { + oldObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{longhornFinalizerKey, "otherFinalizer"}, + }, + }, + newObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{longhornFinalizerKey}, + }, + }, + want: false, + wantErr: false, + }, + "correctConditions": { + oldObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{longhornFinalizerKey, "otherFinalizer"}, + }, + }, + newObj: &longhorn.Node{ + ObjectMeta: v1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{"otherFinalizer"}, + }, + }, + want: true, + wantErr: false, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got, err := IsRemovingLonghornFinalizer(tc.oldObj, tc.newObj) + assert.Equal(got, tc.want) + if tc.wantErr { + assert.Error(err) + } else { + assert.NoError(err) + } + }) + } +} diff --git a/webhook/resources/node/validator.go b/webhook/resources/node/validator.go index e33363b3a2..ad860253c7 100644 --- a/webhook/resources/node/validator.go +++ b/webhook/resources/node/validator.go @@ -15,6 +15,7 @@ import ( "github.com/longhorn/longhorn-manager/types" "github.com/longhorn/longhorn-manager/util" "github.com/longhorn/longhorn-manager/webhook/admission" + "github.com/longhorn/longhorn-manager/webhook/common" longhorn "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2" werror "github.com/longhorn/longhorn-manager/webhook/error" @@ -85,6 +86,15 @@ func (n *nodeValidator) Update(request *admission.Request, oldObj runtime.Object if !ok { return werror.NewInvalidError(fmt.Sprintf("%v is not a *longhorn.Node", newObj), "") } + isRemovingLonghornFinalizer, err := common.IsRemovingLonghornFinalizer(oldObj, newObj) + if err != nil { + err = errors.Wrap(err, "failed to check if removing longhorn.io finalizer from deleted object") + return werror.NewInvalidError(err.Error(), "") + } else if isRemovingLonghornFinalizer { + // We always allow the removal of the longhorn.io finalizer while an object is being deleted. It is the + // controller's responsibility to wait for the correct conditions to attempt to remove it. + return nil + } if newNode.Spec.InstanceManagerCPURequest < 0 { return werror.NewInvalidError("instanceManagerCPURequest should be greater than or equal to 0", "") @@ -103,7 +113,7 @@ func (n *nodeValidator) Update(request *admission.Request, oldObj runtime.Object } // We need to make sure the tags passed in are valid before updating the node. - _, err := util.ValidateTags(newNode.Spec.Tags) + _, err = util.ValidateTags(newNode.Spec.Tags) if err != nil { return werror.NewInvalidError(err.Error(), "") }