-
Notifications
You must be signed in to change notification settings - Fork 98
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 support for ResponseHeaderModifier for HTTPRouteRule objects #1880
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1880 +/- ##
==========================================
+ Coverage 86.71% 86.83% +0.11%
==========================================
Files 88 88
Lines 5971 6025 +54
Branches 50 50
==========================================
+ Hits 5178 5232 +54
Misses 741 741
Partials 52 52 ☔ View full report in Codecov by Sentry. |
Kevin's PR - #1494 |
b31db3f
to
e4f6129
Compare
Shouldn't we be adding the responseHeaderModifier validation here ? @kate-osborn |
e4f6129
to
801ac2d
Compare
@salonichf5 it looks like the intent was to make the HTTPRequestHeaderValidator more generic so it could validate both request and response headers, and then add the response header-specific validation separately in the I like your suggestion of adding a new struct to the HTTPValidator that validates response headers. I would move the functionality of Edit: @salonichf5 after looking through the code again, I think there will be less code duplication if we keep the approach the same. I would just rename |
35fcb04
to
07d3fb2
Compare
site/content/how-to/traffic-management/request-and-response-headers.md
Outdated
Show resolved
Hide resolved
Problem: Users want to add, set, and remove response headers. Solution: Use `add_header` NGINX directive to support `ResponseHeaderModifier`. * If the action is `set`, we simply configure the `add_header` directive with the given value in the HTTPRoute spec. * If the action is `remove`, we configure the `add_header` directive with the value set to an empty string. * If the action is `add`, we create a mapping for the `$http_header_name` variable appending a comma at the end of any client provided headers (if present) and prepend this to the given value in the HTTPRoute spec.
Co-authored-by: Kate Osborn <[email protected]>
Co-authored-by: Kate Osborn <[email protected]>
Co-authored-by: Kate Osborn <[email protected]>
Co-authored-by: Kate Osborn <[email protected]>
Co-authored-by: Kate Osborn <[email protected]>
Co-authored-by: Kate Osborn <[email protected]>
Co-authored-by: Saylor Berman <[email protected]>
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.
Looks good! Just two small changes
Co-authored-by: Kate Osborn <[email protected]>
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.
Looks good! Make sure to include @kevin85421 in the final commit message
Proposed changes
Problem: Users want to add, set, and remove response headers.
Solution: Use
add_header
NGINX directive to supportResponseHeaderModifier
.set
, we simply configure theadd_header
directive with the given value in the HTTPRoute spec.remove
, we configure theadd_header
directive with the value set to an empty string.add
, we create a mapping for the$http_header_name
variable appending a comma at the end of any client provided headers (if present) and prepend this to the given value in the HTTPRoute spec.Testing: Describe any testing that you did.
Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.
Closes #1397
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.