Additional complianceTypes for ConfigurationPolicy #131
Conversation
Refs: - https://issues.redhat.com/browse/ACM-14825 Signed-off-by: Justin Kulikauskas <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JustinKuli The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| state with what is defined in the policy. One specific difference is when the policy includes a list | ||
| in the object template: `musthave` will check the existing list for any items specified in the | ||
| policy, and if any are missing, it will *add* them to the list and apply the change. Tools like | ||
| `kubectl apply` can do something similar on some objects, via a "strategic merge", but not on custom |
There was a problem hiding this comment.
I think strategic merge does work on custom resources:
https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch
There was a problem hiding this comment.
I take that back. I got confused with this:
https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy
There was a problem hiding this comment.
Oh, good to know that server-side-apply can do strategic merges on custom resources, although it begs the question of why exactly the client-side can't do the same thing - it should have access to the same openapi information from the CRDs...
I'll have to try server-side-apply more, but from a quick read I'd be worried it would have more "conflicts" than we'd want?
There was a problem hiding this comment.
@JustinKuli could you research what it'd look like if we did server-side apply while forcing ownership of fields if the field is in the ConfigurationPolicy?
| reverting other "unrelated" changes in the object. This merge strategy works on all resource types, | ||
| including custom ones. | ||
|
|
||
| Add a `musthavestrategic` complianceType, corresponding to `kubectl patch --type=strategic`. This |
There was a problem hiding this comment.
What would happen if the user specified this and it was applied on a custom resource? Would it fall back to merge?
There was a problem hiding this comment.
Something like kubectl patch operatorpolicy --type=strategic ... fails with an error mentioning that application/strategic-merge-patch+json is not supported.
| "patchStrategy" information available in those type definitions. For example, it allows for adding | ||
| an additional environment variable to a Deployment, as long as it has a new name. | ||
|
|
||
| Add a `musthaveapply` complianceType, corresponding to `kubectl apply`. The patch type is chosen |
There was a problem hiding this comment.
I'm thinking that it may be better to just pick this option and not have the other two options since I don't really see a reason to have merge behavior when strategic merge is available.
What do you think?
There was a problem hiding this comment.
Strategic merge is very similar to the bespoke musthave. The name key that musthave uses as a mergeKey is the most common one I found in the API, so it covers a good number of cases. The biggest difference is that musthave works on custom resources.
If I had to pick only one new option, I'd actually prefer musthavemerge. Its behavior is the most different from what we currently have, and I think story 1 makes a good case for why it would be better than mustonlyhave.
There was a problem hiding this comment.
I see what you mean. Perhaps both does make sense.
|
|
||
| 1. Support strategic merge on custom resources (`kubectl` does not support this). | ||
|
|
||
| ## Proposal |
There was a problem hiding this comment.
Could you look into the options that Argo CD provides and why? I believe this is the diff code:
https://github.com/argoproj/gitops-engine/blob/72bcdda3f0a5b80432d9f72e5b30827a530ac349/pkg/diff/diff.go#L75
They seem to allow server side apply if asked for. They seem to try to use strategic merge on native Kubernetes types and fallback to JSON merge (see the ThreeWayDiff function, though we wouldn't provide last-applied-configuration and would just provide a copy of the desired like in the TwoWayDiff function).
Refs: - https://issues.redhat.com/browse/ACM-14825 Signed-off-by: Justin Kulikauskas <[email protected]>
Refs: