diff --git a/webhook/common/common.go b/webhook/common/common.go index ee650fbeee..0e11d4a7a3 100644 --- a/webhook/common/common.go +++ b/webhook/common/common.go @@ -88,3 +88,51 @@ func GetLonghornLabelsPatchOp(obj runtime.Object, requiredLabels, removingLabels return fmt.Sprintf(`{"op": "replace", "path": "/metadata/labels", "value": %v}`, string(bytes)), 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 ad7da7376a..4229c20498 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" @@ -70,6 +71,16 @@ func (n *nodeValidator) Update(request *admission.Request, oldObj runtime.Object oldNode := oldObj.(*longhorn.Node) newNode := newObj.(*longhorn.Node) + 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", "") } @@ -87,7 +98,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(), "") }