diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 0040c2adc1..0674b55da3 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -128,8 +128,11 @@ 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 + webhookScope = admissionregistrationv1.NamespacedScope + webhookReinvocationPolicy = admissionregistrationv1.IfNeededReinvocationPolicy ) func getInfra() (*osconfigv1.Infrastructure, error) { @@ -301,6 +304,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 +328,7 @@ func MachineSetValidatingWebhook() admissionregistrationv1.ValidatingWebhook { Name: "validation.machineset.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machinesetServiceReference, }, @@ -333,6 +338,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 +383,8 @@ func MachineMutatingWebhook() admissionregistrationv1.MutatingWebhook { Name: "default.machine.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, + ReinvocationPolicy: &webhookReinvocationPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machineServiceReference, }, @@ -386,6 +394,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 +417,8 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook { Name: "default.machineset.machine.openshift.io", FailurePolicy: &webhookFailurePolicy, SideEffects: &webhookSideEffects, + MatchPolicy: &webhookMatchPolicy, + ReinvocationPolicy: &webhookReinvocationPolicy, ClientConfig: admissionregistrationv1.WebhookClientConfig{ Service: &machineSetServiceReference, }, @@ -417,6 +428,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" } 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.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 b9984a7703..1f7f52e05b 100644 --- a/pkg/operator/sync_test.go +++ b/pkg/operator/sync_test.go @@ -2,6 +2,7 @@ package operator import ( "context" + "errors" "fmt" "testing" "time" @@ -203,6 +204,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 +393,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 +479,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..a595e6b510 100644 --- a/pkg/operator/webhook_configuration.go +++ b/pkg/operator/webhook_configuration.go @@ -2,14 +2,15 @@ package operator import ( "context" + "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" ) @@ -21,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) { + if requiredOriginal == nil { + return nil, false, fmt.Errorf("Unexpected nil instead of an object") + } + required := requiredOriginal.DeepCopy() - gvr := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations") - resourcedClient := client.Resource(gvr) + 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 + } // 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 err := copyMutatingWebhookCABundle(resourcedClient, required); err != nil { - return nil, false, err - } + copyMutatingWebhookCABundle(existing, required) } - // 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} + modified := resourcemerge.BoolPtr(false) + existingCopy := existing.DeepCopy() - actualUnstr, modified, err := applyUnstructured(resourcedClient, "webhooks", recorder, requiredUnstr, expectedGeneration) - if err != nil { - return nil, modified, 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 - actual := &admissionregistrationv1.MutatingWebhookConfiguration{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(actualUnstr.Object, actual); err != nil { - return nil, modified, err - } - return actual, modified, nil -} + klog.V(4).Infof("MutatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) -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 + actual, err := client.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) + if err != nil { + return nil, false, err } - - existing := &admissionregistrationv1.MutatingWebhookConfiguration{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(existingUnstr.Object, existing); err != nil { - return 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 @@ -82,7 +87,6 @@ func copyMutatingWebhookCABundle(resourceClient dynamic.ResourceInterface, requi webhooks[i] = mutatingWebhook } required.Webhooks = webhooks - return nil } // applyValidatingWebhookConfiguration ensures the form of the specified @@ -90,89 +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.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorLabel] != "" { - if err := copyValidatingWebhookCABundle(resourcedClient, required); err != nil { - return nil, false, err - } + if requiredOriginal == nil { + return nil, false, fmt.Errorf("Unexpected nil instead of an object") } - - // 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) { - required := requiredOriginal.DeepCopy() - if err := setSpecHashAnnotation(required, required.Object[path]); err != nil { - return nil, false, err - } - - 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 @@ -182,39 +115,48 @@ func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, re return nil, false, err } - existingCopy := existing.DeepCopy() - modified, err := ensureObjectMeta(existingCopy, required) - if err != nil { - 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) } - if !modified { - return existingCopy, false, nil - } + modified := resourcemerge.BoolPtr(false) + existingCopy := existing.DeepCopy() - requiredSpec, exists, err := unstructured.NestedFieldNoCopy(required.Object, path) - if err != nil { - return nil, false, err - } - if !exists { - // No spec to update + 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 - if err := unstructured.SetNestedField(toWrite.Object, requiredSpec, path); err != nil { + toWrite.Webhooks = required.Webhooks + + klog.V(4).Infof("ValidatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) + + 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 +} - if klog.V(4) { - klog.Infof("%s %q changes: %v", required.GetKind(), required.GetNamespace()+"/"+required.GetName(), jsonPatchNoError(existing, toWrite)) +func copyValidatingWebhookCABundle(existing, required *admissionregistrationv1.ValidatingWebhookConfiguration) { + existingMutatingWebhooks := make(map[string]admissionregistrationv1.ValidatingWebhook) + for _, validatingWebhook := range existing.Webhooks { + existingMutatingWebhooks[validatingWebhook.Name] = validatingWebhook } - actual, err := resourceClient.Update(context.TODO(), toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) - if err != nil { - return nil, false, err + 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 } - return actual, true, nil + required.Webhooks = webhooks } diff --git a/pkg/operator/webhook_configuration_helper.go b/pkg/operator/webhook_configuration_helper.go index c27cd1e4ad..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, error) { - 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, nil -} - -// 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 { 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