Skip to content

http: support for adding tokenized HTTP headers to local reply mappers#13538

Closed
rulex123 wants to merge 11 commits intoenvoyproxy:masterfrom
rulex123:local-reply-custom-header
Closed

http: support for adding tokenized HTTP headers to local reply mappers#13538
rulex123 wants to merge 11 commits intoenvoyproxy:masterfrom
rulex123:local-reply-custom-header

Conversation

@rulex123
Copy link
Copy Markdown
Contributor

Commit Message: this PR introduces support for adding tokenized HTTP headers to local reply, which was mentioned as part of #11648.
Additional Description:
Risk Level: low
Testing: new unit and integration tests
Docs Changes: n/a
Release Notes: updated version history

Signed-off-by: Erica Manno <erica.manno@gmail.com>
…er parser

Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13538 was opened by rulex123.

see: more, trace.


// Tokenized 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.TokenizedHeaderValueOption tokenized_headers_to_add = 6
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully grok how this works as an end-user. Can you add some docs with a complete example? I.e. how is this any more useful than regular headers_to_add, why doesn't that work with local reply mapper, etc?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the question above: do we actually want to allow using the header formatter? Wouldn't that give us a fully generic way to get what we want?

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Some API questions to get started.

/wait


// Tokenized 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.TokenizedHeaderValueOption tokenized_headers_to_add = 6
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the question above: do we actually want to allow using the header formatter? Wouldn't that give us a fully generic way to get what we want?

Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
@rulex123
Copy link
Copy Markdown
Contributor Author

rulex123 commented Nov 3, 2020

@mattklein123 sorry, it took me a while to circle back to this. I have changed the API so that the configuration of a formatter for a header value is supported. Not sure if that's closer to what you had in mind: could you please take a look when you get a chance and lmk what you think? Thanks!

@mattklein123
Copy link
Copy Markdown
Member

Sorry, stepping back, can you describe at a high level what problem we are trying to solve? And then we can figure out the right solution? I'm a little confused what we are trying to achieve (generic formatting, substitution, etc.). Thank you.

/wait-any

@rulex123
Copy link
Copy Markdown
Contributor Author

rulex123 commented Nov 9, 2020

Yep, good idea to take a step back.

The original issue reported here #11648 was that custom headers were not being added to local replies; that was taken care of by this code change #12093.

However there was a larger discussion around the original issue, and a point was made about possibly introducing support for a tokenised header (where each token is a header name/value pair) that could be useful in a scenario where you want to send hop by hop debug info.

I thought that introducing the idea of a tokenised formatter for a header’s value would be one way to achieve this, but I might have misunderstood something or made the wrong assumptions. I hope that clarifies what I am trying to achieve?

@mattklein123
Copy link
Copy Markdown
Member

I thought that introducing the idea of a tokenised formatter for a header’s value would be one way to achieve this, but I might have misunderstood something or made the wrong assumptions. I hope that clarifies what I am trying to achieve?

I think my question is if we support the full substitution formatter (which I think you are saying we do) can't the user just do whatever concatenation they want manually?

/wait-any

@rulex123
Copy link
Copy Markdown
Contributor Author

Right, what the heck, the current API and formatters already support all this. Sorry for the confusion, I wasn't thinking straight: closing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants