From b72b0caa4e5ff99f33b1644016787d0f9258ce9c Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Fri, 11 Oct 2024 14:23:00 +0530 Subject: [PATCH] fixup! fixup! fixup! regression: Set resource version for CRs --- .../resource/resourceapply/monitoring.go | 73 +++++-------------- .../resource/resourceapply/monitoring_test.go | 2 +- .../resource/resourcemerge/object_merger.go | 43 +++++++++++ 3 files changed, 63 insertions(+), 55 deletions(-) diff --git a/pkg/operator/resource/resourceapply/monitoring.go b/pkg/operator/resource/resourceapply/monitoring.go index 9481db8e98..55244f66b4 100644 --- a/pkg/operator/resource/resourceapply/monitoring.go +++ b/pkg/operator/resource/resourceapply/monitoring.go @@ -2,14 +2,12 @@ package resourceapply import ( "context" - errorsstdlib "errors" - "fmt" + "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" "k8s.io/apimachinery/pkg/api/equality" "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/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" "k8s.io/klog/v2" @@ -17,8 +15,6 @@ import ( "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" - - "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" ) var alertmanagerGVR = schema.GroupVersionResource{Group: "monitoring.coreos.com", Version: "v1", Resource: "alertmanagers"} @@ -88,7 +84,8 @@ func ApplyUnstructuredResourceImproved( } existing, err := client.Resource(resourceGVR).Namespace(namespace).Get(ctx, name, metav1.GetOptions{}) if errors.IsNotFound(err) { - want, err := client.Resource(resourceGVR).Namespace(namespace).Create(ctx, required, metav1.CreateOptions{}) + var want *unstructured.Unstructured + want, err = client.Resource(resourceGVR).Namespace(namespace).Create(ctx, required, metav1.CreateOptions{}) resourcehelper.ReportCreateEvent(recorder, required, err) cache.UpdateCachedResourceMetadata(required, want) return want, true, err @@ -102,44 +99,11 @@ func ApplyUnstructuredResourceImproved( return existing, false, nil } - // Ensure metadata field is present on the object. existingCopy := existing.DeepCopy() - existingObjectMeta, found, err := unstructured.NestedMap(existingCopy.Object, "metadata") - if err != nil { - return nil, false, err - } - if !found { - return nil, false, errorsstdlib.New(fmt.Sprintf("metadata not found in the existing object: %s/%s", existing.GetNamespace(), existingCopy.GetName())) - } - requiredObjectMeta, found, err := unstructured.NestedMap(required.Object, "metadata") - if err != nil { - return nil, false, err - } - if !found { - return nil, false, errorsstdlib.New(fmt.Sprintf("metadata not found in the required object: %s/%s", required.GetNamespace(), required.GetName())) - } - - // Cast the metadata to the correct type. - var existingObjectMetaTyped, requiredObjectMetaTyped metav1.ObjectMeta - err = runtime.DefaultUnstructuredConverter.FromUnstructured(existingObjectMeta, &existingObjectMetaTyped) - if err != nil { - return nil, false, err - } - err = runtime.DefaultUnstructuredConverter.FromUnstructured(requiredObjectMeta, &requiredObjectMetaTyped) - if err != nil { - return nil, false, err - } - - // Fail-fast if the resource versions differ. - if requiredObjectMetaTyped.ResourceVersion != "" && existingObjectMetaTyped.ResourceVersion != requiredObjectMetaTyped.ResourceVersion { - err = errors.NewConflict(resourceGVR.GroupResource(), name, fmt.Errorf("rejected to update %s %s because the object has been modified: desired/actual ResourceVersion: %v/%v", existing.GetKind(), existing.GetName(), requiredObjectMetaTyped.ResourceVersion, existingObjectMetaTyped.ResourceVersion)) - return nil, false, err - } - // Check if the metadata objects differ. This only checks for selective fields (excluding the resource version, among others). + // Replace and/or merge certain metadata fields. didMetadataModify := ptr.To(false) - resourcemerge.EnsureObjectMeta(didMetadataModify, &existingObjectMetaTyped, requiredObjectMetaTyped) - existingCopy.Object["metadata"], err = runtime.DefaultUnstructuredConverter.ToUnstructured(&existingObjectMetaTyped) + err = resourcemerge.EnsureObjectMetaForUnstructured(didMetadataModify, existingCopy, required) if err != nil { return nil, false, err } @@ -151,23 +115,24 @@ func ApplyUnstructuredResourceImproved( if equalityChecker == nil { equalityChecker = equality.Semantic } - existingCopy, didSpecModify, err := ensureGenericSpec(required, existingCopy, defaultingFunc, equalityChecker) + didSpecModify := ptr.To(false) + err = ensureGenericSpec(didSpecModify, required, existingCopy, defaultingFunc, equalityChecker) if err != nil { return nil, false, err } - if !didSpecModify && !*didMetadataModify { + if !*didSpecModify && !*didMetadataModify { // Update cache even if certain fields are not modified, in order to maintain a consistent cache based on the // resource hash. The resource hash depends on the entire metadata, not just the fields that were checked above, cache.UpdateCachedResourceMetadata(required, existingCopy) return existingCopy, false, nil } + // Perform update if resource exists but different from the required (desired) one. if klog.V(4).Enabled() { klog.Infof("%s %q changes: %v", resourceGVR.String(), namespace+"/"+name, JSONPatchNoError(existing, existingCopy)) } - - // Perform update if resource exists but different from the required (desired) one. - actual, err := client.Resource(resourceGVR).Namespace(namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) + var actual *unstructured.Unstructured + actual, err = client.Resource(resourceGVR).Namespace(namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) resourcehelper.ReportUpdateEvent(recorder, existingCopy, err) cache.UpdateCachedResourceMetadata(existingCopy, actual) return actual, true, err @@ -186,27 +151,27 @@ func DeleteUnstructuredResource(ctx context.Context, client dynamic.Interface, r return nil, true, nil } -func ensureGenericSpec(required, existing *unstructured.Unstructured, mimicDefaultingFn mimicDefaultingFunc, equalityChecker equalityChecker) (*unstructured.Unstructured, bool, error) { +func ensureGenericSpec(didSpecModify *bool, required, existing *unstructured.Unstructured, mimicDefaultingFn mimicDefaultingFunc, equalityChecker equalityChecker) error { mimicDefaultingFn(required) requiredSpec, _, err := unstructured.NestedMap(required.UnstructuredContent(), "spec") if err != nil { - return nil, false, err + return err } existingSpec, _, err := unstructured.NestedMap(existing.UnstructuredContent(), "spec") if err != nil { - return nil, false, err + return err } if equalityChecker.DeepEqual(existingSpec, requiredSpec) { - return existing, false, nil + return nil } - existingCopy := existing.DeepCopy() - if err := unstructured.SetNestedMap(existingCopy.UnstructuredContent(), requiredSpec, "spec"); err != nil { - return nil, true, err + if err = unstructured.SetNestedMap(existing.UnstructuredContent(), requiredSpec, "spec"); err != nil { + return err } + *didSpecModify = true - return existingCopy, true, nil + return nil } // mimicDefaultingFunc is used to set fields that are defaulted. This allows for sparse manifests to apply correctly. diff --git a/pkg/operator/resource/resourceapply/monitoring_test.go b/pkg/operator/resource/resourceapply/monitoring_test.go index 33a8af555c..ec1a611385 100644 --- a/pkg/operator/resource/resourceapply/monitoring_test.go +++ b/pkg/operator/resource/resourceapply/monitoring_test.go @@ -147,7 +147,7 @@ func TestApplyServiceMonitor(t *testing.T) { requiredMonitorSpec, _, _ := unstructured.NestedMap(unstructuredServiceMonitor.UnstructuredContent(), "spec") existingMonitorSpec, _, _ := unstructured.NestedMap(updatedMonitorObj.UnstructuredContent(), "spec") if !equality.Semantic.DeepEqual(requiredMonitorSpec, existingMonitorSpec) { - t.Fatalf("expected resulting service monitor spec to match required spec: %s", cmp.Diff(requiredMonitorMetadata, existingMonitorMetadata)) + t.Fatalf("expected resulting service monitor spec to match required spec: %s", cmp.Diff(requiredMonitorSpec, existingMonitorSpec)) } } }) diff --git a/pkg/operator/resource/resourcemerge/object_merger.go b/pkg/operator/resource/resourcemerge/object_merger.go index 4c5dcacaa7..89a757a1dc 100644 --- a/pkg/operator/resource/resourcemerge/object_merger.go +++ b/pkg/operator/resource/resourcemerge/object_merger.go @@ -1,10 +1,14 @@ package resourcemerge import ( + errorsstdlib "errors" + "fmt" "reflect" "strings" 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" ) @@ -18,6 +22,45 @@ func EnsureObjectMeta(modified *bool, existing *metav1.ObjectMeta, required meta MergeOwnerRefs(modified, &existing.OwnerReferences, required.OwnerReferences) } +func EnsureObjectMetaForUnstructured(didMetadataModify *bool, existingCopy *unstructured.Unstructured, required *unstructured.Unstructured) error { + + // Ensure metadata field is present on the object. + existingObjectMeta, found, err := unstructured.NestedMap(existingCopy.Object, "metadata") + if err != nil { + return err + } + if !found { + return errorsstdlib.New(fmt.Sprintf("metadata not found in the existing object: %s/%s", existingCopy.GetNamespace(), existingCopy.GetName())) + } + requiredObjectMeta, found, err := unstructured.NestedMap(required.Object, "metadata") + if err != nil { + return err + } + if !found { + return errorsstdlib.New(fmt.Sprintf("metadata not found in the required object: %s/%s", required.GetNamespace(), required.GetName())) + } + + // Cast the metadata to the correct type. + var existingObjectMetaTyped, requiredObjectMetaTyped metav1.ObjectMeta + err = runtime.DefaultUnstructuredConverter.FromUnstructured(existingObjectMeta, &existingObjectMetaTyped) + if err != nil { + return err + } + err = runtime.DefaultUnstructuredConverter.FromUnstructured(requiredObjectMeta, &requiredObjectMetaTyped) + if err != nil { + return err + } + + // Check if the metadata objects differ. This only checks for selective fields (excluding the resource version, among others). + EnsureObjectMeta(didMetadataModify, &existingObjectMetaTyped, requiredObjectMetaTyped) + existingCopy.Object["metadata"], err = runtime.DefaultUnstructuredConverter.ToUnstructured(&existingObjectMetaTyped) + if err != nil { + return err + } + + return nil +} + // WithCleanLabelsAndAnnotations cleans the metadata off the removal annotations/labels/ownerrefs // (those that end with trailing "-") func WithCleanLabelsAndAnnotations(obj metav1.Object) metav1.Object {