Skip to content

Avoid setting tags if there are no changes, prevent conflict on every resource creation #476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chlunde wants to merge 1 commit into
base: main
Choose a base branch
from
+89 −13
Open

Avoid setting tags if there are no changes, prevent conflict on every resource creation #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions pkg/config/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Comment on lines +370 to +377
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Worth confirming: the subset-check is intentional.

tagsUpToDate only verifies that the three desired system tags are present with the correct values — it doesn't require the tag field to contain exactly those three keys. This creates a subtle asymmetry:

  • System tags already correct + extra user tags present → tagsUpToDate = true → update skipped → user tags are preserved ✅
  • Any system tag wrong + extra user tags present → tagsUpToDate = false → paved.SetValue(tagField, tags) replaces the entire field with only the three system tags → extra user tags are lost ⚠️

The first case is an improvement over the old behavior (which always replaced). But the second case is unchanged and still discards user-defined tags when a system tag needs updating. Is this an acceptable trade-off, or should setExternalTagsWithPaved be doing a merge instead of a full replace when it does write? Happy to discuss the intent here — just want to make sure this asymmetry is deliberate.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/config/resource.go` around lines 370 - 377, The current logic only checks
subset equality in tagsUpToDate and then overwrites the whole tag field in
setExternalTagsWithPaved, which discards user tags when any system tag must be
updated; change setExternalTagsWithPaved to merge instead of replace: read the
existing tag map from the paved (using the same tagField/get method), copy its
entries, overwrite or set the system tag keys from the desired tags map (the
`tags` param) into that copy, and call paved.SetValue(tagField, mergedMap) so
user tags are preserved while system tags are updated; keep tagsUpToDate as the
quick subset check.

}
Comment on lines +370 to +378
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Two small safety concerns worth addressing in tagsUpToDate.

  1. Nil pointer dereference (Line 373): *v will panic if any value in the tags map is nil. The current call site always uses ptr.To(...), so it's safe today, but the function's signature accepts map[string]*string without that guarantee. Adding a nil guard here is cheap insurance:
🛡️ Suggested nil guard 
 for k, v := range tags {
+    if v == nil {
+        return false
+    }
     if curTags[k] != *v {
         return false
     }
 }
  1. Silently discarded error (Line 371): curTags, _ := paved.GetStringObject(tagField) — dropping the error is benign here (a missing or non-string field yields an empty map, which correctly causes the function to return false), but it makes it harder to debug unexpected field types in future. At least a comment explaining the intentional discard would help readers understand the invariant.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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 tagsUpToDate(tags map[string]*string, paved *fieldpath.Paved, tagField string) bool {
curTags, _ := paved.GetStringObject(tagField)
for k, v := range tags {
if v == nil {
return false
}
if curTags[k] != *v {
return false
}
}
return true
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/config/resource.go` around lines 370 - 378, tagsUpToDate currently
dereferences values in the tags map without guarding nil and ignores the error
from paved.GetStringObject; update the function (tagsUpToDate) to first call
paved.GetStringObject(tagField) and keep the current behavior when an error
occurs but add an explanatory comment that a non-existent/non-string field
should be treated as empty, and before comparing each entry ensure the pointer v
is not nil (treat nil as not up-to-date and return false) instead of
dereferencing *v unguarded; reference symbols: tagsUpToDate, tags
map[string]*string, paved.GetStringObject, curTags, and v.


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 {
Expand Down
72 changes: 65 additions & 7 deletions pkg/config/resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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)
}
})
}
Expand All @@ -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,
Expand All @@ -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)
}
})
}
Expand Down
Loading