Envoy doesn't add headers in case of Gateway Timeout

[pugachAG]

Description:

Envoy doesn't add headers specified in the route configuration in case of upstream timeout.

Repro steps:

Send request to upstream which doesn't respond within specified route timeout.

Expected: envoy adds headers specified under response_headers_to_add

Actual: envoy doesn't add any headers:

          'server' => 'envoy',
          'content-length' => '24',
          'Reason' => 'Gateway Timeout',
          'URL' => 'http://authxserver.service/infra_test/idempotent',
          'HTTPVersion' => '1.1',
          'content-type' => 'text/plain',
          'date' => 'Fri, 31 Aug 2018 13:21:31 GMT',
          'Status' => 504

route config:

    {
     "match": {
      "regex": "^/(infra_test/idempotent|/iam/issue-refresh-token|identity/user-exists/.*)"
     },
     "route": {
      "cluster": "authxserver.prod.cluster",
      "timeout": "0.500s",
      "retry_policy": {
       "retry_on": "5xx",
       "num_retries": 2,
       "per_try_timeout": "0.150s"
      },
      "response_headers_to_add": [
       {
        "header": {
         "key": "x-envoy-authxserver-endpoint-type",
         "value": "3"
        }
       }
      ]
     }

[zyfjeff]

When upstream timeout, the response should be returned by envoy, and response_headers_to_add should be understood to refer to the addition of header to the content returned by upstream.

[pugachAG]

Thank you, @zyfjeff. That indeed makes sense.

But is there a way to set header regardless of upstream success/failure?

[zyfjeff]

@pugachAG I looked at the code, the current implementation will only add the header when the upstream response is waited.

@lizan Do you think this issue can be used as an enhancement? If you can, I might take the time to implement it.

https://github.com/envoyproxy/envoy/blob/master/source/common/router/router.cc#L638

[lizan]

Yeah it could be an enhancement, note filters before router could respond to the request too, so the change might be wide. @zuercher (per TODO in the code above) thoughts?

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

[mderazon]

I think the same happens for 503 errors.
Problem with this is that in case of CORS headers not being added to the response, browser shows a CORS error in console and it often obscures the real issue that caused the 504/503.
People think it's a CORS issue, when it's really something wrong with the upstream service

[majames]

Has there been any movement on this? I'm also experiencing the timeout issue being masked by CORs errors in the browser because of this and it would be great if there was some workaround for fixing!

[mattklein123]

cc @alyssawilk this might be a good one to look at as you continue your quest to unify a bunch of handling around local replies.

[alyssawilk]

I suspect it's easy enough to move the code around and apply to the new (unified) local response path. Do you think response headers to add should be added unilaterally (runtime guarded) even on local responses, or configured for [only when proxied | all local responses]?

The other complication I can think of would be caching, making sure we have the right APIs for if headers should be added before insert or applied to cache hits as headers are sent upstream if there's no one correct answer to what users want there. cc @toddmgreer for thoughts on that.

[alyssawilk]

Alternately we could just expect folks dup their headers into the local reply content modification. if so maybe a FAQ for that?

[mattklein123]

I suspect it's easy enough to move the code around and apply to the new (unified) local response path. Do you think response headers to add should be added unilaterally (runtime guarded) even on local responses, or configured for [only when proxied | all local responses]?

My thinking was that we could have common code that both the local reply path and router call to add the response headers, based on the currently snapped/cached route. Agreed this should be runtime guarded, but IMO we could do it unilaterally. Variants of this bug have been opened a ton of times and I think it's the behavior people expect.

The other complication I can think of would be caching, making sure we have the right APIs for if headers should be added before insert or applied to cache hits as headers are sent upstream if there's no one correct answer to what users want there. cc @toddmgreer for thoughts on that.

Yeah I agree this is complicated. My thinking here though is since the cache filter is not a local reply, it would be acting as the router for cached responses, and could do what it wants in terms of adding headers before or after caching?

[alyssawilk]

So the plan would be doing route-based local header modifications and local reply mapper modifications on local responses?

I'm fine with default on with a runtime guard given if folks complain we can always make it configurable after the fact - better than starting with complexity if you don't think it's needed.

[mattklein123]

So the plan would be doing route-based local header modifications and local reply mapper modifications on local responses?

Yeah. Basically just take the code that the router already uses, and move it somewhere common.

I'm fine with default on with a runtime guard given if folks complain we can always make it configurable after the fact - better than starting with complexity if you don't think it's needed.

+1

[toddmgreer]

Yeah I agree this is complicated. My thinking here though is since the cache filter is not a local reply, it would be acting as the router for cached responses, and could do what it wants in terms of adding headers before or after caching?

If "local reply" means "reply generated by sendLocalReply", then CacheFilter doesn't send local replies. If it just means a reply based on in-process (at the time) state, then CacheFilter will often send local replies.

I don't think CacheFilter should decide whether to add custom headers before or after caching--what would it base the decision on? If you want to add a response header that will be the same for future responses from cache, add it before/upstream of CacheFilter. If the response gets cached, the added headers will be present in all responses (possibly from multiple Envoys) based on that cache entry. If you want to add a response header that may be different in every response, add it after/downstream of CacheFilter.

Perhaps it should be a HeaderInjectionFilter, and the config can say where it goes?

[mattklein123]

If "local reply" means "reply generated by sendLocalReply", then CacheFilter doesn't send local replies. If it just means a reply based on in-process (at the time) state, then CacheFilter will often send local replies.

The latter, like the router filter.

Perhaps it should be a HeaderInjectionFilter, and the config can say where it goes?

This is more complicated than I would like, given that we have had this functionality for a long time in the router filter and are trying to make it consistent for local replies. IMO it would be simpler to just say that the cache filter acts like the router and just adds the headers post-caching and pre-response.

[alyssawilk]

It may be a bit more complicated than that, as I think (may be misremembering) when doing cache validation the filter sends the upstream headers, then the cached body. If that's true we should just be careful there's no path where we apply them twice.

[mattklein123]

It may be a bit more complicated than that, as I think (may be misremembering) when doing cache validation the filter sends the upstream headers, then the cached body. If that's true we should just be careful there's no path where we apply them twice.

Good point. Perhaps there is nothing to do if we assume that the router will add the headers and they get cached?

[toddmgreer]

It may be a bit more complicated than that, as I think (may be misremembering) when doing cache validation the filter sends the upstream headers, then the cached body. If that's true we should just be careful there's no path where we apply them twice.

Good point. Perhaps there is nothing to do if we assume that the router will add the headers and they get cached?

Exactly.

[toddmgreer]

It may be a bit more complicated than that, as I think (may be misremembering) when doing cache validation the filter sends the upstream headers, then the cached body. If that's true we should just be careful there's no path where we apply them twice.

Good point. Perhaps there is nothing to do if we assume that the router will add the headers and they get cached?

Exactly.

In successful cache validation, the upstream will respond with a 304, the router filter will add custom headers, and the new custom headers will replace the previously cached ones.

[mattklein123]

Perfect, sounds like a plan. :)