Avoid setting tags if there are no changes, prevent conflict on every resource creation#476
Avoid setting tags if there are no changes, prevent conflict on every resource creation#476chlunde wants to merge 1 commit intocrossplane:mainfrom
Conversation
chlunde
requested review from
erhancagirici,
sergenyalcin and
ulucinar
as code owners
… 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 <chlunde@ifi.uio.no>
📝 WalkthroughWalkthroughThese changes add change-detection to external tag updates by modifying Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/config/resource.go (1)
392-399: Consider wrapping the new error returns with context per the project's guidelines.Both new
return nil, false, errpaths propagate errors frompaved.SetValueandpaved.MarshalJSONwithout any context, which makes it harder for provider developers to understand what failed. Adding meaningful wraps would align with the project's error-messaging guidelines.♻️ Suggested wrapping
- if err := paved.SetValue(tagField, tags); err != nil { - return nil, false, err + if err := paved.SetValue(tagField, tags); err != nil { + return nil, false, errors.Wrap(err, "cannot set external tags on paved object") } pavedByte, err := paved.MarshalJSON() if err != nil { - return nil, false, err + return nil, false, errors.Wrap(err, "cannot marshal paved object after setting external tags") }As per coding guidelines: "Wrap errors with context using patterns like:
errors.Wrap(err, "cannot configure resource")" and "Ensure all error messages are meaningful to provider developers."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/resource.go` around lines 392 - 399, The error returns from paved.SetValue and paved.MarshalJSON lack context; update the error handling in the block using paved.SetValue(tagField, tags) and paved.MarshalJSON() to wrap the underlying errors with descriptive messages (e.g., using errors.Wrap or fmt.Errorf("%w", ...)) such as "cannot set tag field <tagField>" and "cannot marshal paved JSON" so the returned errors provide clear context for provider developers while preserving the original error.pkg/config/resource_test.go (2)
29-71: Consider adding a "no-op" test case to cover the new early-return path.Both existing cases start with a fresh
fake.Managed(no tags pre-set), sotagsUpToDatealways returnsfalse,changedis alwaystrue, andkube.Updateis always called. The new code path — where tags are already up-to-date andkube.Updateis not called — isn't exercised at all inTestTaggerInitialize.A quick way to cover this is to add a case where the managed object is pre-paved with the correct tags, and assert that
kube.Updateis never invoked:"SuccessfulNoChange": { args: args{ mg: func() xpresource.Managed { // pre-set managed object with correct external tags so tagsUpToDate == true mg := &fake.Managed{ObjectMeta: metav1.ObjectMeta{Name: "name"}} // ... pave and set tags on mg before returning ... return mg }(), kube: &test.MockClient{ MockUpdate: test.NewMockUpdateFn(errors.New("Update must not be called when tags are up to date")), }, }, want: want{err: nil}, },Would you be open to adding this? Happy to help draft it if useful!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/resource_test.go` around lines 29 - 71, Add a "no-op" test case to TestTaggerInitialize that verifies the early-return path when tags are already up-to-date: create a new case named "SuccessfulNoChange" where the mg argument is constructed via a function literal that pre-sets the managed object's external tags so tagsUpToDate(...) will return true, use NewTagger(...) and pass a test.MockClient whose MockUpdate is set to a MockUpdateFn that returns an error like "Update must not be called when tags are up to date" (so the test fails if kube.Update is invoked), and expect nil error from tagger.Initialize(...); this exercises the path in Initialize where changed==false and Update is not called.
159-172: The assertion ordering looks good — one optional tweak.The current flow (
err→changed→pavedString) is the right order. One minor note: usingt.Fatalffor all three assertions means the first failure stops the test without showing the other two. For test assertions that are independent,t.Errorfwould give a fuller picture on failure. Totally optional and consistent with the surrounding file style, so take it or leave it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/resource_test.go` around lines 159 - 172, The test uses t.Fatalf for three independent assertions inside the t.Run for setExternalTagsWithPaved, which aborts the subtest on the first failure; change the assertions for the independent checks (the changed comparison using gotChanged / tc.want.changed and the pavedString comparison using string(gotByte) / tc.want.pavedString with cmp.Diff) from t.Fatalf to t.Errorf so multiple failures are reported, while you may keep the initial error assertion (comparing gotErr) as t.Fatalf if you want to stop on unexpected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/resource.go`:
- Around line 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.
- Around line 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.
---
Nitpick comments:
In `@pkg/config/resource_test.go`:
- Around line 29-71: Add a "no-op" test case to TestTaggerInitialize that
verifies the early-return path when tags are already up-to-date: create a new
case named "SuccessfulNoChange" where the mg argument is constructed via a
function literal that pre-sets the managed object's external tags so
tagsUpToDate(...) will return true, use NewTagger(...) and pass a
test.MockClient whose MockUpdate is set to a MockUpdateFn that returns an error
like "Update must not be called when tags are up to date" (so the test fails if
kube.Update is invoked), and expect nil error from tagger.Initialize(...); this
exercises the path in Initialize where changed==false and Update is not called.
- Around line 159-172: The test uses t.Fatalf for three independent assertions
inside the t.Run for setExternalTagsWithPaved, which aborts the subtest on the
first failure; change the assertions for the independent checks (the changed
comparison using gotChanged / tc.want.changed and the pavedString comparison
using string(gotByte) / tc.want.pavedString with cmp.Diff) from t.Fatalf to
t.Errorf so multiple failures are reported, while you may keep the initial error
assertion (comparing gotErr) as t.Fatalf if you want to stop on unexpected
errors.
In `@pkg/config/resource.go`:
- Around line 392-399: The error returns from paved.SetValue and
paved.MarshalJSON lack context; update the error handling in the block using
paved.SetValue(tagField, tags) and paved.MarshalJSON() to wrap the underlying
errors with descriptive messages (e.g., using errors.Wrap or fmt.Errorf("%w",
...)) such as "cannot set tag field <tagField>" and "cannot marshal paved JSON"
so the returned errors provide clear context for provider developers while
preserving the original 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 |
There was a problem hiding this comment.
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.
| 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 | ||
| } |
There was a problem hiding this comment.
Two small safety concerns worth addressing in tagsUpToDate.
- Nil pointer dereference (Line 373):
*vwill panic if any value in thetagsmap isnil. The current call site always usesptr.To(...), so it's safe today, but the function's signature acceptsmap[string]*stringwithout 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
}
}- 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 returnfalse), 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.
| 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.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/config/resource_test.go (2)
42-59: Thanks for updating the test fixtures withObjectMeta— this ensures the external tags are non-empty.I appreciate the thoughtfulness here. Without a
Name, the external tags could be empty strings that match the zero-value of a missing paved field, which would causetagsUpToDateto returntrueand skip the update entirely — making the "Successful" test no longer exercise the update path.Could I suggest adding a test case for the no-change path through
Initialize? Currently, the early-return at Lines 358–360 ofresource.gois only covered indirectly viaTestSetExternalTagsWithPaved. A test where thefake.Managedalready has correct tags in its paved state — and theMockUpdateis wired to fail if called — would give more confidence that the unnecessary API call is truly prevented. That's the core value proposition of this PR, after all!Something along these lines:
💡 Suggested test case for the no-change path
"NoChangeSkipsUpdate": { args: args{ mg: func() xpresource.Managed { m := &fake.Managed{ObjectMeta: metav1.ObjectMeta{Name: "name"}} // Pre-populate the tags so they match what Initialize would set paved, _ := fieldpath.PaveObject(m) tags := map[string]*string{ xpresource.ExternalResourceTagKeyKind: ptr.To(m.GetObjectKind().GroupVersionKind().Kind), xpresource.ExternalResourceTagKeyName: ptr.To(m.GetName()), xpresource.ExternalResourceTagKeyProvider: ptr.To(""), } _ = paved.SetValue("spec.forProvider.tags", tags) // Unmarshal back into the managed resource b, _ := paved.MarshalJSON() _ = json.Unmarshal(b, m) return m }(), kube: &test.MockClient{MockUpdate: test.NewMockUpdateFn(errors.New("update should not be called"))}, }, want: want{ err: nil, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/resource_test.go` around lines 42 - 59, Add a new unit test case "NoChangeSkipsUpdate" in resource_test.go that constructs a fake.Managed (with ObjectMeta{Name: "name"}), pre-populates its paved "spec.forProvider.tags" to exactly match what Initialize would set (the ExternalResourceTag keys for Kind, Name, Provider), then uses test.MockClient{MockUpdate: test.NewMockUpdateFn(errors.New("update should not be called"))} so any update call fails the test; this verifies tagsUpToDate/Initialize short-circuits and avoids calling kube.Update (mirroring TestSetExternalTagsWithPaved but asserting no update is invoked). Ensure the test references Initialize/tagsUpToDate behavior by building the paved object via fieldpath.PaveObject and marshaling back into the managed resource before calling the code under test.
160-169: Thechangedflag assertion on Line 164 is direct and clear.Minor note: the
test.EquateErrors()option on Line 167 is a no-op when comparing strings (it's designed forerrortypes), but it's harmless and consistent with the existing style in this file. No action needed — just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/resource_test.go` around lines 160 - 169, The cmp.Diff call comparing expected pavedString to string(gotByte) uses test.EquateErrors() which is a no-op for string comparisons; remove the unnecessary option to clean up the assertion in the setExternalTagsWithPaved test (update the cmp.Diff invocation that currently passes test.EquateErrors() so it just compares tc.want.pavedString and string(gotByte)); keep the direct changed-flag assertion using gotChanged as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/config/resource.go`:
- Around line 370-378: In tagsUpToDate, guard against nil pointer deref of map
values by checking v != nil before dereferencing (*v) and treat a nil v as
non-matching (return false) or handle per intended semantics; also avoid
silently dropping the error from paved.GetStringObject—either handle the error
or add an inline comment next to the call (GetStringObject) explaining that on
error we intentionally treat curTags as empty so the function returns false
(safe fallback).
---
Nitpick comments:
In `@pkg/config/resource_test.go`:
- Around line 42-59: Add a new unit test case "NoChangeSkipsUpdate" in
resource_test.go that constructs a fake.Managed (with ObjectMeta{Name: "name"}),
pre-populates its paved "spec.forProvider.tags" to exactly match what Initialize
would set (the ExternalResourceTag keys for Kind, Name, Provider), then uses
test.MockClient{MockUpdate: test.NewMockUpdateFn(errors.New("update should not
be called"))} so any update call fails the test; this verifies
tagsUpToDate/Initialize short-circuits and avoids calling kube.Update (mirroring
TestSetExternalTagsWithPaved but asserting no update is invoked). Ensure the
test references Initialize/tagsUpToDate behavior by building the paved object
via fieldpath.PaveObject and marshaling back into the managed resource before
calling the code under test.
- Around line 160-169: The cmp.Diff call comparing expected pavedString to
string(gotByte) uses test.EquateErrors() which is a no-op for string
comparisons; remove the unnecessary option to clean up the assertion in the
setExternalTagsWithPaved test (update the cmp.Diff invocation that currently
passes test.EquateErrors() so it just compares tc.want.pavedString and
string(gotByte)); keep the direct changed-flag assertion using gotChanged as-is.
Reproduction setupUsed Changes to provider template
|
1 participant
I've observed this error message on every resource creation:
By avoiding this unneccessary unconditional Update API call, we get rid of this error message.
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested