From a9c2cc96c01d63b14bcda606ed47b200b57914c6 Mon Sep 17 00:00:00 2001 From: Carl Henrik Lunde Date: Thu, 6 Mar 2025 09:22:35 +0100 Subject: [PATCH] Avoid setting tags if there are no changes, prevent conflict on every resource creation I've observed this error message on every resource creation: 2025-03-06T09:21:58+01:00 DEBUG provider-aws Cannot initialize managed resource {"controller": "managed/ec2.aws.upbound.io/v1beta1, kind=securitygroup", "request": {"name":"example4"}, "uid": "6110a508-2be7-470c-8d00-10302dc9c270", "version": "101685", "external-name": "", "error": "Operation cannot be fulfilled on securitygroups.ec2.aws.upbound.io \"example4\": the object has been modified; please apply your changes to the latest version and try again"} By avoiding this unneccessary unconditional Update API call, we get rid of this error message. Signed-off-by: Carl Henrik Lunde --- pkg/config/resource.go | 30 ++++++++++++---- pkg/config/resource_test.go | 72 +++++++++++++++++++++++++++++++++---- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/pkg/config/resource.go b/pkg/config/resource.go index 793ecddc..3293fe3a 100644 --- a/pkg/config/resource.go +++ b/pkg/config/resource.go @@ -351,10 +351,13 @@ func (t *Tagger) Initialize(ctx context.Context, mg xpresource.Managed) error { if err != nil { return err } - pavedByte, err := setExternalTagsWithPaved(xpresource.GetExternalTags(mg), paved, t.fieldName) + pavedByte, changed, err := setExternalTagsWithPaved(xpresource.GetExternalTags(mg), paved, t.fieldName) if err != nil { return err } + if !changed { + return nil + } if err := json.Unmarshal(pavedByte, mg); err != nil { return err } @@ -364,21 +367,36 @@ func (t *Tagger) Initialize(ctx context.Context, mg xpresource.Managed) error { return nil } -func setExternalTagsWithPaved(externalTags map[string]string, paved *fieldpath.Paved, fieldName string) ([]byte, error) { +func tagsUpToDate(tags map[string]*string, paved *fieldpath.Paved, tagField string) bool { + curTags, _ := paved.GetStringObject(tagField) + for k, v := range tags { + if curTags[k] != *v { + return false + } + } + return true +} + +func setExternalTagsWithPaved(externalTags map[string]string, paved *fieldpath.Paved, fieldName string) ([]byte, bool, error) { + tagField := fmt.Sprintf("spec.forProvider.%s", fieldName) tags := map[string]*string{ xpresource.ExternalResourceTagKeyKind: ptr.To(externalTags[xpresource.ExternalResourceTagKeyKind]), xpresource.ExternalResourceTagKeyName: ptr.To(externalTags[xpresource.ExternalResourceTagKeyName]), xpresource.ExternalResourceTagKeyProvider: ptr.To(externalTags[xpresource.ExternalResourceTagKeyProvider]), } - if err := paved.SetValue(fmt.Sprintf("spec.forProvider.%s", fieldName), tags); err != nil { - return nil, err + if tagsUpToDate(tags, paved, tagField) { + return nil, false, nil + } + + if err := paved.SetValue(tagField, tags); err != nil { + return nil, false, err } pavedByte, err := paved.MarshalJSON() if err != nil { - return nil, err + return nil, false, err } - return pavedByte, nil + return pavedByte, true, nil } type InjectedKey struct { diff --git a/pkg/config/resource_test.go b/pkg/config/resource_test.go index 42020bd6..3b8fbe20 100644 --- a/pkg/config/resource_test.go +++ b/pkg/config/resource_test.go @@ -15,6 +15,7 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/resource/fake" "github.com/crossplane/crossplane-runtime/v2/pkg/test" "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -40,7 +41,7 @@ func TestTaggerInitialize(t *testing.T) { }{ "Successful": { args: args{ - mg: &fake.Managed{}, + mg: &fake.Managed{ObjectMeta: metav1.ObjectMeta{Name: "name"}}, kube: &test.MockClient{MockUpdate: test.NewMockUpdateFn(nil)}, }, want: want{ @@ -49,7 +50,7 @@ func TestTaggerInitialize(t *testing.T) { }, "Failure": { args: args{ - mg: &fake.Managed{}, + mg: &fake.Managed{ObjectMeta: metav1.ObjectMeta{Name: "name"}}, kube: &test.MockClient{MockUpdate: test.NewMockUpdateFn(errBoom)}, }, want: want{ @@ -62,7 +63,7 @@ func TestTaggerInitialize(t *testing.T) { tagger := NewTagger(tc.kube, "tags") gotErr := tagger.Initialize(context.TODO(), tc.mg) if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { - t.Fatalf("generateTypeName(...): -want error, +got error: %s", diff) + t.Fatalf("tagger.Initialize(...): -want error, +got error: %s", diff) } }) } @@ -76,13 +77,14 @@ func TestSetExternalTagsWithPaved(t *testing.T) { } type want struct { pavedString string + changed bool err error } cases := map[string]struct { args want }{ - "Successful": { + "SuccessfulNew": { args: args{ externalTags: map[string]string{ xpresource.ExternalResourceTagKeyKind: kind, @@ -93,21 +95,77 @@ func TestSetExternalTagsWithPaved(t *testing.T) { fieldName: "tags", }, want: want{ + changed: true, + pavedString: fmt.Sprintf(`{"spec":{"forProvider":{"tags":{"%s":"%s","%s":"%s","%s":"%s"}}}}`, + xpresource.ExternalResourceTagKeyKind, kind, + xpresource.ExternalResourceTagKeyName, name, + xpresource.ExternalResourceTagKeyProvider, provider), + }, + }, + "SuccessfulChange": { + args: args{ + paved: fieldpath.Pave(map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "tags": map[string]any{ + xpresource.ExternalResourceTagKeyKind: "WrongKind", + xpresource.ExternalResourceTagKeyName: name, + xpresource.ExternalResourceTagKeyProvider: provider, + }, + }, + }, + }), + externalTags: map[string]string{ + xpresource.ExternalResourceTagKeyKind: kind, + xpresource.ExternalResourceTagKeyName: name, + xpresource.ExternalResourceTagKeyProvider: provider, + }, + fieldName: "tags", + }, + want: want{ + changed: true, pavedString: fmt.Sprintf(`{"spec":{"forProvider":{"tags":{"%s":"%s","%s":"%s","%s":"%s"}}}}`, xpresource.ExternalResourceTagKeyKind, kind, xpresource.ExternalResourceTagKeyName, name, xpresource.ExternalResourceTagKeyProvider, provider), }, }, + "SuccessfulNoChange": { + args: args{ + paved: fieldpath.Pave(map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "tags": map[string]any{ + xpresource.ExternalResourceTagKeyKind: kind, + xpresource.ExternalResourceTagKeyName: name, + xpresource.ExternalResourceTagKeyProvider: provider, + }, + }, + }, + }), + externalTags: map[string]string{ + xpresource.ExternalResourceTagKeyKind: kind, + xpresource.ExternalResourceTagKeyName: name, + xpresource.ExternalResourceTagKeyProvider: provider, + }, + fieldName: "tags", + }, + want: want{ + changed: false, + }, + }, } for n, tc := range cases { t.Run(n, func(t *testing.T) { - gotByte, gotErr := setExternalTagsWithPaved(tc.externalTags, tc.paved, tc.fieldName) + gotByte, gotChanged, gotErr := setExternalTagsWithPaved(tc.externalTags, tc.paved, tc.fieldName) if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { - t.Fatalf("generateTypeName(...): -want error, +got error: %s", diff) + t.Fatalf("setExternalTagsWithPaved(...): -want error, +got error: %s", diff) + } + if tc.want.changed != gotChanged { + t.Fatalf("setExternalTagsWithPaved(...): want changed %t, got changed: %t", tc.want.changed, gotChanged) } if diff := cmp.Diff(tc.want.pavedString, string(gotByte), test.EquateErrors()); diff != "" { - t.Fatalf("generateTypeName(...): -want gotByte, +got gotByte: %s", diff) + t.Fatalf("setExternalTagsWithPaved(...): -want gotByte, +got gotByte: %s", diff) } }) }