-
Notifications
You must be signed in to change notification settings - Fork 453
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 and test single filter action per header restriction #1497
Add and test single filter action per header restriction #1497
Conversation
Hi @rainest. 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. |
Kong/kubernetes-ingress-controller#3119 demonstrates a draft implementation in Kong's implementation. Not quite sure what the expected chicken-before-egg order is for demonstrating changes that aren't actually in the spec yet. https://github.com/Kong/kubernetes-ingress-controller/actions/runs/3365550934/jobs/5581123044 is a simplified conformance run that runs this test only. I originally included a test for multiple of the same action, but it turns out this is already forbidden by the schema. Something like:
is rejected by the API server with
|
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 catching this @rainest!
Reason: string(v1beta1.RouteReasonUnsupportedValue), | ||
} | ||
|
||
kubernetes.HTTPRouteMustHaveCondition(t, suite.Client, suite.TimeoutConfig, route, gwNN, headerConflictCond) |
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.
If we do solve this with webhook validation, we either may not need this conformance test, or we should explicitly require that conformance tests are run without the validating webhook present so we can ensure that controllers still behave well in its absence. I tend to think we should just start with webhook validation, but open to other opinions here.
/cc @youngnick
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 agree that we probably only want the webhook--we may as well avoid the additional implementation burden. I've updated the docstring to remove references to the condition as such, since these should never reach implementations.
/ok-to-test |
6dd4354
to
ff8b759
Compare
ff8b759
to
2625c49
Compare
Add documentation for HTTPHeaderFilter establishing a single action per header limit, with explanation and examples showing the expected CSV format for multiple-value headers.
Add an HTTPRoute webhook validation rule for RequestHeaderModifier and ResponseHeaderModifier filters. The rule rejects filters that contain multiple actions for the same header name.
2625c49
to
f7343c5
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 @rainest!
Ignore case when checking to see if a header has multiple actions defined. Add tests for case insensitivity, valid header filters, and multiple of the same action for the same header.
Aside from the minor change @robscott asked for, and the followups about clarifying the webhook's position, this LGTM. |
Oops, I meant to approve, so I can leave the final LGTM to someone else. /approve |
Thanks @rainest! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rainest, robscott, youngnick 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 |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Documents restrictions on header filters and how implementations should handle invalid filters. Header filters cannot modify the same header twice. When users require multiple values for the same header, they must specify the comma-separated values in configuration.
Adds a conformance test confirming that invalid headers result in a falseAccepted
Condition with reasonRouteReasonUnsupportedValue
.Adds a webhook rule that rejects header filters that violate the single action per header rule.
Which issue(s) this PR fixes:
Fix #480. Order of operations is a moot point if there is only one action for a given header, and there are no outcomes that actually require multiple actions.
Does this PR introduce a user-facing change?: