-
Notifications
You must be signed in to change notification settings - Fork 143
Add MustCreate management policy #873
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,8 @@ func defaultSupportedManagementPolicies() []sets.Set[xpv1.ManagementAction] { | |
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionAll), | ||
| // 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), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // ObserveOnly, just observe action is done, the external resource is | ||
| // considered as read-only. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve), | ||
|
|
@@ -68,29 +70,55 @@ func defaultSupportedManagementPolicies() []sets.Set[xpv1.ManagementAction] { | |
| // No LateInitialize filling in the spec.forProvider, allowing some | ||
| // external resource fields to be managed externally. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete), | ||
| // No LateInitialize filling in the spec.forProvider, allowing some | ||
| // external resource fields to be managed externally. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete), | ||
| // No Delete, the external resource is not deleted when the managed | ||
| // resource is deleted. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize), | ||
| // No Delete, the external resource is not deleted when the managed | ||
| // resource is deleted. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize), | ||
| // No Delete and no LateInitialize, the external resource is not deleted | ||
| // when the managed resource is deleted and the spec.forProvider is not | ||
| // late initialized. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate), | ||
| // No Delete and no LateInitialize, the external resource is not deleted | ||
| // when the managed resource is deleted and the spec.forProvider is not | ||
| // late initialized. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate), | ||
| // No Update, the external resource is not updated when the managed | ||
| // resource is updated. Useful for immutable external resources. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete, xpv1.ManagementActionLateInitialize), | ||
| // No Update, the external resource is not updated when the managed | ||
| // resource is updated. Useful for immutable external resources. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete, xpv1.ManagementActionLateInitialize), | ||
| // No Update and no Delete, the external resource is not updated | ||
| // when the managed resource is updated and the external resource | ||
| // is not deleted when the managed resource is deleted. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionLateInitialize), | ||
| // No Update and no Delete, the external resource is not updated | ||
| // when the managed resource is updated and the external resource | ||
| // is not deleted when the managed resource is deleted. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionLateInitialize), | ||
| // No Update and no LateInitialize, the external resource is not updated | ||
| // when the managed resource is updated and the spec.forProvider is not | ||
| // late initialized. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete), | ||
| // No Update and no LateInitialize, the external resource is not updated | ||
| // when the managed resource is updated and the spec.forProvider is not | ||
| // late initialized. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete), | ||
| // No Update, no Delete and no LateInitialize, the external resource is | ||
| // not updated when the managed resource is updated, the external resource | ||
| // is not deleted when the managed resource is deleted and the | ||
| // spec.forProvider is not late initialized. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate), | ||
| // No Update, no Delete and no LateInitialize, the external resource is | ||
| // not updated when the managed resource is updated, the external resource | ||
| // is not deleted when the managed resource is deleted and the | ||
| // spec.forProvider is not late initialized. With MustCreate. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate), | ||
| // Like ObserveOnly, but the external resource is deleted when the | ||
| // managed resource is deleted. | ||
| sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionDelete), | ||
|
|
@@ -187,7 +215,18 @@ func (m *ManagementPoliciesResolver) ShouldCreate() bool { | |
| return true | ||
| } | ||
|
|
||
| return m.managementPolicies.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionAll) | ||
| return m.managementPolicies.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionAll, xpv1.ManagementActionMustCreate) | ||
| } | ||
|
|
||
| // MustCreate returns true if the Create action is required. If the resource already exists an error will | ||
| // be raised in the reconciler. | ||
| // If the management policy feature is disabled, it returns false. | ||
| func (m *ManagementPoliciesResolver) MustCreate() bool { | ||
| if !m.enabled { | ||
| return false | ||
| } | ||
|
|
||
| return m.managementPolicies.Has(xpv1.ManagementActionMustCreate) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // ShouldUpdate returns true if the Update action is allowed. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,8 @@ const ( | |
| errExternalResourceNotExist = "external resource does not exist" | ||
|
|
||
| errManagedNotImplemented = "managed resource does not implement connection details" | ||
|
|
||
| errMustCreate = "managed resource has mustcreate policy but external resource already exists" | ||
| ) | ||
|
Comment on lines
+74
to
75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make the MustCreate error actionable for users. 🛠️ 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 |
||
|
|
||
| // Event reasons. | ||
|
|
@@ -115,6 +117,8 @@ type ManagementPoliciesChecker interface { //nolint:interfacebloat // This has t | |
| ShouldOnlyObserve() bool | ||
| // ShouldCreate returns true if the Create action is allowed. | ||
| ShouldCreate() bool | ||
| // MustCreate returns true if the create action is required to be executed | ||
| MustCreate() bool | ||
| // ShouldLateInitialize returns true if the LateInitialize action is | ||
| // allowed. | ||
| ShouldLateInitialize() bool | ||
|
|
@@ -1144,6 +1148,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu | |
| return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) | ||
| } | ||
|
|
||
| // If the resource already exists, the MustCreate policy is set, and there are no create annotations then | ||
| // this MR did not create the resource and an error is raised. | ||
| 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))) | ||
|
|
||
| return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) | ||
| } | ||
|
|
||
| // If this resource has a non-zero creation grace period we want to wait | ||
| // for that period to expire before we trust that the resource really | ||
| // doesn't exist. This is because some external APIs are eventually | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.