From 56c279cd2612856048c9dbf7a9dafe903995fe7f Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Tue, 8 Sep 2020 17:53:31 +0200 Subject: [PATCH 1/4] Refactor applyUnstructured and add/refactor tests --- pkg/operator/suite_test.go | 42 +++++ pkg/operator/sync_test.go | 148 +++++++++++++++++- pkg/operator/webhook_configuration.go | 38 ++--- pkg/operator/webhook_configuration_helper.go | 4 +- .../events/eventstesting/recorder_testing.go | 48 ++++++ .../events/eventstesting/recorder_wrapper.go | 53 +++++++ vendor/modules.txt | 1 + 7 files changed, 313 insertions(+), 21 deletions(-) create mode 100644 pkg/operator/suite_test.go create mode 100644 vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_testing.go create mode 100644 vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_wrapper.go diff --git a/pkg/operator/suite_test.go b/pkg/operator/suite_test.go new file mode 100644 index 0000000000..4110fe26d5 --- /dev/null +++ b/pkg/operator/suite_test.go @@ -0,0 +1,42 @@ +package operator + +import ( + "path/filepath" + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/envtest/printer" +) + +var ( + cfg *rest.Config + testEnv *envtest.Environment +) + +func TestMachinesetController(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecsWithDefaultAndCustomReporters(t, + "Operator Suite", + []Reporter{printer.NewlineReporter{}}) +} + +var _ = BeforeSuite(func() { + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "install")}, + } + + var err error + cfg, err = testEnv.Start() + Expect(err).ToNot(HaveOccurred()) + Expect(cfg).ToNot(BeNil()) +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + Expect(testEnv.Stop()).To(Succeed()) +}) diff --git a/pkg/operator/sync_test.go b/pkg/operator/sync_test.go index b9984a7703..ad4ca045aa 100644 --- a/pkg/operator/sync_test.go +++ b/pkg/operator/sync_test.go @@ -2,10 +2,12 @@ package operator import ( "context" + "errors" "fmt" "testing" "time" + "github.com/openshift/library-go/pkg/operator/events/eventstesting" mapiv1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" @@ -203,6 +205,18 @@ func TestSyncValidatingWebhooks(t *testing.T) { }, shouldSync: true, }, + { + testCase: "It should update webhookConfiguration if its some webhook has a change in a field populated by defaults", + exisingWebhook: func() *unstructured.Unstructured { + webhook := defaultConfiguration.DeepCopy() + // Set a non default policy + policy := admissionregistrationv1.Exact + webhook.Webhooks[0].MatchPolicy = &policy + exisingWebhook, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(webhook) + return &unstructured.Unstructured{Object: exisingWebhook} + }, + shouldSync: true, + }, { testCase: "It shoud update webhookConfiguration if some webhooks are removed from the list", exisingWebhook: func() *unstructured.Unstructured { @@ -380,7 +394,7 @@ func testSyncWebhookConfiguration( clientIn := fakedynamic.NewSimpleDynamicClient(scheme.Scheme, []runtime.Object{}...) optr.dynamicClient = clientIn - serverError := fmt.Errorf("Server error") + serverError := errors.New("Server error") addReactor := func(operation string) { reactor := func(action clientTesting.Action) (bool, runtime.Object, error) { return true, &admissionregistrationv1.ValidatingWebhookConfiguration{}, serverError @@ -466,6 +480,138 @@ func testSyncWebhookConfiguration( } } +func TestApplyUnstructured(t *testing.T) { + testResource := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations") + + cases := []struct { + name string + path string + existing runtime.Object + object func() *unstructured.Unstructured + expectModified bool + serverError string + err error + }{ + { + name: "Successfully create an object", + path: "webhooks", + object: func() *unstructured.Unstructured { + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) + if err != nil { + t.Fatalf("Failed to covnert resource to unstructured: %v", err) + } + return &unstructured.Unstructured{Object: obj} + }, + expectModified: true, + }, + { + name: "Successfully update an object", + path: "webhooks", + object: func() *unstructured.Unstructured { + resource := mapiv1.NewMutatingWebhookConfiguration() + resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0]) + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) + if err != nil { + t.Fatalf("Failed to covnert resource to unstructured: %v", err) + } + return &unstructured.Unstructured{Object: obj} + }, + existing: mapiv1.NewMutatingWebhookConfiguration(), + expectModified: true, + }, + { + name: "Fail to apply a nil object", + object: func() *unstructured.Unstructured { return nil }, + err: errors.New("Unexpected nil instead of an object"), + }, + { + name: "Fail to apply an empty object", + path: "webhooks", + object: func() *unstructured.Unstructured { return &unstructured.Unstructured{} }, + err: errors.New("Object does not contain the specified path: webhooks"), + }, + { + name: "Fail to create an object in case of a server error on 'get' requests", + path: "webhooks", + object: func() *unstructured.Unstructured { + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) + if err != nil { + t.Fatalf("Failed to covnert resource to unstructured: %v", err) + } + return &unstructured.Unstructured{Object: obj} + }, + serverError: "get", + err: errors.New("Custom server error"), + }, + { + name: "Fail to create an object in case of a server error on 'create' requests", + path: "webhooks", + object: func() *unstructured.Unstructured { + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) + if err != nil { + t.Fatalf("Failed to covnert resource to unstructured: %v", err) + } + return &unstructured.Unstructured{Object: obj} + }, + serverError: "create", + err: errors.New("Custom server error"), + }, + { + name: "Fail to update an object in case of a server error on 'create' requests", + path: "webhooks", + object: func() *unstructured.Unstructured { + resource := mapiv1.NewMutatingWebhookConfiguration() + resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0]) + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) + if err != nil { + t.Fatalf("Failed to covnert resource to unstructured: %v", err) + } + return &unstructured.Unstructured{Object: obj} + }, + existing: mapiv1.NewMutatingWebhookConfiguration(), + serverError: "update", + err: errors.New("Custom server error"), + }, + } + + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + var objects []runtime.Object + if test.existing != nil { + objects = append(objects, test.existing) + } + clientIn := fakedynamic.NewSimpleDynamicClient(scheme.Scheme, objects...) + client := clientIn.Resource(testResource) + if test.serverError != "" { + reactor := func(action clientTesting.Action) (bool, runtime.Object, error) { + return true, nil, test.err + } + clientIn.PrependReactor(test.serverError, "*", reactor) + } + + applied, modified, err := applyUnstructured(client, test.path, eventstesting.NewTestingEventRecorder(t), test.object(), 0) + if test.err != nil { + if err == nil || test.err.Error() != err.Error() { + t.Fatalf("Expected error to be equal %v, got %v", test.err, err) + } + return + } + + if test.expectModified != modified { + t.Errorf("Expected modified to be %v, got %v", test.expectModified, modified) + return + } + + expectedChange := test.err == nil + if expectedChange && equality.Semantic.DeepEqual(applied, test.object) { + t.Error("Expected object to be updated") + } else if !expectedChange && !equality.Semantic.DeepEqual(applied, test.object) { + t.Error("Expected object to not be updated") + } + }) + } +} + func Test_ensureDependecyAnnotations(t *testing.T) { cases := []struct { name string diff --git a/pkg/operator/webhook_configuration.go b/pkg/operator/webhook_configuration.go index 63b2295006..7bee44cbf1 100644 --- a/pkg/operator/webhook_configuration.go +++ b/pkg/operator/webhook_configuration.go @@ -2,6 +2,7 @@ package operator import ( "context" + "fmt" "github.com/openshift/library-go/pkg/operator/events" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" @@ -28,10 +29,10 @@ func applyMutatingWebhookConfiguration(client dynamic.Interface, recorder events gvr := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations") resourcedClient := client.Resource(gvr) + required := requiredOriginal.DeepCopy() // Providing upgrade compatibility with service-ca-bundle operator // and ignore clientConfig.caBundle changes on "inject-cabundle" label - required := requiredOriginal.DeepCopy() - if required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { + if required != nil && required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { if err := copyMutatingWebhookCABundle(resourcedClient, required); err != nil { return nil, false, err } @@ -100,7 +101,7 @@ func applyValidatingWebhookConfiguration(client dynamic.Interface, recorder even required := requiredOriginal.DeepCopy() // Providing upgrade compatibility with service-ca-bundle operator // and ignore clientConfig.caBundle changes on "inject-cabundle" label - if required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { + if required != nil && required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { if err := copyValidatingWebhookCABundle(resourcedClient, required); err != nil { return nil, false, err } @@ -162,10 +163,20 @@ func copyValidatingWebhookCABundle(resourceClient dynamic.ResourceInterface, req // For further detail, check the top-level comment. func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, recorder events.Recorder, requiredOriginal *unstructured.Unstructured, expectedGeneration int64) (*unstructured.Unstructured, bool, error) { - + if requiredOriginal == nil { + return nil, false, fmt.Errorf("Unexpected nil instead of an object") + } required := requiredOriginal.DeepCopy() - if err := setSpecHashAnnotation(required, required.Object[path]); err != nil { + requiredSpec, exists, err := unstructured.NestedFieldNoCopy(required.Object, path) + if err != nil { + return nil, false, err + } + if !exists { + return nil, false, fmt.Errorf("Object does not contain the specified path: %s", path) + } + + if err := setSpecHashAnnotation(required, requiredSpec); err != nil { return nil, false, err } @@ -183,21 +194,14 @@ func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, re } existingCopy := existing.DeepCopy() - modified, err := ensureObjectMeta(existingCopy, required) + existingSpec, _, err := unstructured.NestedFieldNoCopy(existingCopy.Object, path) if err != nil { return nil, false, err } - - if !modified { - return existingCopy, false, nil - } - - requiredSpec, exists, err := unstructured.NestedFieldNoCopy(required.Object, path) - if err != nil { + if err := setSpecHashAnnotation(existingCopy, existingSpec); err != nil { return nil, false, err } - if !exists { - // No spec to update + if modified := ensureObjectMeta(existingCopy, required); !modified { return existingCopy, false, nil } @@ -207,9 +211,7 @@ func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, re return nil, false, err } - if klog.V(4) { - klog.Infof("%s %q changes: %v", required.GetKind(), required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) - } + klog.V(4).Infof("%s %q changes: %v", required.GetKind(), required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) actual, err := resourceClient.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) reportUpdateEvent(recorder, required, err) diff --git a/pkg/operator/webhook_configuration_helper.go b/pkg/operator/webhook_configuration_helper.go index c27cd1e4ad..bde654c494 100644 --- a/pkg/operator/webhook_configuration_helper.go +++ b/pkg/operator/webhook_configuration_helper.go @@ -22,7 +22,7 @@ const specHashAnnotation = "operator.openshift.io/spec-hash" // This is based on resourcemerge.EnsureObjectMeta but uses the metav1.Object interface instead // TODO: Update this to use resourcemerge.EnsureObjectMeta or update resourcemerge.EnsureObjectMeta to use the interface -func ensureObjectMeta(existing, required metav1.Object) (bool, error) { +func ensureObjectMeta(existing, required metav1.Object) bool { modified := resourcemerge.BoolPtr(false) namespace := existing.GetNamespace() @@ -40,7 +40,7 @@ func ensureObjectMeta(existing, required metav1.Object) (bool, error) { existing.SetLabels(labels) existing.SetAnnotations(annotations) - return *modified, nil + return *modified } // setSpecHashAnnotation computes the hash of the provided spec and sets an annotation of the diff --git a/vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_testing.go b/vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_testing.go new file mode 100644 index 0000000000..b86fd35af5 --- /dev/null +++ b/vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_testing.go @@ -0,0 +1,48 @@ +package eventstesting + +import ( + "fmt" + "testing" + + "github.com/openshift/library-go/pkg/operator/events" +) + +type TestingEventRecorder struct { + t *testing.T + component string +} + +// NewTestingEventRecorder provides event recorder that will log all recorded events to the error log. +func NewTestingEventRecorder(t *testing.T) events.Recorder { + return &TestingEventRecorder{t: t, component: "test"} +} + +func (r *TestingEventRecorder) ComponentName() string { + return r.component +} + +func (r *TestingEventRecorder) ForComponent(c string) events.Recorder { + return &TestingEventRecorder{t: r.t, component: c} +} + +func (r *TestingEventRecorder) Shutdown() {} + +func (r *TestingEventRecorder) WithComponentSuffix(suffix string) events.Recorder { + return r.ForComponent(fmt.Sprintf("%s-%s", r.ComponentName(), suffix)) +} + +func (r *TestingEventRecorder) Event(reason, message string) { + r.t.Logf("Event: %v: %v", reason, message) +} + +func (r *TestingEventRecorder) Eventf(reason, messageFmt string, args ...interface{}) { + r.Event(reason, fmt.Sprintf(messageFmt, args...)) +} + +func (r *TestingEventRecorder) Warning(reason, message string) { + r.t.Logf("Warning: %v: %v", reason, message) +} + +func (r *TestingEventRecorder) Warningf(reason, messageFmt string, args ...interface{}) { + r.Warning(reason, fmt.Sprintf(messageFmt, args...)) +} diff --git a/vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_wrapper.go b/vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_wrapper.go new file mode 100644 index 0000000000..97e5ec340b --- /dev/null +++ b/vendor/github.com/openshift/library-go/pkg/operator/events/eventstesting/recorder_wrapper.go @@ -0,0 +1,53 @@ +package eventstesting + +import ( + "testing" + + "github.com/openshift/library-go/pkg/operator/events" +) + +type EventRecorder struct { + realEventRecorder events.Recorder + testingEventRecorder *TestingEventRecorder +} + +func NewEventRecorder(t *testing.T, r events.Recorder) events.Recorder { + return &EventRecorder{ + testingEventRecorder: NewTestingEventRecorder(t).(*TestingEventRecorder), + realEventRecorder: r, + } +} + +func (e *EventRecorder) Event(reason, message string) { + e.realEventRecorder.Event(reason, message) + e.testingEventRecorder.Event(reason, message) +} + +func (e *EventRecorder) Shutdown() {} + +func (e *EventRecorder) Eventf(reason, messageFmt string, args ...interface{}) { + e.realEventRecorder.Eventf(reason, messageFmt, args...) + e.testingEventRecorder.Eventf(reason, messageFmt, args...) +} + +func (e *EventRecorder) Warning(reason, message string) { + e.realEventRecorder.Warning(reason, message) + e.testingEventRecorder.Warning(reason, message) +} + +func (e *EventRecorder) Warningf(reason, messageFmt string, args ...interface{}) { + e.realEventRecorder.Warningf(reason, messageFmt, args...) + e.testingEventRecorder.Warningf(reason, messageFmt, args...) +} + +func (e *EventRecorder) ForComponent(componentName string) events.Recorder { + return e +} + +func (e *EventRecorder) WithComponentSuffix(componentNameSuffix string) events.Recorder { + return e +} + +func (e *EventRecorder) ComponentName() string { + return "test-recorder" +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 1f847b71fb..8d918257c3 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -216,6 +216,7 @@ github.com/openshift/cluster-api-provider-gcp/pkg/apis/gcpprovider/v1beta1 # github.com/openshift/library-go v0.0.0-20200909173121-1d055d971916 github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers github.com/openshift/library-go/pkg/operator/events +github.com/openshift/library-go/pkg/operator/events/eventstesting github.com/openshift/library-go/pkg/operator/resource/resourceapply github.com/openshift/library-go/pkg/operator/resource/resourcehash github.com/openshift/library-go/pkg/operator/resource/resourcehelper From 0326bca13d741df30bc73b56bd173b85ec056ccb Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Wed, 23 Sep 2020 15:14:48 +0200 Subject: [PATCH 2/4] Enforce values for MachineWebhooks, which should be strictly set by operator Prevent user from setting malitious values in the rest of the webhook configuration fields, which could cause major diviations in mutation/validation procedure. --- pkg/apis/machine/v1beta1/machine_webhook.go | 26 +++++++++++++++++++-- pkg/operator/operator.go | 2 +- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 0040c2adc1..46a874fc19 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -128,8 +128,12 @@ const ( var ( // webhookFailurePolicy is ignore so we don't want to block machine lifecycle on the webhook operational aspects. // This would be particularly problematic for chicken egg issues when bootstrapping a cluster. - webhookFailurePolicy = admissionregistrationv1.Ignore - webhookSideEffects = admissionregistrationv1.SideEffectClassNone + webhookFailurePolicy = admissionregistrationv1.Ignore + webhookSideEffects = admissionregistrationv1.SideEffectClassNone + webhookMatchPolicy = admissionregistrationv1.Equivalent + webhookResourceSelector = metav1.LabelSelector{} + webhookScope = admissionregistrationv1.NamespacedScope + webhookReinvocationPolicy = admissionregistrationv1.IfNeededReinvocationPolicy ) func getInfra() (*osconfigv1.Infrastructure, error) { @@ -292,6 +296,9 @@ func MachineValidatingWebhook() admissionregistrationv1.ValidatingWebhook { Name: "validation.machine.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, + NamespaceSelector: &webhookResourceSelector, + ObjectSelector: &webhookResourceSelector, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &serviceReference, }, @@ -301,6 +308,7 @@ func MachineValidatingWebhook() admissionregistrationv1.ValidatingWebhook { APIGroups: []string{machine.GroupName}, APIVersions: []string{SchemeGroupVersion.Version}, Resources: []string{"machines"}, + Scope: &webhookScope, }, Operations: []admissionregistrationv1.OperationType{ admissionregistrationv1.Create, @@ -324,6 +332,9 @@ func MachineSetValidatingWebhook() admissionregistrationv1.ValidatingWebhook { Name: "validation.machineset.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, + NamespaceSelector: &webhookResourceSelector, + ObjectSelector: &webhookResourceSelector, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machinesetServiceReference, }, @@ -333,6 +344,7 @@ func MachineSetValidatingWebhook() admissionregistrationv1.ValidatingWebhook { APIGroups: []string{machine.GroupName}, APIVersions: []string{SchemeGroupVersion.Version}, Resources: []string{"machinesets"}, + Scope: &webhookScope, }, Operations: []admissionregistrationv1.OperationType{ admissionregistrationv1.Create, @@ -377,6 +389,10 @@ func MachineMutatingWebhook() admissionregistrationv1.MutatingWebhook { Name: "default.machine.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, + NamespaceSelector: &webhookResourceSelector, + ObjectSelector: &webhookResourceSelector, + ReinvocationPolicy: &webhookReinvocationPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machineServiceReference, }, @@ -386,6 +402,7 @@ func MachineMutatingWebhook() admissionregistrationv1.MutatingWebhook { APIGroups: []string{machine.GroupName}, APIVersions: []string{SchemeGroupVersion.Version}, Resources: []string{"machines"}, + Scope: &webhookScope, }, Operations: []admissionregistrationv1.OperationType{ admissionregistrationv1.Create, @@ -408,6 +425,10 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook { Name: "default.machineset.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, + NamespaceSelector: &webhookResourceSelector, + ObjectSelector: &webhookResourceSelector, + ReinvocationPolicy: &webhookReinvocationPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machineSetServiceReference, }, @@ -417,6 +438,7 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook { APIGroups: []string{machine.GroupName}, APIVersions: []string{SchemeGroupVersion.Version}, Resources: []string{"machinesets"}, + Scope: &webhookScope, }, Operations: []admissionregistrationv1.OperationType{ admissionregistrationv1.Create, diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 0f63f94507..86fef3b4fd 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -260,7 +260,7 @@ func isMachineWebhook(obj interface{}) bool { return mutatingWebhook.Name == "machine-api" } - validatingWebhook, ok := obj.(*admissionregistrationv1.MutatingWebhookConfiguration) + validatingWebhook, ok := obj.(*admissionregistrationv1.ValidatingWebhookConfiguration) if ok { return validatingWebhook.Name == "machine-api" } From 7f73f9029a53038c6f7657955b5ffbe38cd3171d Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 24 Sep 2020 13:07:27 +0200 Subject: [PATCH 3/4] Apply webhook after generation change --- pkg/apis/machine/v1beta1/machine_webhook.go | 10 --------- pkg/operator/webhook_configuration.go | 25 ++++++++------------- 2 files changed, 9 insertions(+), 26 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 46a874fc19..0674b55da3 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -131,7 +131,6 @@ var ( webhookFailurePolicy = admissionregistrationv1.Ignore webhookSideEffects = admissionregistrationv1.SideEffectClassNone webhookMatchPolicy = admissionregistrationv1.Equivalent - webhookResourceSelector = metav1.LabelSelector{} webhookScope = admissionregistrationv1.NamespacedScope webhookReinvocationPolicy = admissionregistrationv1.IfNeededReinvocationPolicy ) @@ -296,9 +295,6 @@ func MachineValidatingWebhook() admissionregistrationv1.ValidatingWebhook { Name: "validation.machine.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, - MatchPolicy: &webhookMatchPolicy, - NamespaceSelector: &webhookResourceSelector, - ObjectSelector: &webhookResourceSelector, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &serviceReference, }, @@ -333,8 +329,6 @@ func MachineSetValidatingWebhook() admissionregistrationv1.ValidatingWebhook { FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, MatchPolicy: &webhookMatchPolicy, - NamespaceSelector: &webhookResourceSelector, - ObjectSelector: &webhookResourceSelector, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machinesetServiceReference, }, @@ -390,8 +384,6 @@ func MachineMutatingWebhook() admissionregistrationv1.MutatingWebhook { FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, MatchPolicy: &webhookMatchPolicy, - NamespaceSelector: &webhookResourceSelector, - ObjectSelector: &webhookResourceSelector, ReinvocationPolicy: &webhookReinvocationPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machineServiceReference, @@ -426,8 +418,6 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook { FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, MatchPolicy: &webhookMatchPolicy, - NamespaceSelector: &webhookResourceSelector, - ObjectSelector: &webhookResourceSelector, ReinvocationPolicy: &webhookReinvocationPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machineSetServiceReference, diff --git a/pkg/operator/webhook_configuration.go b/pkg/operator/webhook_configuration.go index 7bee44cbf1..89fc513908 100644 --- a/pkg/operator/webhook_configuration.go +++ b/pkg/operator/webhook_configuration.go @@ -176,10 +176,6 @@ func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, re return nil, false, fmt.Errorf("Object does not contain the specified path: %s", path) } - if err := setSpecHashAnnotation(required, requiredSpec); err != nil { - return nil, false, err - } - existing, err := resourceClient.Get(context.TODO(), required.GetName(), metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { @@ -194,29 +190,26 @@ func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, re } existingCopy := existing.DeepCopy() - existingSpec, _, err := unstructured.NestedFieldNoCopy(existingCopy.Object, path) - if err != nil { - return nil, false, err + modified := ensureObjectMeta(existingCopy, required) + if !modified && existingCopy.GetGeneration() == expectedGeneration { + return existingCopy, false, nil } - if err := setSpecHashAnnotation(existingCopy, existingSpec); err != nil { + if err := unstructured.SetNestedField(existingCopy.Object, requiredSpec, path); err != nil { return nil, false, err } - if modified := ensureObjectMeta(existingCopy, required); !modified { - return existingCopy, false, nil - } // at this point we know that we're going to perform a write. We're just trying to get the object correct toWrite := existingCopy // shallow copy so the code reads easier - if err := unstructured.SetNestedField(toWrite.Object, requiredSpec, path); err != nil { - return nil, false, err - } klog.V(4).Infof("%s %q changes: %v", required.GetKind(), required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) actual, err := resourceClient.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } - return actual, true, nil + if actual.GetGeneration() > expectedGeneration || modified { + reportUpdateEvent(recorder, required, err) + return actual, true, nil + } + return actual, false, nil } From 455cee3cbce54a44fd1d5262899f3adf9d421795 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 24 Sep 2020 15:29:43 +0200 Subject: [PATCH 4/4] Make webhook apply typed and add tests --- pkg/operator/sync.go | 4 +- pkg/operator/sync_test.go | 263 +++++++++---------- pkg/operator/webhook_configuration.go | 203 ++++++-------- pkg/operator/webhook_configuration_helper.go | 47 ---- 4 files changed, 208 insertions(+), 309 deletions(-) diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 990bf35f01..bc1a4bd64a 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -168,7 +168,7 @@ func (optr *Operator) syncWebhookConfiguration() error { func (optr *Operator) syncValidatingWebhook() error { expected := mapiv1.NewValidatingWebhookConfiguration() if webhook, updated, err := applyValidatingWebhookConfiguration( - optr.dynamicClient, + optr.kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations(), events.NewLoggingEventRecorder(optr.name), expected, expectedValidatingWebhooksConfiguration(expected.Name, optr.generations), @@ -184,7 +184,7 @@ func (optr *Operator) syncValidatingWebhook() error { func (optr *Operator) syncMutatingWebhook() error { expected := mapiv1.NewMutatingWebhookConfiguration() if webhook, updated, err := applyMutatingWebhookConfiguration( - optr.dynamicClient, + optr.kubeClient.AdmissionregistrationV1().MutatingWebhookConfigurations(), events.NewLoggingEventRecorder(optr.name), expected, expectedMutatingWebhooksConfiguration(expected.Name, optr.generations), diff --git a/pkg/operator/sync_test.go b/pkg/operator/sync_test.go index ad4ca045aa..1f7f52e05b 100644 --- a/pkg/operator/sync_test.go +++ b/pkg/operator/sync_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/openshift/library-go/pkg/operator/events/eventstesting" mapiv1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" @@ -480,137 +479,137 @@ func testSyncWebhookConfiguration( } } -func TestApplyUnstructured(t *testing.T) { - testResource := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations") - - cases := []struct { - name string - path string - existing runtime.Object - object func() *unstructured.Unstructured - expectModified bool - serverError string - err error - }{ - { - name: "Successfully create an object", - path: "webhooks", - object: func() *unstructured.Unstructured { - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) - if err != nil { - t.Fatalf("Failed to covnert resource to unstructured: %v", err) - } - return &unstructured.Unstructured{Object: obj} - }, - expectModified: true, - }, - { - name: "Successfully update an object", - path: "webhooks", - object: func() *unstructured.Unstructured { - resource := mapiv1.NewMutatingWebhookConfiguration() - resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0]) - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) - if err != nil { - t.Fatalf("Failed to covnert resource to unstructured: %v", err) - } - return &unstructured.Unstructured{Object: obj} - }, - existing: mapiv1.NewMutatingWebhookConfiguration(), - expectModified: true, - }, - { - name: "Fail to apply a nil object", - object: func() *unstructured.Unstructured { return nil }, - err: errors.New("Unexpected nil instead of an object"), - }, - { - name: "Fail to apply an empty object", - path: "webhooks", - object: func() *unstructured.Unstructured { return &unstructured.Unstructured{} }, - err: errors.New("Object does not contain the specified path: webhooks"), - }, - { - name: "Fail to create an object in case of a server error on 'get' requests", - path: "webhooks", - object: func() *unstructured.Unstructured { - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) - if err != nil { - t.Fatalf("Failed to covnert resource to unstructured: %v", err) - } - return &unstructured.Unstructured{Object: obj} - }, - serverError: "get", - err: errors.New("Custom server error"), - }, - { - name: "Fail to create an object in case of a server error on 'create' requests", - path: "webhooks", - object: func() *unstructured.Unstructured { - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) - if err != nil { - t.Fatalf("Failed to covnert resource to unstructured: %v", err) - } - return &unstructured.Unstructured{Object: obj} - }, - serverError: "create", - err: errors.New("Custom server error"), - }, - { - name: "Fail to update an object in case of a server error on 'create' requests", - path: "webhooks", - object: func() *unstructured.Unstructured { - resource := mapiv1.NewMutatingWebhookConfiguration() - resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0]) - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) - if err != nil { - t.Fatalf("Failed to covnert resource to unstructured: %v", err) - } - return &unstructured.Unstructured{Object: obj} - }, - existing: mapiv1.NewMutatingWebhookConfiguration(), - serverError: "update", - err: errors.New("Custom server error"), - }, - } - - for _, test := range cases { - t.Run(test.name, func(t *testing.T) { - var objects []runtime.Object - if test.existing != nil { - objects = append(objects, test.existing) - } - clientIn := fakedynamic.NewSimpleDynamicClient(scheme.Scheme, objects...) - client := clientIn.Resource(testResource) - if test.serverError != "" { - reactor := func(action clientTesting.Action) (bool, runtime.Object, error) { - return true, nil, test.err - } - clientIn.PrependReactor(test.serverError, "*", reactor) - } - - applied, modified, err := applyUnstructured(client, test.path, eventstesting.NewTestingEventRecorder(t), test.object(), 0) - if test.err != nil { - if err == nil || test.err.Error() != err.Error() { - t.Fatalf("Expected error to be equal %v, got %v", test.err, err) - } - return - } - - if test.expectModified != modified { - t.Errorf("Expected modified to be %v, got %v", test.expectModified, modified) - return - } - - expectedChange := test.err == nil - if expectedChange && equality.Semantic.DeepEqual(applied, test.object) { - t.Error("Expected object to be updated") - } else if !expectedChange && !equality.Semantic.DeepEqual(applied, test.object) { - t.Error("Expected object to not be updated") - } - }) - } -} +// func TestApplyUnstructured(t *testing.T) { +// testResource := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations") + +// cases := []struct { +// name string +// path string +// existing runtime.Object +// object func() *unstructured.Unstructured +// expectModified bool +// serverError string +// err error +// }{ +// { +// name: "Successfully create an object", +// path: "webhooks", +// object: func() *unstructured.Unstructured { +// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) +// if err != nil { +// t.Fatalf("Failed to covnert resource to unstructured: %v", err) +// } +// return &unstructured.Unstructured{Object: obj} +// }, +// expectModified: true, +// }, +// { +// name: "Successfully update an object", +// path: "webhooks", +// object: func() *unstructured.Unstructured { +// resource := mapiv1.NewMutatingWebhookConfiguration() +// resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0]) +// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) +// if err != nil { +// t.Fatalf("Failed to covnert resource to unstructured: %v", err) +// } +// return &unstructured.Unstructured{Object: obj} +// }, +// existing: mapiv1.NewMutatingWebhookConfiguration(), +// expectModified: true, +// }, +// { +// name: "Fail to apply a nil object", +// object: func() *unstructured.Unstructured { return nil }, +// err: errors.New("Unexpected nil instead of an object"), +// }, +// { +// name: "Fail to apply an empty object", +// path: "webhooks", +// object: func() *unstructured.Unstructured { return &unstructured.Unstructured{} }, +// err: errors.New("Object does not contain the specified path: webhooks"), +// }, +// { +// name: "Fail to create an object in case of a server error on 'get' requests", +// path: "webhooks", +// object: func() *unstructured.Unstructured { +// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) +// if err != nil { +// t.Fatalf("Failed to covnert resource to unstructured: %v", err) +// } +// return &unstructured.Unstructured{Object: obj} +// }, +// serverError: "get", +// err: errors.New("Custom server error"), +// }, +// { +// name: "Fail to create an object in case of a server error on 'create' requests", +// path: "webhooks", +// object: func() *unstructured.Unstructured { +// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration()) +// if err != nil { +// t.Fatalf("Failed to covnert resource to unstructured: %v", err) +// } +// return &unstructured.Unstructured{Object: obj} +// }, +// serverError: "create", +// err: errors.New("Custom server error"), +// }, +// { +// name: "Fail to update an object in case of a server error on 'create' requests", +// path: "webhooks", +// object: func() *unstructured.Unstructured { +// resource := mapiv1.NewMutatingWebhookConfiguration() +// resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0]) +// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) +// if err != nil { +// t.Fatalf("Failed to covnert resource to unstructured: %v", err) +// } +// return &unstructured.Unstructured{Object: obj} +// }, +// existing: mapiv1.NewMutatingWebhookConfiguration(), +// serverError: "update", +// err: errors.New("Custom server error"), +// }, +// } + +// for _, test := range cases { +// t.Run(test.name, func(t *testing.T) { +// var objects []runtime.Object +// if test.existing != nil { +// objects = append(objects, test.existing) +// } +// clientIn := fakedynamic.NewSimpleDynamicClient(scheme.Scheme, objects...) +// client := clientIn.Resource(testResource) +// if test.serverError != "" { +// reactor := func(action clientTesting.Action) (bool, runtime.Object, error) { +// return true, nil, test.err +// } +// clientIn.PrependReactor(test.serverError, "*", reactor) +// } + +// applied, modified, err := applyUnstructured(client, test.path, eventstesting.NewTestingEventRecorder(t), test.object(), 0) +// if test.err != nil { +// if err == nil || test.err.Error() != err.Error() { +// t.Fatalf("Expected error to be equal %v, got %v", test.err, err) +// } +// return +// } + +// if test.expectModified != modified { +// t.Errorf("Expected modified to be %v, got %v", test.expectModified, modified) +// return +// } + +// expectedChange := test.err == nil +// if expectedChange && equality.Semantic.DeepEqual(applied, test.object) { +// t.Error("Expected object to be updated") +// } else if !expectedChange && !equality.Semantic.DeepEqual(applied, test.object) { +// t.Error("Expected object to not be updated") +// } +// }) +// } +// } func Test_ensureDependecyAnnotations(t *testing.T) { cases := []struct { diff --git a/pkg/operator/webhook_configuration.go b/pkg/operator/webhook_configuration.go index 89fc513908..a595e6b510 100644 --- a/pkg/operator/webhook_configuration.go +++ b/pkg/operator/webhook_configuration.go @@ -5,12 +5,12 @@ import ( "fmt" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/dynamic" + v1 "k8s.io/client-go/kubernetes/typed/admissionregistration/v1" "k8s.io/klog" ) @@ -22,54 +22,58 @@ const genericCABundleInjectorLabel = "service.beta.openshift.io/inject-cabundle" // it will be created. If it does exist, the metadata of the required // mutatingwebhookconfiguration will be merged with the existing mutatingwebhookconfiguration // and an update performed if the mutatingwebhookconfiguration spec and metadata differ from -// the previously required spec and metadata. For further detail, check the top-level comment. -func applyMutatingWebhookConfiguration(client dynamic.Interface, recorder events.Recorder, +// the previously required spec and metadata based on generation change. +func applyMutatingWebhookConfiguration(client v1.MutatingWebhookConfigurationInterface, recorder events.Recorder, requiredOriginal *admissionregistrationv1.MutatingWebhookConfiguration, expectedGeneration int64) (*admissionregistrationv1.MutatingWebhookConfiguration, bool, error) { - - gvr := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations") - resourcedClient := client.Resource(gvr) - - required := requiredOriginal.DeepCopy() - // Providing upgrade compatibility with service-ca-bundle operator - // and ignore clientConfig.caBundle changes on "inject-cabundle" label - if required != nil && required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { - if err := copyMutatingWebhookCABundle(resourcedClient, required); err != nil { - return nil, false, err - } + if requiredOriginal == nil { + return nil, false, fmt.Errorf("Unexpected nil instead of an object") } + required := requiredOriginal.DeepCopy() - // Explictily specify type for required webhook configuration to get object meta accessor - requiredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(required) + existing, err := client.Get(context.TODO(), required.GetName(), metav1.GetOptions{}) if err != nil { + if apierrors.IsNotFound(err) { + actual, err := client.Create(context.TODO(), required, metav1.CreateOptions{}) + reportCreateEvent(recorder, required, err) + if err != nil { + return nil, false, err + } + return actual, true, nil + } return nil, false, err } - requiredUnstr := &unstructured.Unstructured{Object: requiredObj} - actualUnstr, modified, err := applyUnstructured(resourcedClient, "webhooks", recorder, requiredUnstr, expectedGeneration) - if err != nil { - return nil, modified, err + // Providing upgrade compatibility with service-ca-bundle operator + // and ignore clientConfig.caBundle changes on "inject-cabundle" label + if required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { + copyMutatingWebhookCABundle(existing, required) } - actual := &admissionregistrationv1.MutatingWebhookConfiguration{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(actualUnstr.Object, actual); err != nil { - return nil, modified, err - } - return actual, modified, nil -} + modified := resourcemerge.BoolPtr(false) + existingCopy := existing.DeepCopy() -func copyMutatingWebhookCABundle(resourceClient dynamic.ResourceInterface, required *admissionregistrationv1.MutatingWebhookConfiguration) error { - existingUnstr, err := resourceClient.Get(context.TODO(), required.GetName(), metav1.GetOptions{}) - if apierrors.IsNotFound(err) { - return nil - } else if err != nil { - return err + resourcemerge.EnsureObjectMeta(modified, &existingCopy.ObjectMeta, required.ObjectMeta) + if !*modified && existingCopy.GetGeneration() == expectedGeneration { + return existingCopy, false, nil } + // at this point we know that we're going to perform a write. We're just trying to get the object correct + toWrite := existingCopy // shallow copy so the code reads easier + toWrite.Webhooks = required.Webhooks + + klog.V(4).Infof("MutatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) - existing := &admissionregistrationv1.MutatingWebhookConfiguration{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(existingUnstr.Object, existing); err != nil { - return err + actual, err := client.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) + if err != nil { + return nil, false, err + } + if *modified || actual.GetGeneration() > expectedGeneration { + reportUpdateEvent(recorder, required, err) + return actual, true, nil } + return actual, false, nil +} +func copyMutatingWebhookCABundle(existing, required *admissionregistrationv1.MutatingWebhookConfiguration) { existingMutatingWebhooks := make(map[string]admissionregistrationv1.MutatingWebhook) for _, mutatingWebhook := range existing.Webhooks { existingMutatingWebhooks[mutatingWebhook.Name] = mutatingWebhook @@ -83,7 +87,6 @@ func copyMutatingWebhookCABundle(resourceClient dynamic.ResourceInterface, requi webhooks[i] = mutatingWebhook } required.Webhooks = webhooks - return nil } // applyValidatingWebhookConfiguration ensures the form of the specified @@ -91,95 +94,18 @@ func copyMutatingWebhookCABundle(resourceClient dynamic.ResourceInterface, requi // it will be created. If it does exist, the metadata of the required // validatingwebhookconfiguration will be merged with the existing validatingwebhookconfiguration // and an update performed if the validatingwebhookconfiguration spec and metadata differ from -// the previously required spec and metadata. For further detail, check the top-level comment. -func applyValidatingWebhookConfiguration(client dynamic.Interface, recorder events.Recorder, +// the previously required spec and metadata based on generation change. +func applyValidatingWebhookConfiguration(client v1.ValidatingWebhookConfigurationInterface, recorder events.Recorder, requiredOriginal *admissionregistrationv1.ValidatingWebhookConfiguration, expectedGeneration int64) (*admissionregistrationv1.ValidatingWebhookConfiguration, bool, error) { - - gvr := admissionregistrationv1.SchemeGroupVersion.WithResource("validatingwebhookconfigurations") - resourcedClient := client.Resource(gvr) - - required := requiredOriginal.DeepCopy() - // Providing upgrade compatibility with service-ca-bundle operator - // and ignore clientConfig.caBundle changes on "inject-cabundle" label - if required != nil && required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { - if err := copyValidatingWebhookCABundle(resourcedClient, required); err != nil { - return nil, false, err - } - } - - // Explictily specify type for required webhook configuration to get object meta accessor - requiredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(required) - if err != nil { - return nil, false, err - } - - requiredUnstr := &unstructured.Unstructured{Object: requiredObj} - - actualUnstr, modified, err := applyUnstructured(resourcedClient, "webhooks", recorder, requiredUnstr, expectedGeneration) - if err != nil { - return nil, modified, err - } - - actual := &admissionregistrationv1.ValidatingWebhookConfiguration{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(actualUnstr.Object, actual); err != nil { - return nil, modified, err - } - return actual, modified, nil -} - -func copyValidatingWebhookCABundle(resourceClient dynamic.ResourceInterface, required *admissionregistrationv1.ValidatingWebhookConfiguration) error { - existingUnstr, err := resourceClient.Get(context.TODO(), required.GetName(), metav1.GetOptions{}) - if apierrors.IsNotFound(err) { - return nil - } else if err != nil { - return err - } - - existing := &admissionregistrationv1.ValidatingWebhookConfiguration{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(existingUnstr.Object, existing); err != nil { - return err - } - - existingValidatingWebhooks := make(map[string]admissionregistrationv1.ValidatingWebhook) - for _, validatingWebhook := range existing.Webhooks { - existingValidatingWebhooks[validatingWebhook.Name] = validatingWebhook - } - - webhooks := make([]admissionregistrationv1.ValidatingWebhook, len(required.Webhooks)) - for i, validatingWebhook := range required.Webhooks { - if webhook, ok := existingValidatingWebhooks[validatingWebhook.Name]; ok { - validatingWebhook.ClientConfig.CABundle = webhook.ClientConfig.CABundle - } - webhooks[i] = validatingWebhook - } - required.Webhooks = webhooks - return nil -} - -// applyUnstructured ensures the form of the specified usntructured is present in the API. -// If it does not exist, it will be created. If it does exist, the metadata of the required -// usntructured will be merged with the existing usntructured and an update performed if the -// usntructured spec and metadata differ from the previously required spec and metadata. -// For further detail, check the top-level comment. -func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, recorder events.Recorder, - requiredOriginal *unstructured.Unstructured, expectedGeneration int64) (*unstructured.Unstructured, bool, error) { if requiredOriginal == nil { return nil, false, fmt.Errorf("Unexpected nil instead of an object") } required := requiredOriginal.DeepCopy() - requiredSpec, exists, err := unstructured.NestedFieldNoCopy(required.Object, path) - if err != nil { - return nil, false, err - } - if !exists { - return nil, false, fmt.Errorf("Object does not contain the specified path: %s", path) - } - - existing, err := resourceClient.Get(context.TODO(), required.GetName(), metav1.GetOptions{}) + existing, err := client.Get(context.TODO(), required.GetName(), metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - actual, err := resourceClient.Create(context.TODO(), required, metav1.CreateOptions{}) + actual, err := client.Create(context.TODO(), required, metav1.CreateOptions{}) reportCreateEvent(recorder, required, err) if err != nil { return nil, false, err @@ -189,27 +115,48 @@ func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, re return nil, false, err } + // Providing upgrade compatibility with service-ca-bundle operator + // and ignore clientConfig.caBundle changes on "inject-cabundle" label + if required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { + copyValidatingWebhookCABundle(existing, required) + } + + modified := resourcemerge.BoolPtr(false) existingCopy := existing.DeepCopy() - modified := ensureObjectMeta(existingCopy, required) - if !modified && existingCopy.GetGeneration() == expectedGeneration { + + resourcemerge.EnsureObjectMeta(modified, &existingCopy.ObjectMeta, required.ObjectMeta) + if !*modified && existingCopy.GetGeneration() == expectedGeneration { return existingCopy, false, nil } - if err := unstructured.SetNestedField(existingCopy.Object, requiredSpec, path); err != nil { - return nil, false, err - } - // at this point we know that we're going to perform a write. We're just trying to get the object correct toWrite := existingCopy // shallow copy so the code reads easier + toWrite.Webhooks = required.Webhooks - klog.V(4).Infof("%s %q changes: %v", required.GetKind(), required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) + klog.V(4).Infof("ValidatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) - actual, err := resourceClient.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) + actual, err := client.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) if err != nil { return nil, false, err } - if actual.GetGeneration() > expectedGeneration || modified { + if *modified || actual.GetGeneration() > expectedGeneration { reportUpdateEvent(recorder, required, err) return actual, true, nil } return actual, false, nil } + +func copyValidatingWebhookCABundle(existing, required *admissionregistrationv1.ValidatingWebhookConfiguration) { + existingMutatingWebhooks := make(map[string]admissionregistrationv1.ValidatingWebhook) + for _, validatingWebhook := range existing.Webhooks { + existingMutatingWebhooks[validatingWebhook.Name] = validatingWebhook + } + + webhooks := make([]admissionregistrationv1.ValidatingWebhook, len(required.Webhooks)) + for i, validatingWebhook := range required.Webhooks { + if webhook, ok := existingMutatingWebhooks[validatingWebhook.Name]; ok { + validatingWebhook.ClientConfig.CABundle = webhook.ClientConfig.CABundle + } + webhooks[i] = validatingWebhook + } + required.Webhooks = webhooks +} diff --git a/pkg/operator/webhook_configuration_helper.go b/pkg/operator/webhook_configuration_helper.go index bde654c494..37c5c180cf 100644 --- a/pkg/operator/webhook_configuration_helper.go +++ b/pkg/operator/webhook_configuration_helper.go @@ -1,8 +1,6 @@ package operator import ( - "crypto/sha256" - "encoding/json" "fmt" "strings" @@ -12,56 +10,11 @@ import ( "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" ) -const specHashAnnotation = "operator.openshift.io/spec-hash" - -// This is based on resourcemerge.EnsureObjectMeta but uses the metav1.Object interface instead -// TODO: Update this to use resourcemerge.EnsureObjectMeta or update resourcemerge.EnsureObjectMeta to use the interface -func ensureObjectMeta(existing, required metav1.Object) bool { - modified := resourcemerge.BoolPtr(false) - - namespace := existing.GetNamespace() - name := existing.GetName() - labels := existing.GetLabels() - annotations := existing.GetAnnotations() - - resourcemerge.SetStringIfSet(modified, &namespace, required.GetNamespace()) - resourcemerge.SetStringIfSet(modified, &name, required.GetName()) - resourcemerge.MergeMap(modified, &labels, required.GetLabels()) - resourcemerge.MergeMap(modified, &annotations, required.GetAnnotations()) - - existing.SetNamespace(namespace) - existing.SetName(name) - existing.SetLabels(labels) - existing.SetAnnotations(annotations) - - return *modified -} - -// setSpecHashAnnotation computes the hash of the provided spec and sets an annotation of the -// hash on the provided ObjectMeta. This method is used internally by Apply methods, and -// is exposed to support testing with fake clients that need to know the mutated form of the -// resource resulting from an Apply call. -func setSpecHashAnnotation(objMeta metav1.Object, spec interface{}) error { - jsonBytes, err := json.Marshal(spec) - if err != nil { - return err - } - specHash := fmt.Sprintf("%x", sha256.Sum256(jsonBytes)) - annotations := objMeta.GetAnnotations() - if annotations == nil { - annotations = map[string]string{} - } - annotations[specHashAnnotation] = specHash - objMeta.SetAnnotations(annotations) - return nil -} - func reportCreateEvent(recorder events.Recorder, obj runtime.Object, originalErr error) { gvk := resourcehelper.GuessObjectGroupVersionKind(obj) if originalErr == nil {