Add MustCreate management policy #873
Conversation
| // All actions explicitly set, the same as default. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionDelete), | ||
| // All actions explicitly set with MustCreate instead of Create. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionDelete), |
There was a problem hiding this comment.
the combinatorics are starting to get cumbersome here - are we already tracking loosening up our requirement to define every possible allowed combination of policies? i couldn't find a tracking issue for that just looking now...
There was a problem hiding this comment.
This was to ensure folks dont shoot themselves in the foot by adding unsupported/untested combinations, or combinations that dont make sense. Most of the sensible ones are here already.
Adding more policies will make this worse and if we do we should rethink this.
|
Similarly to the comment on adding the Orphan management policy, I am vary of adding new policies to the management policies except for the primitives we have. Adding In general I think the basic policies should follow the methods of the external managed client. However, I do understand where the issue is coming from and what we are trying to achive here, I just don't think adding a new management policy is the right way. I wonder should we introduce some management options field? We could add |
|
Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as |
|
/fresh |
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
bobh66
force-pushed
the
add_mustcreate_policy
branch
from
597a4df to
c972d61
Compare
📝 WalkthroughWalkthroughAdds a new ManagementAction value Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant MetaHelper as Meta Helper
participant ExternalAPI as External API
participant StatusUpdater as Status Updater
Reconciler->>MetaHelper: ExternalCreateNotStarted(o)
MetaHelper->>MetaHelper: read external-create annotations
MetaHelper-->>Reconciler: true/false
alt normal create allowed or create already started
Reconciler->>ExternalAPI: Observe / Create / Update
ExternalAPI-->>Reconciler: result
Reconciler->>StatusUpdater: update status/conditions
else MustCreate required and external exists and not started
rect rgba(255, 100, 100, 0.5)
Reconciler->>Reconciler: detect MustCreate + resource exists + not started
Reconciler->>StatusUpdater: emit warning event and set ReconcileError/Creating condition
StatusUpdater-->>Reconciler: status updated
Reconciler-->>Reconciler: return without requeue
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Would you like me to list the exact new and modified function signatures and constants for a focused review? Thanks for the clear change set. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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
🤖 Fix all issues with AI agents
In `@pkg/reconciler/managed/policies.go`:
- Around line 218-229: The docstring for ManagementPoliciesResolver.MustCreate
is incorrect: it currently says the method returns true when the management
policy feature is disabled, but the implementation returns false; update the
comment to match the code (or change behavior if intended). Specifically, edit
the MustCreate comment to state that it returns false when the feature is
disabled and that it returns true only when
managementPolicies.Has(xpv1.ManagementActionMustCreate) is true, referencing the
MustCreate method and the managementPolicies/ enabled fields so reviewers can
locate the logic.
In `@pkg/reconciler/managed/reconciler.go`:
- Around line 74-75: The user-facing error constant errMustCreate should be made
actionable: update the string value of errMustCreate (and any other places where
it's used around the reconciler logic at the other occurrence) to include the
resource identifier (use the managed resource name or external name from the
managed object) and a clear next-step suggestion such as "delete the existing
external resource or remove the MustCreate policy". Locate the errMustCreate
constant in reconciler.go and the code paths that raise/return it (around the
other occurrence noted) and format the message to interpolate the resource name
and the suggested remediation so events/conditions show "resource <name> already
exists; either delete the external resource or remove MustCreate".
🧹 Nitpick comments (4)
pkg/meta/meta.go (1)
374-385: Clarify the doc comment + simplify return.
Would you be open to aligning the comment with the actual behavior (no external-create annotations) and returning the boolean directly?♻️ Suggested tweak
-// ExternalCreateNotStarted returns true if the external resource was detected -// during the initial Observe phase and was not created by this managed resource. +// ExternalCreateNotStarted returns true when no external-create annotations +// have been recorded on the managed resource yet. func ExternalCreateNotStarted(o metav1.Object) bool { pending := GetExternalCreatePending(o) succeeded := GetExternalCreateSucceeded(o) failed := GetExternalCreateFailed(o) - if pending.IsZero() && succeeded.IsZero() && failed.IsZero() { - return true - } - - return false + return pending.IsZero() && succeeded.IsZero() && failed.IsZero() }pkg/meta/meta_test.go (1)
1228-1286: Tiny clarity nit: error label mismatch.
Is “ExternalCreateMissing” intentional? If not, aligning to the helper name makes failures easier to scan.✏️ Suggested tweak
- if diff := cmp.Diff(tc.want, got); diff != "" { - t.Errorf("ExternalCreateMissing(...): -want, +got:\n%s", diff) - } + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("ExternalCreateNotStarted(...): -want, +got:\n%s", diff) + }pkg/reconciler/managed/reconciler_modern_test.go (1)
1783-1869: Great coverage for MustCreate paths; small cleanup.
Could we drop the duplicateWithObservedGeneration(42)and (if you update the error string) align the expected error message here too?♻️ Suggested cleanup
- want.SetConditions(xpv1.Creating().WithObservedGeneration(42), xpv1.ReconcileError(errors.New(errMustCreate)).WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions( + xpv1.Creating().WithObservedGeneration(42), + xpv1.ReconcileError(errors.New(errMustCreate)).WithObservedGeneration(42), + )pkg/reconciler/managed/reconciler_legacy_test.go (1)
1816-1863: Great coverage for the MustCreate error scenario!This test properly verifies that when an external resource already exists and
MustCreatepolicy is set, the reconciler raises an error condition and doesn't requeue (which aligns with the PR objective of stopping processing in this scenario).One small thing I noticed at line 1829 - there's a duplicate
.WithObservedGeneration(42)call on theReconcileErrorcondition. While it likely doesn't affect functionality, could you clarify if this is intentional? If not, it would be cleaner to remove the redundant call for consistency with the other condition assertions.🧹 Suggested cleanup for duplicate method call
- want.SetConditions(xpv1.Creating().WithObservedGeneration(42), xpv1.ReconcileError(errors.New(errMustCreate)).WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.Creating().WithObservedGeneration(42), xpv1.ReconcileError(errors.New(errMustCreate)).WithObservedGeneration(42))
| errMustCreate = "managed resource has mustcreate policy but external resource already exists" | ||
| ) |
There was a problem hiding this comment.
Make the MustCreate error actionable for users.
Right now the condition/event message is terse and uses jargon (“mustcreate”). Could we include the external name (or managed name) and a next step (delete existing resource or remove MustCreate)? This is user-facing in conditions/events.
🛠️ Suggested tweak
- errMustCreate = "managed resource has mustcreate policy but external resource already exists"
+ errFmtMustCreate = "external resource %q already exists; delete it or remove the MustCreate policy to proceed"- if observation.ResourceExists && policy.MustCreate() && meta.ExternalCreateNotStarted(managed) {
- log.Debug(errMustCreate)
- record.Event(managed, event.Warning(reasonCannotCreate, errors.New(errMustCreate)))
- status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New(errMustCreate)))
+ if observation.ResourceExists && policy.MustCreate() && meta.ExternalCreateNotStarted(managed) {
+ extName := meta.GetExternalName(managed)
+ if extName == "" {
+ extName = managed.GetName()
+ }
+ err := errors.Errorf(errFmtMustCreate, extName)
+ log.Debug(err.Error())
+ record.Event(managed, event.Warning(reasonCannotCreate, err))
+ status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(err))Also applies to: 1151-1158
🤖 Prompt for AI Agents
In `@pkg/reconciler/managed/reconciler.go` around lines 74 - 75, The user-facing
error constant errMustCreate should be made actionable: update the string value
of errMustCreate (and any other places where it's used around the reconciler
logic at the other occurrence) to include the resource identifier (use the
managed resource name or external name from the managed object) and a clear
next-step suggestion such as "delete the existing external resource or remove
the MustCreate policy". Locate the errMustCreate constant in reconciler.go and
the code paths that raise/return it (around the other occurrence noted) and
format the message to interpolate the resource name and the suggested
remediation so events/conditions show "resource <name> already exists; either
delete the external resource or remove MustCreate".
c972d61 to
a52580f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/meta/meta_test.go`:
- Around line 1281-1288: Update the test error string to match the function
under test: change the t.Errorf message that currently reads
"ExternalCreateMissing(...)" to "ExternalCreateNotStarted(...)" in the test loop
that invokes ExternalCreateNotStarted(tc.args.o) so the error output correctly
references ExternalCreateNotStarted when cmp.Diff detects a mismatch.
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
bobh66
force-pushed
the
add_mustcreate_policy
branch
from
a52580f to
f23c684
Compare
3 participants
Description of your changes
Added a
MustCreatemanagement policy that requires the external resource to NOT exist before the MR can create it.Fixes #872
Tested using provider-kubernetes running a private branch of crossplane-runtime. Created a
Secretand also anObjectcontaining aSecretwith the same name but different data, andmanagementPolicies: ['MustCreate', 'Observe', 'Update', 'LateInitialize', 'Delete']and verified that when theObjectwas created theSecretremained unchanged and theObjectreported an error that theSecretit was trying to create was already in existence.Docs PR is crossplane/docs#993
I have:
earthly +reviewableto ensure this PR is ready for review.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.