-
Notifications
You must be signed in to change notification settings - Fork 473
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
Adding SectionName to PolicyTargetReference #2283
Adding SectionName to PolicyTargetReference #2283
Conversation
Welcome @zhaohuabing! |
Hi @zhaohuabing. 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. |
1063517
to
474a6f4
Compare
474a6f4
to
ae71661
Compare
ae71661
to
e5d2ac8
Compare
Hey @zhaohuabing, thanks for working on this! If I'm understanding correctly, it looks like you're trying to take on a couple of separate issues:
I think we mostly have consensus on 1, but we're waiting for #2128 to merge before moving forward. On 2, I don't think we quite have enough consensus to move forward yet. Maybe once #2128 merges you can check in with @arkodg to see if he needs any help with the implementation of #2147. It may also be helpful to take a look at our documentation for the GEP process. All API changes have to go through that process so we can't start directly with a PR to change the API itself without first having an approved GEP in an "implementable" state. |
@robscott Thanks for the guidelines! What I really need is to have a Since the community have already reached agreement on 1, could we consider adding this field as a first step? This matter is quite time-sensitive for me, and I'm a bit worried that it will take a while to get #2128 merged. Moreover, PR 2128 is mainly about the discoverability and isn't related to the For the GEP process, since GEP-713 already mentions |
e5d2ac8
to
9fe398a
Compare
hey @zhaohuabing can you also edit Line 685 in 4651e91
SectionName . Imo, since this PR is in draft mode, it shouldn't distract others from reviewing the Policy Discoverability PR first
|
e25f9ce
to
9c2a145
Compare
Yes, we use "most specific wins" in other places, so I am very much in favor. @zhaohuabing, sorry, but you'll need to include some information in the SectionName part of GEP-713 about this behavior. Something like: When multiple Policies of the same type target the same object, one with a SectionName specified, and one without, the one with a SectionName is more specific, and so will have all its settings apply. The less-specifc Policy will not attach to the target. I'll also take a note to add or extend conflict-resolution rules for direct policy attachment further up, with the "more specific wins" rule. (I have more updates to do to GEP-713 already, this will add to the pile). |
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Thanks for that last change @zhaohuabing, this LGTM. /lgtm |
Ping @robscott for one more check, then we can remove the hold. |
/retest-required |
@zhaohuabing: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
/ok-to-test |
@zhaohuabing, you'll need to rerun |
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.
Thanks for all the work on this @zhaohuabing! As much as I'm hesitant to add additional complexity here, this does feel inevitable. Have a few nits, but this looks good overall. In general just trying to at least make sure we have some guidance in here that ensures this is only used when it's actually necessary.
Would also appreciate some feedback from @kflynn on this because they've raised some important concerns about the complexity of our existing policy attachment model, so want to be careful when adding more.
1e547eb
to
9260e31
Compare
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
9260e31
to
d426c47
Compare
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Thanks @zhaohuabing! /lgtm |
This allows attaching policies to a specific section within the targeted object.
Related issue: #2147
What type of PR is this?
/kind feature
What this PR does / why we need it:
What this PR does:
This PR allows attaching policies to a specific section within the targeted object.
Why we need it:
Lacking SectionName in PolicyTargetReference is blocking my current work. I'm implementing a direct policy attachment and need to target a specific listener inside a gateway.