Changed default excluded labels from hardcoded values to be provided as CLI args, fixed incorrect exclusion bypass with annotation #293
Conversation
|
|
k8s-ci-robot
added
cncf-cla: no
|
Hi @duduedri96. 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
the
size/M
|
/check-cla |
k8s-ci-robot
added
cncf-cla: yes
dedri-github
force-pushed
the
dev/excluded-labels-cli-args
branch
from
8202889 to
c31b955
Compare
k8s-ci-robot
added
size/L
dedri-github
force-pushed
the
dev/excluded-labels-cli-args
branch
from
c31b955 to
7c63faf
Compare
|
Note: force-pushed an update that also fixes the bug that was mentioned in the GitHub issue. |
dedri-github
force-pushed
the
dev/excluded-labels-cli-args
branch
from
7c63faf to
4224e86
Compare
dedri-github
changed the title
|
Thanks @adrianludwin ! Will work on that during the next weekend. |
dedri-github
force-pushed
the
dev/excluded-labels-cli-args
branch
from
4224e86 to
bdee4bd
Compare
|
@adrianludwin , I made the changes you requested, can you please re-review? About the behavior change, I left it as-is and replied on your comment above with my thoughts, and I'm not sure what to do further. |
dedri-github
force-pushed
the
dev/excluded-labels-cli-args
branch
from
bdee4bd to
31405af
Compare
k8s-ci-robot
added
size/M
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
docs/user-guide/how-to.md
Outdated
| * `--nopropagation-label`: has the same effect as the `propagate.hnc.x-k8s.io/none` | ||
| annotation as specified in [limiting propagation](how-to.md#limit-the-propagation-of-an-object-to-descendant-namespaces), | ||
| but is useful when there's no control over what annotations the object has in order | ||
| to disable the propagation of that object. This argument may be specified multiple times, | ||
| with each parameter representing one `key=val` pair of a label that should exclude an object | ||
| from propagation. | ||
| * Rancher objects that have the label `cattle.io/creator=norman` are not propagated by | ||
| default (refer to [Concepts: built in exceptions](concepts.md#built-in-exceptions) for more | ||
| information). |
There was a problem hiding this comment.
Can you please fix the indentation to match what's above it? Thanks!
docs/user-guide/how-to.md
Outdated
| with each parameter representing one `key=val` pair of a label that should exclude an object | ||
| from propagation. | ||
| * Rancher objects that have the label `cattle.io/creator=norman` are not propagated by | ||
| default (refer to [Concepts: built in exceptions](concepts.md#built-in-exceptions) for more |
There was a problem hiding this comment.
"by default" -> "by the default manifests"
Changed default exclusion by label mechanism to use this CLI argument instead of a hardcoded list of labels. Moved the default Rancher label exclusion to be a default CLI argument on the manager. Fixed a bug that caused excluded resources to be propagated with the propagate.hnc.x-k8s.io/all annotation. Tested: made sure the current test for default exclusion fails after my CLI argument change, changed the test to reflect the changes, test now passes as expected. Wrote a test that verifies the propagate.hnc.x-k8s.io/all annotation bug doesn't occur, which failed before the bugfix. Executed e2e tests and they pass. Manually tested the manager by adding the new CLI argument and with that a labeled resource was skipped, then removed the argument and it has propagated. Did the same with two different annotations specified together as two CLI arguments, and resources that had any of those labels were skipped as expected. Manually tried to cause an excluded resource to propagate by adding the propagate.hnc.x-k8s.io/all annotation, and it didn't propagate.
dedri-github
force-pushed
the
dev/excluded-labels-cli-args
branch
from
31405af to
75518d5
Compare
|
@adrianludwin , implemented your suggestions. Thank you! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianludwin, duduedri96 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
|
@duduedri96 thanks for this change! Can you please cherry-pick it into the v1.1 branch as well? Then I'll release v1.1 RC2 for testing. |
|
Thanks @adrianludwin , created #299 cherry pick PR. |
Cherry pick of #293 to v1.1
As discussed in #276, currently resources which are created by Rancher (and have the
cattle.io/creator=normanlabel) are being excluded by default, without any way to disable this behavior. In addition, there a bug that causes resources with thepropagate.hnc.x-k8s.io/allannotation to be propagated even though they are excluded.This PR changes the exclude mechanism to rely on a CLI argument to the manager instead of a hardcoded value. It also sets the default arguments of the manager to have the
cattle.io/creator=normanexcluded by default, to keep the same default behavior as before. Documentation is updated to reflect the change. In addition, the bug that caused excluded resources to be propagated when using thepropagate.hnc.x-k8s.io/allis fixed.Edit: force-pushed an update that also fixes the bug that was mentioned in the issue.