From 864e9b8187dbfa9a9289b6abc3fa7c55011c969b Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 17 Mar 2021 10:56:02 +0000 Subject: [PATCH] Revert use webhookApply implementation from Library Go --- pkg/operator/sync.go | 114 ++++++++++++++++++++++--- pkg/operator/sync_test.go | 173 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 275 insertions(+), 12 deletions(-) diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 74566e715..2ec112e72 100644 --- a/pkg/operator/sync.go +++ b/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) { diff --git a/pkg/operator/sync_test.go b/pkg/operator/sync_test.go index 6fe626c65..fb9e711b8 100644 --- a/pkg/operator/sync_test.go +++ b/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