-
Notifications
You must be signed in to change notification settings - Fork 491
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
Add CEL for BackendTLSPolicy targetRefs #3496
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Norwin Schnyder <[email protected]>
Hi @snorwin. 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-sigs/prow 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.
/ok-to-test |
…olicy CEL validation Signed-off-by: Norwin Schnyder <[email protected]>
Signed-off-by: Norwin Schnyder <[email protected]>
aea4b95
to
94ef768
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.
@snorwin It would be great if you could attend the next community meeting and do a small presentation on the background and solution, for the awareness of other implementers. Also, a godoc update would be super helpful to all. |
02e9707
to
429ef35
Compare
@candita I added a description similar to the one for the I'll do my best to attend the community meeting, but the pre-xmas season is quite busy for me. |
Signed-off-by: Norwin Schnyder <[email protected]>
429ef35
to
76e172c
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.
Thanks, @snorwin!
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mlavacca, robscott, snorwin 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 |
@snorwin I asked in the Slack channel. I would prefer to make this more well-known now that we have 5 implementations and are aiming to move BackendTLSPolicy to standard. |
/lgtm Hold until this is more widely known and accepted. |
New changes are detected. LGTM label has been removed. |
Co-authored-by: Candace Holman <[email protected]>
8871d3c
to
60b7185
Compare
Thanks for the work on this @snorwin! I've added this to the agenda for our next meeting (Jan 6), and one of us can summarize the changes then (no worries if you can't make it). I think this is a strict improvement and unlikely to be problematic to implementations, but agree with @candita that we should make sure this is widely known in advance of the v1.3 release. Since this is on the agenda now for the next meeting, I'm fine with merging this at any point, but will defer to @candita on the timing. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces validation to ensure that
targetRefs
onBackendTLSPolicy
are unique. Without this validation, implementations may face challenges in properly resolving these references. A CEL is introduced, similar to the one used in theHTTPRoute
, to facilitate this validation.Which issue(s) this PR fixes:
None
Does this PR introduce a user-facing change?: