Support granular managment policies #224
Conversation
lsviben
force-pushed
the
granular_managment_policies
branch
from
fd20ebc
to
4fba84b
Compare
lsviben
requested review from
ulucinar and
turkenh
and removed request for
ulucinar
lsviben
force-pushed
the
granular_managment_policies
branch
from
4fba84b
to
c26c975
Compare
lsviben
force-pushed
the
granular_managment_policies
branch
3 times, most recently
from
1a099c8
to
291bef2
Compare
lsviben
force-pushed
the
granular_managment_policies
branch
from
291bef2
to
982b36f
Compare
Signed-off-by: lsviben <[email protected]>
lsviben
force-pushed
the
granular_managment_policies
branch
from
982b36f
to
0767c52
Compare
| @@ -133,7 +133,7 @@ func (g *Builder) AddToBuilder(typeNames *TypeNames, r *resource) (*types.Named, | |||
|
|
|||
| for _, p := range r.topLevelRequiredParams { | |||
| g.validationRules += "\n" | |||
| g.validationRules += fmt.Sprintf(`// +kubebuilder:validation:XValidation:rule="self.managementPolicy == 'ObserveOnly' || has(self.forProvider.%s)",message="%s is a required parameter"`, p, p) | |||
| g.validationRules += fmt.Sprintf(`// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.%s)",message="%s is a required parameter"`, p, p) | |||
There was a problem hiding this comment.
Are we sure that for a Delete operation, we are fine with not setting a required parameter? Did we validate this? I think it's not possible to have delete-only resource right, at least with the default policy sets? Looks like Delete policy always comes with the Create policy, is this correct?
If so, are we relying on the default supported policy sets?
There was a problem hiding this comment.
Aswered a bit here, but now I see that possibly delete wont work if we dont have required fields, not sure how Terraform reacts to that. As I mentioned I didn't test all unsupported combinations. All supported policies have Delete in a combination with either Create or Update.
There was a problem hiding this comment.
... not sure how Terraform reacts to that...
This is also my concern here. I can help validating the behavior.
Another concern is the validation rules (build-time entities) are not aware of if management policies are actually enabled or not (the decision is only available at runtime). But with these "static" rules, we are conditioning whether, for instance, management policy for a resource is set to Observe.
Imagine a case where the management policy is set to Observe and the management policies feature is disabled at runtime. For now, we are relying on the crossplane-runtime to error when the management policies are not enabled but a non-default (non *) management policy is declared. I don't think this is a blocker for this PR, just trying to understand the implications so that we can maintain the new feature.
Description of your changes
Updated the ObserveOnly management policy support to the new design which
incorporates granular management policies.
The main change is that instead of enum based
spec.managementPolicy: FullControl/ObserveOnly/OrphanOnDelete, now we have an more granular array of actionsspec.managementPolicy:["Observe", "Create", "Update", "LateInitialize", "Delete"].So as upjet was already supporting ObserveOnly with Import, we needed to do minor changes on conditionals to make sure that
spec.managementPolicy:["Observe"]is still recognized as ObserveOnly. In fact, all combinations which don't contain Create or Update actions can be handled similarly.Furthermore, as there is now a management policy which skips late initialziation, I noticed that upjet was using late initialization for updating critical annotations on resources. Those annotations are usually set after Create, but as upjet offers async creation, and most resources use that (all in aws-provider for example), they are not set after Create, but in Observe, with using late-init to update the resource.
In this case, when late-init is off, we update the annotations manually in the Observe step. We could consider doing this in all situations and stop relying on late-init to do it for us. Or handle this differently.
Fixes #220
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
Tested it in the same process as crossplane/crossplane-runtime#456 as they are connected. More info on that issue.