-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
http: add headers via local reply mapper #12093
http: add headers via local reply mapper #12093
Conversation
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[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.
/lgtm api
I don’t think that |
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@htuch can you re-approve pls? I needed to push a small doc string update in one of the proto files. |
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 looks great with a small comment.
/wait
// HTTP headers to add to a local reply. This allows the response mapper to append, to add | ||
// or to override headers of any local reply before it is sent to a downstream client. | ||
repeated config.core.v3.HeaderValueOption headers_to_add = 5 | ||
[(validate.rules).repeated = {max_items: 1000}]; |
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 would probably remove this. The limit seems somewhat arbitrary.
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.
We have these limits in other places for heads to add. I think it was put in for fuzzing.
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 see, OK. Sounds good.
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.
Yes, this is why I added this requirement - I checked other places where the list of headers was defined and basically all of them had this limitation.
api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
Show resolved
Hide resolved
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
I needed to merge master and push due to the conflict in |
eed90b5
to
5841230
Compare
This reverts commit 53370b3. Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
I’m seeing the following error (I needed to merge master due to a conflict in
and I’m not sure how it’s related to my PR. I can update the content of the aforementioned file ^ but I’m not sure whether I should be doing this 🤔 @mattklein123 am I missing something here? |
I think that merging #12142 should help to get rid of the error I mentioned in my message above. |
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@mattklein123 @htuch After a few merges of master it’s green again and ready to be merged. |
Signed-off-by: Rafal Augustyniak <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]> Signed-off-by: scheler <[email protected]>
Add an option for local reply mapper to add HTTP headers to local responses.
Commit Message:
Additional Description:
Risk Level: Low
Testing: Added unit tests and updated integration tests
Docs Changes: Updated
Release Notes: Updated
Fixes #11707