Allow deletion of node finalizer without passing checks

Longhorn 7475

Signed-off-by: Eric Weber <eric.weber@suse.com>

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
+}

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)
+			}
+		})
+	}
+}

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(), "")
 	}