Add finalizer in all webhook create and update functions

datastore/longhorn.go

@@ -1419,10 +1419,6 @@ func (s *DataStore) CreateEngineImage(img *longhorn.EngineImage) (*longhorn.Engi
 
 // UpdateEngineImage updates Longhorn EngineImage and verifies update
 func (s *DataStore) UpdateEngineImage(img *longhorn.EngineImage) (*longhorn.EngineImage, error) {
-	if err := util.AddFinalizer(longhornFinalizerKey, img); err != nil {
-		return nil, err
-	}
-
 	obj, err := s.lhClient.LonghornV1beta2().EngineImages(s.namespace).Update(context.TODO(), img, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, err
@@ -1621,9 +1617,6 @@ func (s *DataStore) CreateBackingImage(backingImage *longhorn.BackingImage) (*lo
 
 // UpdateBackingImage updates Longhorn BackingImage and verifies update
 func (s *DataStore) UpdateBackingImage(backingImage *longhorn.BackingImage) (*longhorn.BackingImage, error) {
-	if err := util.AddFinalizer(longhornFinalizerKey, backingImage); err != nil {
-		return nil, err
-	}
 	obj, err := s.lhClient.LonghornV1beta2().BackingImages(s.namespace).Update(context.TODO(), backingImage, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, err
@@ -1766,9 +1759,6 @@ func initBackingImageManager(backingImageManager *longhorn.BackingImageManager)
 
 // UpdateBackingImageManager updates Longhorn BackingImageManager and verifies update
 func (s *DataStore) UpdateBackingImageManager(backingImageManager *longhorn.BackingImageManager) (*longhorn.BackingImageManager, error) {
-	if err := util.AddFinalizer(longhornFinalizerKey, backingImageManager); err != nil {
-		return nil, err
-	}
 	if err := labelLonghornNode(backingImageManager.Spec.NodeID, backingImageManager); err != nil {
 		return nil, err
 	}
@@ -1908,9 +1898,6 @@ func (s *DataStore) CreateBackingImageDataSource(backingImageDataSource *longhor
 	if err := initBackingImageDataSource(backingImageDataSource); err != nil {
 		return nil, err
 	}
-	if err := util.AddFinalizer(longhornFinalizerKey, backingImageDataSource); err != nil {
-		return nil, err
-	}
 	ret, err := s.lhClient.LonghornV1beta2().BackingImageDataSources(s.namespace).Create(context.TODO(), backingImageDataSource, metav1.CreateOptions{})
 	if err != nil {
 		return nil, err
@@ -1942,10 +1929,6 @@ func initBackingImageDataSource(backingImageDataSource *longhorn.BackingImageDat
 
 // UpdateBackingImageDataSource updates Longhorn BackingImageDataSource and verifies update
 func (s *DataStore) UpdateBackingImageDataSource(backingImageDataSource *longhorn.BackingImageDataSource) (*longhorn.BackingImageDataSource, error) {
-	if err := util.AddFinalizer(longhornFinalizerKey, backingImageDataSource); err != nil {
-		return nil, err
-	}
-
 	obj, err := s.lhClient.LonghornV1beta2().BackingImageDataSources(s.namespace).Update(context.TODO(), backingImageDataSource, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, err
@@ -3181,10 +3164,6 @@ func (s *DataStore) CreateShareManager(sm *longhorn.ShareManager) (*longhorn.Sha
 
 // UpdateShareManager updates Longhorn ShareManager resource and verifies update
 func (s *DataStore) UpdateShareManager(sm *longhorn.ShareManager) (*longhorn.ShareManager, error) {
-	if err := util.AddFinalizer(longhornFinalizerKey, sm); err != nil {
-		return nil, err
-	}
-
 	obj, err := s.lhClient.LonghornV1beta2().ShareManagers(s.namespace).Update(context.TODO(), sm, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, err

webhook/common/common.go

@@ -18,13 +18,17 @@ var (
 	longhornFinalizerKey = longhorn.SchemeGroupVersion.Group
 )
 
-func GetLonghornFinalizerPatchOp(obj runtime.Object) (string, error) {
-	if err := util.AddFinalizer(longhornFinalizerKey, obj); err != nil {
+func GetLonghornFinalizerPatchOpIfNeeded(obj runtime.Object) (string, error) {
+	metadata, err := meta.Accessor(obj)
+	if err != nil {
 		return "", err
 	}
+	if metadata.GetDeletionTimestamp() != nil {
+		// We should not add a finalizer to an object that is being deleted.
+		return "", nil
+	}
 
-	metadata, err := meta.Accessor(obj)
-	if err != nil {
+	if err := util.AddFinalizer(longhornFinalizerKey, obj); err != nil {
 		return "", err
 	}
 

webhook/resources/backingimage/mutator.go

@@ -43,9 +43,13 @@ func (b *backingImageMutator) Resource() admission.Resource {
 }
 
 func (b *backingImageMutator) Create(request *admission.Request, newObj runtime.Object) (admission.PatchOps, error) {
+	backingImage := newObj.(*longhorn.BackingImage)
 	var patchOps admission.PatchOps
 
-	backingImage := newObj.(*longhorn.BackingImage)
+	var err error
+	if patchOps, err = mutate(newObj); err != nil {
+		return nil, err
+	}
 
 	name := util.AutoCorrectName(backingImage.Name, datastore.NameMaximumLength)
 	if name != backingImage.Name {
@@ -88,20 +92,18 @@ func (b *backingImageMutator) Create(request *admission.Request, newObj runtime.
 	}
 	patchOps = append(patchOps, patchOp)
 
-	patchOp, err = common.GetLonghornFinalizerPatchOp(backingImage)
-	if err != nil {
-		err := errors.Wrapf(err, "failed to get finalizer patch for backingImage %v", backingImage.Name)
-		return nil, werror.NewInvalidError(err.Error(), "")
-	}
-	patchOps = append(patchOps, patchOp)
-
 	return patchOps, nil
 }
 
 func (b *backingImageMutator) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) (admission.PatchOps, error) {
 	backingImage := newObj.(*longhorn.BackingImage)
 	var patchOps admission.PatchOps
 
+	var err error
+	if patchOps, err = mutate(newObj); err != nil {
+		return nil, err
+	}
+
 	// Backward compatibility
 	// SourceType is set to "download" if it is empty
 	if backingImage.Spec.SourceType == "" {
@@ -117,3 +119,20 @@ func (b *backingImageMutator) Update(request *admission.Request, oldObj runtime.
 
 	return patchOps, nil
 }
+
+// mutate contains functionality shared by Create and Update.
+func mutate(newObj runtime.Object) (admission.PatchOps, error) {
+	backingImage := newObj.(*longhorn.BackingImage)
+	var patchOps admission.PatchOps
+
+	patchOp, err := common.GetLonghornFinalizerPatchOpIfNeeded(backingImage)
+	if err != nil {
+		err := errors.Wrapf(err, "failed to get finalizer patch for backingImage %v", backingImage.Name)
+		return nil, werror.NewInvalidError(err.Error(), "")
+	}
+	if patchOp != "" {
+		patchOps = append(patchOps, patchOp)
+	}
+
+	return patchOps, nil
+}

webhook/resources/backingimagedatasource/mutator.go

@@ -3,12 +3,14 @@ package backingimagedatasource
 import (
 	"fmt"
 
-	admissionregv1 "k8s.io/api/admissionregistration/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-
 	"github.com/longhorn/longhorn-manager/datastore"
 	longhorn "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2"
 	"github.com/longhorn/longhorn-manager/webhook/admission"
+	"github.com/longhorn/longhorn-manager/webhook/common"
+	werror "github.com/longhorn/longhorn-manager/webhook/error"
+	"github.com/pkg/errors"
+	admissionregv1 "k8s.io/api/admissionregistration/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 type backingImageDataSourceMutator struct {
@@ -35,25 +37,35 @@ func (b *backingImageDataSourceMutator) Resource() admission.Resource {
 }
 
 func (b *backingImageDataSourceMutator) Create(request *admission.Request, newObj runtime.Object) (admission.PatchOps, error) {
-	return mutateBackingImageDatasource(newObj)
+	return mutate(newObj)
 }
 
 func (b *backingImageDataSourceMutator) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) (admission.PatchOps, error) {
-	return mutateBackingImageDatasource(newObj)
+	return mutate(newObj)
 }
 
-func mutateBackingImageDatasource(newObj runtime.Object) (admission.PatchOps, error) {
+// mutate contains functionality shared by Create and Update.
+func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 	var patchOps admission.PatchOps
 
-	backingimagedatasource := newObj.(*longhorn.BackingImageDataSource)
+	backingImageDataSource := newObj.(*longhorn.BackingImageDataSource)
 
-	if backingimagedatasource.Spec.SourceType == "" {
+	if backingImageDataSource.Spec.SourceType == "" {
 		patchOps = append(patchOps, fmt.Sprintf(`{"op": "replace", "path": "/spec/sourceType", "value": "%s"}`, longhorn.BackingImageDataSourceTypeDownload))
 	}
 
-	if backingimagedatasource.Spec.Parameters == nil {
+	if backingImageDataSource.Spec.Parameters == nil {
 		patchOps = append(patchOps, `{"op": "replace", "path": "/spec/parameters", "value": {}}`)
 	}
 
+	patchOp, err := common.GetLonghornFinalizerPatchOpIfNeeded(backingImageDataSource)
+	if err != nil {
+		err := errors.Wrapf(err, "failed to get finalizer patch for backingImageDataSource %v", backingImageDataSource.Name)
+		return nil, werror.NewInvalidError(err.Error(), "")
+	}
+	if patchOp != "" {
+		patchOps = append(patchOps, patchOp)
+	}
+
 	return patchOps, nil
 }

webhook/resources/backingimagemanager/mutator.go

@@ -36,27 +36,14 @@ func (b *backingImageManagerMutator) Resource() admission.Resource {
 }
 
 func (b *backingImageManagerMutator) Create(request *admission.Request, newObj runtime.Object) (admission.PatchOps, error) {
-	backingImageManager := newObj.(*longhorn.BackingImageManager)
-
-	patchOps, err := mutate(newObj)
-	if err != nil {
-		return nil, werror.NewInvalidError(err.Error(), "")
-	}
-
-	patchOp, err := common.GetLonghornFinalizerPatchOp(backingImageManager)
-	if err != nil {
-		err := errors.Wrapf(err, "failed to get finalizer patch for backingImageManager %v", backingImageManager.Name)
-		return nil, werror.NewInvalidError(err.Error(), "")
-	}
-	patchOps = append(patchOps, patchOp)
-
-	return patchOps, nil
+	return mutate(newObj)
 }
 
 func (b *backingImageManagerMutator) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) (admission.PatchOps, error) {
 	return mutate(newObj)
 }
 
+// mutate contains functionality shared by Create and Update.
 func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 	backingImageManager := newObj.(*longhorn.BackingImageManager)
 	var patchOps admission.PatchOps
@@ -65,5 +52,14 @@ func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 		patchOps = append(patchOps, `{"op": "replace", "path": "/spec/backingImages", "value": {}}`)
 	}
 
+	patchOp, err := common.GetLonghornFinalizerPatchOpIfNeeded(backingImageManager)
+	if err != nil {
+		err := errors.Wrapf(err, "failed to get finalizer patch for backingImageManager %v", backingImageManager.Name)
+		return nil, werror.NewInvalidError(err.Error(), "")
+	}
+	if patchOp != "" {
+		patchOps = append(patchOps, patchOp)
+	}
+
 	return patchOps, nil
 }

webhook/resources/backup/mutator.go

@@ -42,18 +42,12 @@ func (b *backupMutator) Resource() admission.Resource {
 
 func (b *backupMutator) Create(request *admission.Request, newObj runtime.Object) (admission.PatchOps, error) {
 	backup := newObj.(*longhorn.Backup)
+	var patchOps admission.PatchOps
 
-	patchOps, err := mutate(newObj)
-	if err != nil {
-		return nil, werror.NewInvalidError(err.Error(), "")
-	}
-
-	patchOp, err := common.GetLonghornFinalizerPatchOp(backup)
-	if err != nil {
-		err := errors.Wrapf(err, "failed to get finalizer patch for backup %v", backup.Name)
-		return nil, werror.NewInvalidError(err.Error(), "")
+	var err error
+	if patchOps, err = mutate(newObj); err != nil {
+		return nil, err
 	}
-	patchOps = append(patchOps, patchOp)
 
 	backupLabels := backup.Spec.Labels
 	if backupLabels == nil {
@@ -88,6 +82,7 @@ func (b *backupMutator) Update(request *admission.Request, oldObj runtime.Object
 	return mutate(newObj)
 }
 
+// mutate contains functionality shared by Create and Update.
 func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 	backup := newObj.(*longhorn.Backup)
 	var patchOps admission.PatchOps
@@ -96,5 +91,14 @@ func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 		patchOps = append(patchOps, `{"op": "replace", "path": "/spec/labels", "value": {}}`)
 	}
 
+	patchOp, err := common.GetLonghornFinalizerPatchOpIfNeeded(backup)
+	if err != nil {
+		err := errors.Wrapf(err, "failed to get finalizer patch for backup %v", backup.Name)
+		return nil, werror.NewInvalidError(err.Error(), "")
+	}
+	if patchOp != "" {
+		patchOps = append(patchOps, patchOp)
+	}
+
 	return patchOps, nil
 }

webhook/resources/backupvolume/mutator.go

@@ -30,21 +30,32 @@ func (b *backupVolumeMutator) Resource() admission.Resource {
 		ObjectType: &longhorn.BackupVolume{},
 		OperationTypes: []admissionregv1.OperationType{
 			admissionregv1.Create,
+			admissionregv1.Update,
 		},
 	}
 }
 
 func (b *backupVolumeMutator) Create(request *admission.Request, newObj runtime.Object) (admission.PatchOps, error) {
-	var patchOps admission.PatchOps
+	return mutate(newObj)
+}
+
+func (b *backupVolumeMutator) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) (admission.PatchOps, error) {
+	return mutate(newObj)
+}
 
+// mutate contains functionality shared by Create and Update.
+func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 	backupVolume := newObj.(*longhorn.BackupVolume)
+	var patchOps admission.PatchOps
 
-	patchOp, err := common.GetLonghornFinalizerPatchOp(backupVolume)
+	patchOp, err := common.GetLonghornFinalizerPatchOpIfNeeded(backupVolume)
 	if err != nil {
 		err := errors.Wrapf(err, "failed to get finalizer patch for backupVolume %v", backupVolume.Name)
 		return nil, werror.NewInvalidError(err.Error(), "")
 	}
-	patchOps = append(patchOps, patchOp)
+	if patchOp != "" {
+		patchOps = append(patchOps, patchOp)
+	}
 
 	return patchOps, nil
 }

webhook/resources/engine/mutator.go

@@ -38,17 +38,17 @@ func (e *engineMutator) Resource() admission.Resource {
 }
 
 func (e *engineMutator) Create(request *admission.Request, newObj runtime.Object) (admission.PatchOps, error) {
-	return mutateEngine(newObj, true)
+	return mutate(newObj)
 }
 
 func (e *engineMutator) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) (admission.PatchOps, error) {
-	return mutateEngine(newObj, false)
+	return mutate(newObj)
 }
 
-func mutateEngine(newObj runtime.Object, needFinalizer bool) (admission.PatchOps, error) {
-	var patchOps admission.PatchOps
-
+// mutate contains functionality shared by Create and Update.
+func mutate(newObj runtime.Object) (admission.PatchOps, error) {
 	engine := newObj.(*longhorn.Engine)
+	var patchOps admission.PatchOps
 
 	if engine.Spec.ReplicaAddressMap == nil {
 		patchOps = append(patchOps, `{"op": "replace", "path": "/spec/replicaAddressMap", "value": {}}`)
@@ -69,12 +69,12 @@ func mutateEngine(newObj runtime.Object, needFinalizer bool) (admission.PatchOps
 	}
 	patchOps = append(patchOps, patchOp)
 
-	if needFinalizer {
-		patchOp, err = common.GetLonghornFinalizerPatchOp(engine)
-		if err != nil {
-			err := errors.Wrapf(err, "failed to get finalizer patch for engine %v", engine.Name)
-			return nil, werror.NewInvalidError(err.Error(), "")
-		}
+	patchOp, err = common.GetLonghornFinalizerPatchOpIfNeeded(engine)
+	if err != nil {
+		err := errors.Wrapf(err, "failed to get finalizer patch for engine %v", engine.Name)
+		return nil, werror.NewInvalidError(err.Error(), "")
+	}
+	if patchOp != "" {
 		patchOps = append(patchOps, patchOp)
 	}