Bug 2041971: Revert use webhookApply implementation from Library Go

pkg/operator/sync.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/imdario/mergo"
 	"github.com/openshift/library-go/pkg/operator/events"
 	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
 	"github.com/openshift/library-go/pkg/operator/resource/resourcehash"
@@ -13,6 +14,7 @@ import (
 	"github.com/openshift/machine-api-operator/pkg/metrics"
 	"github.com/openshift/machine-api-operator/pkg/util/conditions"
 	mapiwebhooks "github.com/openshift/machine-api-operator/pkg/webhooks"
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -195,33 +197,121 @@ func (optr *Operator) syncWebhookConfiguration() error {
 }
 
 func (optr *Operator) syncValidatingWebhook() error {
-	validatingWebhook, updated, err := resourceapply.ApplyValidatingWebhookConfiguration(context.TODO(), optr.kubeClient.AdmissionregistrationV1(),
-		events.NewLoggingEventRecorder(optr.name),
-		mapiwebhooks.NewValidatingWebhookConfiguration())
+	client := optr.kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations()
+	ctx := context.TODO()
+	expected := mapiwebhooks.NewValidatingWebhookConfiguration()
+
+	current, err := client.Get(ctx, expected.Name, metav1.GetOptions{})
+	if apierrors.IsNotFound(err) {
+		// Doesn't exist yet so create a fresh configuration
+		if _, err := client.Create(ctx, expected, metav1.CreateOptions{}); err != nil {
+			return fmt.Errorf("error creating ValidatingWebhookConfiguration: %v", err)
+		}
+		return nil
+	} else if err != nil {
+		return fmt.Errorf("error getting ValidatingWebhookConfiguration: %v", err)
+	}
+
+	// The webhook already exists, so merge the existing fields with the desired fields
+	if err := mergo.Merge(expected, current); err != nil {
+		return fmt.Errorf("error merging ValidatingWebhookConfiguration: %v", err)
+	}
+	// Merge webhooks separately as slices are normally overwritten by mergo and
+	// we need to preserve the defaults that are set within the webhooks
+	expected.Webhooks, err = mergeValidatingWebhooks(expected.Webhooks, current.Webhooks)
 	if err != nil {
-		return err
+		return fmt.Errorf("error merging ValidatingWebhooks: %v", err)
 	}
-	if updated {
-		resourcemerge.SetValidatingWebhooksConfigurationGeneration(&optr.generations, validatingWebhook)
+
+	if _, err := client.Update(ctx, expected, metav1.UpdateOptions{}); err != nil {
+		return fmt.Errorf("error updating ValidatingWebhookConfiguration: %v", err)
 	}
 
 	return nil
 }
 
+// mergeValidatingWebhooks merges the two sets of webhooks so that any defaulted or additional fields (eg CABundle)
+// are preserved from the current set of webhooks.
+// In any case where fields in the webhooks differ, expected takes precedence.
+// Webhooks are merged using Name as the key, if any webhook is present in current but not expected, it is dropped.
+func mergeValidatingWebhooks(expected, current []admissionregistrationv1.ValidatingWebhook) ([]admissionregistrationv1.ValidatingWebhook, error) {
+	currentSet := make(map[string]admissionregistrationv1.ValidatingWebhook)
+	for _, webhook := range current {
+		currentSet[webhook.Name] = webhook
+	}
+
+	out := []admissionregistrationv1.ValidatingWebhook{}
+	for _, expectedWebhook := range expected {
+		// If a current webhook exists with the same name, merge it with the expected one
+		if currentWebhook, found := currentSet[expectedWebhook.Name]; found {
+			if err := mergo.Merge(&expectedWebhook, currentWebhook); err != nil {
+				return nil, fmt.Errorf("error merging webhook %q: %v", expectedWebhook.Name, err)
+			}
+		}
+		out = append(out, expectedWebhook)
+	}
+
+	return out, nil
+}
+
 func (optr *Operator) syncMutatingWebhook() error {
-	validatingWebhook, updated, err := resourceapply.ApplyMutatingWebhookConfiguration(context.TODO(), optr.kubeClient.AdmissionregistrationV1(),
-		events.NewLoggingEventRecorder(optr.name),
-		mapiwebhooks.NewMutatingWebhookConfiguration())
+	client := optr.kubeClient.AdmissionregistrationV1().MutatingWebhookConfigurations()
+	ctx := context.TODO()
+	expected := mapiwebhooks.NewMutatingWebhookConfiguration()
+
+	current, err := client.Get(ctx, expected.Name, metav1.GetOptions{})
+	if apierrors.IsNotFound(err) {
+		// Doesn't exist yet so create a fresh configuration
+		if _, err := client.Create(ctx, expected, metav1.CreateOptions{}); err != nil {
+			return fmt.Errorf("error creating MutatingWebhookConfiguration: %v", err)
+		}
+		return nil
+	} else if err != nil {
+		return fmt.Errorf("error getting MutatingWebhookConfiguration: %v", err)
+	}
+
+	// The webhook already exists, so merge the existing fields with the desired fields
+	if err := mergo.Merge(expected, current); err != nil {
+		return fmt.Errorf("error merging MutatingWebhookConfiguration: %v", err)
+	}
+	// Merge webhooks separately as slices are normally overwritten by mergo and
+	// we need to preserve the defaults that are set within the webhooks
+	expected.Webhooks, err = mergeMutatingWebhooks(expected.Webhooks, current.Webhooks)
 	if err != nil {
-		return err
+		return fmt.Errorf("error merging MutatingWebhooks: %v", err)
 	}
-	if updated {
-		resourcemerge.SetMutatingWebhooksConfigurationGeneration(&optr.generations, validatingWebhook)
+
+	if _, err := client.Update(ctx, expected, metav1.UpdateOptions{}); err != nil {
+		return fmt.Errorf("error updating MutatingWebhookConfiguration: %v", err)
 	}
 
 	return nil
 }
 
+// mergeMutatingWebhooks merges the two sets of webhooks so that any defaulted or additional fields (eg CABundle)
+// are preserved from the current set of webhooks.
+// In any case where fields in the webhooks differ, expected takes precedence.
+// Webhooks are merged using Name as the key, if any webhook is present in current but not expected, it is dropped.
+func mergeMutatingWebhooks(expected, current []admissionregistrationv1.MutatingWebhook) ([]admissionregistrationv1.MutatingWebhook, error) {
+	currentSet := make(map[string]admissionregistrationv1.MutatingWebhook)
+	for _, webhook := range current {
+		currentSet[webhook.Name] = webhook
+	}
+
+	out := []admissionregistrationv1.MutatingWebhook{}
+	for _, expectedWebhook := range expected {
+		// If a current webhook exists with the same name, merge it with the expected one
+		if currentWebhook, found := currentSet[expectedWebhook.Name]; found {
+			if err := mergo.Merge(&expectedWebhook, currentWebhook); err != nil {
+				return nil, fmt.Errorf("error merging webhook %q: %v", expectedWebhook.Name, err)
+			}
+		}
+		out = append(out, expectedWebhook)
+	}
+
+	return out, nil
+}
+
 func (optr *Operator) checkDeploymentRolloutStatus(resource *appsv1.Deployment) (reconcile.Result, error) {
 	d, err := optr.kubeClient.AppsV1().Deployments(resource.Namespace).Get(context.Background(), resource.Name, metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {

pkg/operator/sync_test.go

@@ -1,15 +1,20 @@
 package operator
 
 import (
+	"context"
 	"testing"
 	"time"
 
+	. "github.com/onsi/gomega"
+	mapiwebhooks "github.com/openshift/machine-api-operator/pkg/webhooks"
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/utils/pointer"
 )
 
 func TestCheckDeploymentRolloutStatus(t *testing.T) {
@@ -135,6 +140,174 @@ func TestCheckDeploymentRolloutStatus(t *testing.T) {
 	}
 }
 
+func TestSyncValidatingWebhooks(t *testing.T) {
+	defaultConfiguration := mapiwebhooks.NewValidatingWebhookConfiguration()
+
+	withCABundle := defaultConfiguration.DeepCopy()
+	for i, webhook := range withCABundle.Webhooks {
+		webhook.ClientConfig.CABundle = []byte("test")
+		webhook.TimeoutSeconds = pointer.Int32Ptr(10)
+		withCABundle.Webhooks[i] = webhook
+	}
+
+	fail := admissionregistrationv1.Fail
+	withExtraWebhook := withCABundle.DeepCopy()
+	withExtraWebhook.Webhooks = append(withExtraWebhook.Webhooks, admissionregistrationv1.ValidatingWebhook{
+		Name:          "extra.webhook",
+		FailurePolicy: &fail,
+	})
+
+	withChangedFields := withCABundle.DeepCopy()
+	for i, webhook := range withChangedFields.Webhooks {
+		fail := admissionregistrationv1.Fail
+		webhook.FailurePolicy = &fail
+
+		webhook.ClientConfig.Service.Name = "wrong.service.name"
+		webhook.Rules = append(webhook.Rules, webhook.Rules...)
+		withChangedFields.Webhooks[i] = webhook
+	}
+
+	cases := []struct {
+		name            string
+		existingWebhook *admissionregistrationv1.ValidatingWebhookConfiguration
+		expectedWebhook *admissionregistrationv1.ValidatingWebhookConfiguration
+	}{
+		{
+			name:            "It should create the configuration if it does not exist",
+			expectedWebhook: defaultConfiguration.DeepCopy(),
+		},
+		{
+			name:            "It should not overwrite the cabundle or defaulted fields once populated",
+			expectedWebhook: withCABundle.DeepCopy(),
+			existingWebhook: withCABundle.DeepCopy(),
+		},
+		{
+			name:            "It should drop any extra webhooks present",
+			expectedWebhook: withCABundle.DeepCopy(),
+			existingWebhook: withExtraWebhook.DeepCopy(),
+		},
+		{
+			name:            "It should overwrite any fields that have been changed",
+			expectedWebhook: withCABundle.DeepCopy(),
+			existingWebhook: withChangedFields.DeepCopy(),
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			stop := make(chan struct{})
+			defer close(stop)
+
+			kubeObjs := []runtime.Object{}
+			if tc.existingWebhook != nil {
+				kubeObjs = append(kubeObjs, tc.existingWebhook)
+			}
+
+			optr := newFakeOperator(kubeObjs, nil, stop)
+
+			err := optr.syncValidatingWebhook()
+			g.Expect(err).ToNot(HaveOccurred())
+
+			if tc.expectedWebhook == nil {
+				// Nothing to check
+				return
+			}
+
+			client := optr.kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations()
+			gotWebhook, err := client.Get(context.Background(), tc.expectedWebhook.Name, metav1.GetOptions{})
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(gotWebhook).To(Equal(tc.expectedWebhook))
+		})
+	}
+}
+
+func TestSyncMutatingWebhooks(t *testing.T) {
+	defaultConfiguration := mapiwebhooks.NewMutatingWebhookConfiguration()
+
+	withCABundle := defaultConfiguration.DeepCopy()
+	for i, webhook := range withCABundle.Webhooks {
+		webhook.ClientConfig.CABundle = []byte("test")
+		webhook.TimeoutSeconds = pointer.Int32Ptr(10)
+		never := admissionregistrationv1.NeverReinvocationPolicy
+		webhook.ReinvocationPolicy = &never
+		withCABundle.Webhooks[i] = webhook
+	}
+
+	fail := admissionregistrationv1.Fail
+	withExtraWebhook := withCABundle.DeepCopy()
+	withExtraWebhook.Webhooks = append(withExtraWebhook.Webhooks, admissionregistrationv1.MutatingWebhook{
+		Name:          "extra.webhook",
+		FailurePolicy: &fail,
+	})
+
+	withChangedFields := withCABundle.DeepCopy()
+	for i, webhook := range withChangedFields.Webhooks {
+		fail := admissionregistrationv1.Fail
+		webhook.FailurePolicy = &fail
+
+		webhook.ClientConfig.Service.Name = "wrong.service.name"
+		webhook.Rules = append(webhook.Rules, webhook.Rules...)
+		withChangedFields.Webhooks[i] = webhook
+	}
+
+	cases := []struct {
+		name            string
+		existingWebhook *admissionregistrationv1.MutatingWebhookConfiguration
+		expectedWebhook *admissionregistrationv1.MutatingWebhookConfiguration
+	}{
+		{
+			name:            "It should create the configuration if it does not exist",
+			expectedWebhook: defaultConfiguration.DeepCopy(),
+		},
+		{
+			name:            "It should not overwrite the cabundle or defaulted fields once populated",
+			expectedWebhook: withCABundle.DeepCopy(),
+			existingWebhook: withCABundle.DeepCopy(),
+		},
+		{
+			name:            "It should drop any extra webhooks present",
+			expectedWebhook: withCABundle.DeepCopy(),
+			existingWebhook: withExtraWebhook.DeepCopy(),
+		},
+		{
+			name:            "It should overwrite any fields that have been changed",
+			expectedWebhook: withCABundle.DeepCopy(),
+			existingWebhook: withChangedFields.DeepCopy(),
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			stop := make(chan struct{})
+			defer close(stop)
+
+			kubeObjs := []runtime.Object{}
+			if tc.existingWebhook != nil {
+				kubeObjs = append(kubeObjs, tc.existingWebhook)
+			}
+
+			optr := newFakeOperator(kubeObjs, nil, stop)
+
+			err := optr.syncMutatingWebhook()
+			g.Expect(err).ToNot(HaveOccurred())
+
+			if tc.expectedWebhook == nil {
+				// Nothing to check
+				return
+			}
+
+			client := optr.kubeClient.AdmissionregistrationV1().MutatingWebhookConfigurations()
+			gotWebhook, err := client.Get(context.Background(), tc.expectedWebhook.Name, metav1.GetOptions{})
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(gotWebhook).To(Equal(tc.expectedWebhook))
+		})
+	}
+}
+
 func Test_ensureDependecyAnnotations(t *testing.T) {
 	cases := []struct {
 		name string