-
Notifications
You must be signed in to change notification settings - Fork 37
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
Selective policy enforcement #57
Selective policy enforcement #57
Conversation
Initial draft for review Signed-off-by: Ian Miller <imiller@redhat.com>
Signed-off-by: Ian Miller <imiller@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like the idea of using an annotation on the replicated policy in order to determine whether to update that policy's enforcement. That, and possibly having a separate controller to manage those annotations, seems like a good solution that can be adjusted based on users' needs.
But I feel like this proposal asserts that the only field that should ever be selectively rolled out is the remediationAction
. What if users want to selectively roll out changes to policies, while keeping them in "enforce" mode?
Along those lines, is the new remediationAction
value of "controlled" required? Or could this just be implemented via an annotation on replicated policies, and by adding logic to the propagator so that any updates to the parent policy are passed to the replicated policy only if the annotation allows it? The annotation might behave more like a "pause" feature in that case.
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
@JustinKuli Thanks for the comments! One of the thoughts in writing the proposal was to keep the child policies in sync with the parent policies at all times to reduce complexity in creating/maintaining the child policies. The only deviation would be adding the annotation to the child (hopefully easy to manage this difference). This way any changes in the parent policy are always reflected in the child but only the enforcement onto the cluster is affected. I don't think the "controlled" remediation action is strictly required. This could be done with only the existing "inform" remediationAction and knowledge in the controller with respect to the annotation. Having the "controlled" value makes it more explicit that this behavior is an "opt-in", but has a couple downsides:
You also make a good point that this could be reversed and implemented as a "pause" on an enforce policy. In our use case the inform with selective enforcement seemed to fit a bit closer. |
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Ian Miller <imiller@redhat.com>
Summarizing an enforcement approach discussed with @mprahl @gparvin @JustinKuli @serngawy @jc-rh and @imiller0.
An update to the enhancment for review will be posted shortly |
Based on feedback and discussion update the mechanism for selecting clusters for enforcement to use existing Placement APIs with optional remediationActionOverride directives. Signed-off-by: Ian Miller <imiller@redhat.com>
221d89c
to
c455c06
Compare
Signed-off-by: Ian Miller <imiller@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one minor comment to mention this should work with PolicySet also. I don't think it really changes anything in the proposal.
Signed-off-by: Ian Miller <imiller@redhat.com>
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
Clarify behavior when there are multiple PlacementBindings. Signed-off-by: Ian Miller <imiller@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! This is a great and well thought out feature.
/hold for others to review
enhancements/sig-policy/28-selective-policy-enforcment/README.md
Outdated
Show resolved
Hide resolved
The subFilter is a generic concept, not necessarily limited to the remediationActionOverride. Moving it to the top level allows future extensions of this functionality to other overrides/transforms which can also make use of the subFilter. Signed-off-by: Ian Miller <imiller@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
/lgtm |
@jc-rh: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jc-rh: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Very well-written! I feel good about this.
supporting this as a feature users, or higher level | ||
controllers/orchestrators, do not have to implement a procedural | ||
pattern of copying policies and manipulating bindings/labels to | ||
progressivly enforce the policy on clusters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
progressivly enforce the policy on clusters. | |
progressively enforce the policy on clusters. |
Adding diagram depicting the selection logic for multiple PlacementBindings with various subFilter and remediationActionOverride settings. Courtesy of @JustinKuli Signed-off-by: Ian Miller <imiller@redhat.com>
c720ab7
to
dc38996
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: imiller0, jc-rh, JustinKuli, mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
@imiller0: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/unhold |
Proposal to create a mechanism through which an inform policy can be selectively enforced on a subset of the clusters it is bound to.