From 99821bd429ba0c4ed18fc18d3394511fa240d6c3 Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 21 Jan 2022 10:12:45 -0500 Subject: [PATCH] need to store the passed value to the apply cache for later comparison even if we mutated it for storage --- .../resourceapply/admissionregistration.go | 30 ++++++++----------- .../admissionregistration_test.go | 8 ++--- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/pkg/operator/resource/resourceapply/admissionregistration.go b/pkg/operator/resource/resourceapply/admissionregistration.go index 57b419a56a..fafa39c404 100644 --- a/pkg/operator/resource/resourceapply/admissionregistration.go +++ b/pkg/operator/resource/resourceapply/admissionregistration.go @@ -14,12 +14,6 @@ import ( "k8s.io/klog/v2" ) -// ApplyMutatingWebhookConfiguration applies the resource -func ApplyMutatingWebhookConfiguration(ctx context.Context, client admissionregistrationclientv1.MutatingWebhookConfigurationsGetter, recorder events.Recorder, - requiredOriginal *admissionregistrationv1.MutatingWebhookConfiguration) (*admissionregistrationv1.MutatingWebhookConfiguration, bool, error) { - return ApplyMutatingWebhookConfigurationImproved(ctx, client, recorder, requiredOriginal, noCache) -} - // ApplyMutatingWebhookConfigurationImproved ensures the form of the specified // mutatingwebhookconfiguration is present in the API. If it does not exist, // it will be created. If it does exist, the metadata of the required @@ -42,7 +36,8 @@ func ApplyMutatingWebhookConfigurationImproved(ctx context.Context, client admis if err != nil { return nil, false, err } - cache.UpdateCachedResourceMetadata(required, actual) + // need to store the original so that the early comparison of hashes is done based on the original, not a mutated copy + cache.UpdateCachedResourceMetadata(requiredOriginal, actual) return actual, true, nil } else if err != nil { return nil, false, err @@ -60,7 +55,8 @@ func ApplyMutatingWebhookConfigurationImproved(ctx context.Context, client admis copyMutatingWebhookCABundle(existing, required) webhooksEquivalent := equality.Semantic.DeepEqual(existingCopy.Webhooks, required.Webhooks) if webhooksEquivalent && !*modified { - cache.UpdateCachedResourceMetadata(required, existingCopy) + // need to store the original so that the early comparison of hashes is done based on the original, not a mutated copy + cache.UpdateCachedResourceMetadata(requiredOriginal, existingCopy) 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 @@ -74,7 +70,8 @@ func ApplyMutatingWebhookConfigurationImproved(ctx context.Context, client admis if err != nil { return nil, false, err } - cache.UpdateCachedResourceMetadata(required, actual) + // need to store the original so that the early comparison of hashes is done based on the original, not a mutated copy + cache.UpdateCachedResourceMetadata(requiredOriginal, actual) return actual, true, nil } @@ -93,12 +90,6 @@ func copyMutatingWebhookCABundle(from, to *admissionregistrationv1.MutatingWebho } } -// ApplyValidatingWebhookConfiguration applies the resource -func ApplyValidatingWebhookConfiguration(ctx context.Context, client admissionregistrationclientv1.ValidatingWebhookConfigurationsGetter, recorder events.Recorder, - requiredOriginal *admissionregistrationv1.ValidatingWebhookConfiguration) (*admissionregistrationv1.ValidatingWebhookConfiguration, bool, error) { - return ApplyValidatingWebhookConfigurationImproved(ctx, client, recorder, requiredOriginal, noCache) -} - // ApplyValidatingWebhookConfigurationImproved ensures the form of the specified // validatingwebhookconfiguration is present in the API. If it does not exist, // it will be created. If it does exist, the metadata of the required @@ -120,7 +111,8 @@ func ApplyValidatingWebhookConfigurationImproved(ctx context.Context, client adm if err != nil { return nil, false, err } - cache.UpdateCachedResourceMetadata(required, actual) + // need to store the original so that the early comparison of hashes is done based on the original, not a mutated copy + cache.UpdateCachedResourceMetadata(requiredOriginal, actual) return actual, true, nil } else if err != nil { return nil, false, err @@ -138,7 +130,8 @@ func ApplyValidatingWebhookConfigurationImproved(ctx context.Context, client adm copyValidatingWebhookCABundle(existing, required) webhooksEquivalent := equality.Semantic.DeepEqual(existingCopy.Webhooks, required.Webhooks) if webhooksEquivalent && !*modified { - cache.UpdateCachedResourceMetadata(required, existingCopy) + // need to store the original so that the early comparison of hashes is done based on the original, not a mutated copy + cache.UpdateCachedResourceMetadata(requiredOriginal, existingCopy) 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 @@ -152,7 +145,8 @@ func ApplyValidatingWebhookConfigurationImproved(ctx context.Context, client adm if err != nil { return nil, false, err } - cache.UpdateCachedResourceMetadata(required, actual) + // need to store the original so that the early comparison of hashes is done based on the original, not a mutated copy + cache.UpdateCachedResourceMetadata(requiredOriginal, actual) return actual, true, nil } diff --git a/pkg/operator/resource/resourceapply/admissionregistration_test.go b/pkg/operator/resource/resourceapply/admissionregistration_test.go index 9145ff0859..b8afe66648 100644 --- a/pkg/operator/resource/resourceapply/admissionregistration_test.go +++ b/pkg/operator/resource/resourceapply/admissionregistration_test.go @@ -173,10 +173,10 @@ func TestApplyMutatingConfiguration(t *testing.T) { recorder := events.NewInMemoryRecorder("test") testApply := func(expectModify bool) { - updatedHook, modified, err := ApplyMutatingWebhookConfiguration( + updatedHook, modified, err := ApplyMutatingWebhookConfigurationImproved( context.TODO(), client.AdmissionregistrationV1(), - recorder, test.input()) + recorder, test.input(), noCache) if err != nil { t.Fatal(err) } @@ -357,10 +357,10 @@ func TestApplyValidatingConfiguration(t *testing.T) { recorder := events.NewInMemoryRecorder("test") testApply := func(expectModify bool) { - updatedHook, modified, err := ApplyValidatingWebhookConfiguration( + updatedHook, modified, err := ApplyValidatingWebhookConfigurationImproved( context.TODO(), client.AdmissionregistrationV1(), - recorder, test.input()) + recorder, test.input(), noCache) if err != nil { t.Fatal(err) }