Skip to content
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

response header manipulation for DirectResponseAction #3520

Closed
derekargueta opened this issue Jun 1, 2018 · 1 comment
Closed

response header manipulation for DirectResponseAction #3520

derekargueta opened this issue Jun 1, 2018 · 1 comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@derekargueta
Copy link
Member

Title: header manipulation for DirectResponseAction

Description:
Our goal here is when we return an HTML file in a DirectResponseAction (i.e. a Forbidden page), to have it's content-type set to text/html. By default it's text/plain, rendering incorrectly in the browser. This could be done by setting the content-type response header but there is no way to specify response headers for just a DirectResponseAction that I can identify (proto struct). Another solution is to have Envoy infer the content-type for DirectResponseActions using the filesystem.

Currently, response headers can be modified at 1) the entire route configuration (doc), 2) the virtual host level (doc), or 3) in a single RouteAction (doc) which doesn't cover DirectResponseAction since they are "side-by-side".

Potential steps to resolve this are either

  1. move the response_headers_to_add options from the RouteAction to the Route so that these options can apply regardless if the action is a RouteAction, DirectResponseAction, RedirectAction or any other future actions (preferred IMO but breaks the API)
  2. Add response_headers_to_add options to DirectResponseAction, (and potentially) RedirectAction, and other future actions
  3. Support the ability to detect content-type for DirectResponseAction based on filename extension if loading from file (easy but only solves a very narrow use-case, in this case ours)

This isn't a strict requirement we have to meet, but a feature that would be useful.

cc @rgs1 @brian-pane

@mattklein123 mattklein123 added enhancement Feature requests. Not bugs or questions. help wanted Needs help! labels Jun 1, 2018
@mattklein123
Copy link
Member

I'm in favor of (1). We can't do a move, but we can add and deprecate.

zuercher pushed a commit that referenced this issue Jul 25, 2018
This adds the ability to specify response_headers_to_* and request_headers_to_add at the route level, for #3520

Risk Level: low
Testing: updated unit tests
Docs Changes: added
Fixes Issue: #3520

Signed-off-by: Derek Argueta <dereka@pinterest.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

2 participants