Add per-object-level filtering based on labels (excluding Rancher-generated resources) #125
Conversation
|
Hi @nobbs. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
nobbs
force-pushed
the
dev/per-object-filtering
branch
from
6cf6bb2
to
79ed357
Compare
k8s-ci-robot
added
size/L
There was a problem hiding this comment.
Thanks for this change, it looks great! I've left a few comments the we're good to go. My overall comment is just to not add complexity where we don't need it yet, e.g. if we're only excluding ConfigMaps by name, we don't need to also test their groups/kinds as well. We can easily add that in the future if we need to.
Also, can you please change the title of the commit / PR to explicitly call out Rancher. I have no concerns about hardcoding this for now, we can always move it to a command-line option or even to HNCConfiguration later if needed.
/ok-to-test
k8s-ci-robot
added
ok-to-test
nobbs
force-pushed
the
dev/per-object-filtering
branch
from
79ed357
to
572cc1e
Compare
k8s-ci-robot
added
size/M
nobbs
changed the title
|
@adrianludwin Thank you for the thorough review! I've made the changes you requested - you are certainly right, that complexity that is not required should not be added. As this change seems to be enough to make HNC usable with Rancher / RKE-based k8s clusters, I've got not problem with closing #123 without merging. |
There was a problem hiding this comment.
One last tiny nit (sorry!), plus can you please update the docs at https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#built-in-exceptions to mention this?
nobbs
force-pushed
the
dev/per-object-filtering
branch
from
572cc1e
to
c94f87d
Compare
The set of rules is hardcoded as of now, as proposed in kubernetes-sigs#104. The current rules are Rancher-specific, as Rancher does some propagation of roles, rolebindings and serviceaccounts on its own via its "projects" (used to group namespaces). Label equality is checked against both key and value - no regex support for now. Tested: added a new test to check whether roles / configmap propagation is prevented if the label is matched.
nobbs
force-pushed
the
dev/per-object-filtering
branch
from
c94f87d
to
3e7ec48
Compare
Done. And there is nothing to be sorry for ;) |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianludwin, nobbs 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 |
k8s-ci-robot
added
the
approved
The set of rules is hardcoded as of now, as proposed in #104. The current rules are Rancher-specific, as Rancher does some propagation of roles, rolebindings and serviceaccounts on its own via its "projects" (used to group namespaces). Label equality is checked against both key and value - no regex support for now.
Tested: addded a new test to check whether roles propagation is prevented if the label is matched; also checked, that this prevents propagation only for the specified groups / kinds.
I'm not really satisfied with hardcoding these, I personally would prefer a configurable solution. Unfortunately, I'm not sure how to best implement that.