-
Notifications
You must be signed in to change notification settings - Fork 96
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 RequestHeaderModifier for HTTPRouteRule objects #717
Conversation
f4d73c4
to
9d69a14
Compare
261e236
to
1ea40fe
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.
hi @ciarams87 Please see my feedback.
I didn't comment on the code yet, just the approach, so that we agree on the requirements and the approach first.
UPDATED:
I updated the review comments - the map approach is reasonable, no need to consider njs. (I incorrectly tested some map NGINX config in older version of NGINX. new versions of NGINX - starting from 1.23.0 - combine all indentical header lines
Changes with nginx 1.23.0 21 Jun 2022
*) Change in internal API: now header lines are represented as linked
lists.
*) Change: now nginx combines arbitrary header lines with identical
names when sending to FastCGI, SCGI, and uwsgi backends, in the
$r->header_in() method of the ngx_http_perl_module, and during lookup
of the "$http_...", "$sent_http_...", "$sent_trailer_...",
"$upstream_http_...", and "$upstream_trailer_..." variables.
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.
additional feedback, considering the fact that the approach with using maps works well.
b562f93
to
99bd512
Compare
355c004
to
eec590d
Compare
61c201a
to
f2f20ef
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.
Looks good! Just a few small suggestions.
3cdf684
to
4d10b9d
Compare
33d9bc1
to
ecb282e
Compare
046a20c
to
1d5188a
Compare
cac212f
to
69e51a8
Compare
69e51a8
to
dcf807b
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.
👍
dcf807b
to
60ab96e
Compare
Proposed changes
Problem: As an App Dev I will want to manipulate headers at ingress. Either to add correlation IDs, provenance data, strip unnecessary headers, or yet another use case not foreseen or delineated here.
Solution:
Header manipulation is supported through the HTTPRouteFilterType == RequestHeaderModifier. This PR extends the current filter logic to support the RequestHeaderModifier filter. This RequestHeaderModifier filter maps to the
proxy_set_header
NGINX directive.There are three possible actions -
set
,add
, andremove
. Set redefines any header value, add appends to a given header value if present, and remove strips the header from the request. All three are configured using the sameproxy_set_header
NGINX directive.If the action is
set
, we simply configure the directive with the given value in the HTTPRoute spec.If the action is
remove
, we configure the 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.See https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_set_header.
This commit also validates the headers and values and rejects anything that might cause NGINX to fail to reload or any malicious value. We also reject redefining the Host header.
Testing: Unit-testing (100% coverage), e2e testing using the supplied example and variations of same (example uses an echo server to verify the correct request headers are indeed being set, added, and removed)
Closes #480
Checklist
Before creating a PR, run through this checklist and mark each as complete.