Prevent users adding or changing hooks once the machine is deleted

pkg/util/annotations/helpers.go

@@ -43,3 +43,33 @@ func hasAnnotation(o metav1.Object, annotation string) bool {
 	_, ok := annotations[annotation]
 	return ok
 }
+
+// FilterFunc is used to filter annotation values.
+type FilterFunc func(key, value string) bool
+
+// Filter filters the annotations given based on the FilterFunc provided.
+func Filter(annotations map[string]string, filterFunc FilterFunc) map[string]string {
+	filtered := make(map[string]string)
+	for key, value := range annotations {
+		if filterFunc(key, value) {
+			filtered[key] = value
+		}
+	}
+	return filtered
+}
+
+// KeyHasPrefix filters keys based on the presence of a prefix.
+func KeyHasPrefix(prefix string) FilterFunc {
+	return func(key, _ string) bool {
+		return strings.HasPrefix(key, prefix)
+	}
+}
+
+// NotIn filters key value pairs that are not present within the
+// source filter set.
+func NotIn(filterSet map[string]string) FilterFunc {
+	return func(key, value string) bool {
+		filterVal, ok := filterSet[key]
+		return !ok || filterVal != value
+	}
+}

pkg/webhooks/machine_webhook.go

@@ -11,6 +11,7 @@ import (
 	osconfigv1 "github.com/openshift/api/config/v1"
 	machinev1 "github.com/openshift/api/machine/v1beta1"
 	osclientset "github.com/openshift/client-go/config/clientset/versioned"
+	"github.com/openshift/machine-api-operator/pkg/util/annotations"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -478,6 +479,20 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook {
 	}
 }
 
+func (h *machineValidatorHandler) validateMachine(m, oldM *machinev1.Machine) (bool, []string, utilerrors.Aggregate) {
+	errs := validateMachineLifecycleHooks(m, oldM)
+
+	ok, warnings, err := h.webhookOperations(m, h.admissionConfig)
+	if !ok {
+		errs = append(errs, err.Errors()...)
+	}
+
+	if len(errs) > 0 {
+		return false, warnings, utilerrors.NewAggregate(errs)
+	}
+	return true, warnings, nil
+}
+
 // Handle handles HTTP requests for admission webhook servers.
 func (h *machineValidatorHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
 	m := &machinev1.Machine{}
@@ -486,9 +501,20 @@ func (h *machineValidatorHandler) Handle(ctx context.Context, req admission.Requ
 		return admission.Errored(http.StatusBadRequest, err)
 	}
 
+	var oldM *machinev1.Machine
+	if len(req.OldObject.Raw) > 0 {
+		// oldM must only be initialised if there is an old object (ie on UPDATE or DELETE).
+		// It should be nil otherwise to allow skipping certain validations that rely on
+		// the presence of the old object.
+		oldM = &machinev1.Machine{}
+		if err := h.decoder.DecodeRaw(req.OldObject, oldM); err != nil {
+			return admission.Errored(http.StatusBadRequest, err)
+		}
+	}
+
 	klog.V(3).Infof("Validate webhook called for Machine: %s", m.GetName())
 
-	ok, warnings, errs := h.webhookOperations(m, h.admissionConfig)
+	ok, warnings, errs := h.validateMachine(m, oldM)
 	if !ok {
 		return admission.Denied(errs.Error()).WithWarnings(warnings...)
 	}
@@ -1203,3 +1229,30 @@ func isAzureGovCloud(platformStatus *osconfigv1.PlatformStatus) bool {
 	return platformStatus != nil && platformStatus.Azure != nil &&
 		platformStatus.Azure.CloudName != osconfigv1.AzurePublicCloud
 }
+
+func validateMachineLifecycleHooks(m, oldM *machinev1.Machine) []error {
+	var errs []error
+	if !isDeleting(m) || oldM == nil {
+		return errs
+	}
+
+	newPreDrain := annotations.Filter(m.GetAnnotations(), annotations.KeyHasPrefix(machinev1.MachinePreDrainDeleteHookAnnotationPrefix))
+	oldPreDrain := annotations.Filter(oldM.GetAnnotations(), annotations.KeyHasPrefix(machinev1.MachinePreDrainDeleteHookAnnotationPrefix))
+	changedPreDrain := annotations.Filter(newPreDrain, annotations.NotIn(oldPreDrain))
+	if len(changedPreDrain) > 0 {
+		errs = append(errs, field.Forbidden(field.NewPath("metadata", "annotations"), fmt.Sprintf("pre-drain hooks are immutable when machine is marked for deletion: the following hooks are new or changed: %v", changedPreDrain)))
+	}
+
+	newPreTerminate := annotations.Filter(m.GetAnnotations(), annotations.KeyHasPrefix(machinev1.MachinePreTerminateDeleteHookAnnotationPrefix))
+	oldPreTerminate := annotations.Filter(oldM.GetAnnotations(), annotations.KeyHasPrefix(machinev1.MachinePreTerminateDeleteHookAnnotationPrefix))
+	changedPreTerminate := annotations.Filter(newPreTerminate, annotations.NotIn(oldPreTerminate))
+	if len(changedPreTerminate) > 0 {
+		errs = append(errs, field.Forbidden(field.NewPath("metadata", "annotations"), fmt.Sprintf("pre-terminate hooks are immutable when machine is marked for deletion: the following hooks are new or changed: %v", changedPreTerminate)))
+	}
+
+	return errs
+}
+
+func isDeleting(obj metav1.Object) bool {
+	return obj.GetDeletionTimestamp() != nil
+}

pkg/webhooks/machine_webhook_test.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -497,13 +498,20 @@ func TestMachineUpdate(t *testing.T) {
 		g.Expect(c.Delete(ctx, azureSecret)).To(Succeed())
 	}()
 
+	preDrainHookName := "pre-drain"
+	preDrainHookKey := fmt.Sprintf("%s/%s", machinev1.MachinePreDrainDeleteHookAnnotationPrefix, preDrainHookName)
+	preDrainHookOwner := "pre-drain-owner"
+
 	testCases := []struct {
 		name                     string
 		platformType             osconfigv1.PlatformType
 		clusterID                string
 		expectedError            string
+		baseMachineAnnotations   map[string]string
 		baseProviderSpecValue    *kruntime.RawExtension
 		updatedProviderSpecValue func() *kruntime.RawExtension
+		updateAfterDelete        bool
+		updateMachine            func(m *machinev1.Machine)
 	}{
 		{
 			name:         "with a valid AWS ProviderSpec",
@@ -769,6 +777,49 @@ func TestMachineUpdate(t *testing.T) {
 			},
 			expectedError: "providerSpec.network.devices: Required value: at least 1 network device must be provided",
 		},
+		{
+			name:         "when adding a lifecycle hook",
+			platformType: osconfigv1.AWSPlatformType,
+			clusterID:    awsClusterID,
+			baseProviderSpecValue: &kruntime.RawExtension{
+				Object: defaultAWSProviderSpec.DeepCopy(),
+			},
+			updateMachine: func(m *machinev1.Machine) {
+				m.Annotations = map[string]string{
+					preDrainHookKey: preDrainHookOwner,
+				}
+			},
+		},
+		{
+			name:         "when adding a lifecycle hook after the machine has been deleted",
+			platformType: osconfigv1.AWSPlatformType,
+			clusterID:    awsClusterID,
+			baseProviderSpecValue: &kruntime.RawExtension{
+				Object: defaultAWSProviderSpec.DeepCopy(),
+			},
+			updateAfterDelete: true,
+			updateMachine: func(m *machinev1.Machine) {
+				m.Annotations = map[string]string{
+					preDrainHookKey: preDrainHookOwner,
+				}
+			},
+			expectedError: "metadata.annotations: Forbidden: pre-drain hooks are immutable when machine is marked for deletion: the following hooks are new or changed: map[pre-drain.delete.hook.machine.cluster.x-k8s.io/pre-drain:pre-drain-owner]",
+		},
+		{
+			name:         "when removing a lifecycle hook after the machine has been deleted",
+			platformType: osconfigv1.AWSPlatformType,
+			clusterID:    awsClusterID,
+			baseProviderSpecValue: &kruntime.RawExtension{
+				Object: defaultAWSProviderSpec.DeepCopy(),
+			},
+			baseMachineAnnotations: map[string]string{
+				preDrainHookKey: preDrainHookOwner,
+			},
+			updateAfterDelete: true,
+			updateMachine: func(m *machinev1.Machine) {
+				m.Annotations = map[string]string{}
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -823,6 +874,10 @@ func TestMachineUpdate(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					GenerateName: "machine-creation-",
 					Namespace:    namespace.Name,
+					Annotations:  tc.baseMachineAnnotations,
+					Finalizers: []string{
+						"machine-test",
+					},
 				},
 				Spec: machinev1.MachineSpec{
 					ProviderSpec: machinev1.ProviderSpec{
@@ -832,11 +887,29 @@ func TestMachineUpdate(t *testing.T) {
 			}
 			err = c.Create(ctx, m)
 			gs.Expect(err).ToNot(HaveOccurred())
-			defer func() {
+			if tc.updateAfterDelete {
 				gs.Expect(c.Delete(ctx, m)).To(Succeed())
+			} else {
+				defer func() {
+					gs.Expect(c.Delete(ctx, m)).To(Succeed())
+				}()
+			}
+
+			key := client.ObjectKey{Namespace: m.Namespace, Name: m.Name}
+			defer func() {
+				mc := &machinev1.Machine{}
+				gs.Expect(c.Get(ctx, key, mc)).To(Succeed())
+				mc.Finalizers = []string{}
+				gs.Expect(c.Update(ctx, mc)).To(Succeed())
 			}()
 
-			m.Spec.ProviderSpec.Value = tc.updatedProviderSpecValue()
+			gs.Expect(c.Get(ctx, key, m)).To(Succeed())
+			if tc.updatedProviderSpecValue != nil {
+				m.Spec.ProviderSpec.Value = tc.updatedProviderSpecValue()
+			}
+			if tc.updateMachine != nil {
+				tc.updateMachine(m)
+			}
 			err = c.Update(ctx, m)
 			if tc.expectedError != "" {
 				gs.Expect(err).ToNot(BeNil())