Envoy: Customize response code on upstream reset

[jakubdyszkiewicz]

Hello,

we would like to customize the response code of upstream reset. Right now, we see that 503 is hardcoded in Filter::onUpstreamReset

Motivation

Status code
Without Envoy, when app A was communicating with B and was not available, the A's client received connection refused. With Envoy in place, the A will receive 503 status code. We would like to differentiate this behaviour with different status code, for example 504 or 502, so our developers that this is not a problem with B itself throwing 503, but with a connection to B.

If you think that makes sense, we are ready to contribute it, but need a guidance where to put a parameters for this.

[mattklein123]

I think this is a dup of https://github.com/envoyproxy/envoy/issues/1178. Can we track there?

[jakubdyszkiewicz]

Ok, the response body part is definitely duplicate. I used search, but I guess I typed wrong keywords.

But I think the hardcoded status code is a different problem. What do you think?

[mattklein123]

Sure fair enough. Can you update the title/text to clarify that you are looking for response code customization?

[lukidzi]

You can assigne this issue to me. Could you suggest where should I add configuration for custom code and response message?

[mattklein123]

I would take a look at the router filter configuration most likely. If we want to do this in a more general sense we could like at HTTP connection manager (HCM) level configuration for send local reply?

[lukidzi]

Ok. So I thought about structure of config and I came up with something like this:

http_connection_manager_config
  ...
  "http_filters": [],
  "send_local_reply_config":
      - "match" : 
            "status" : 503,
            "reason" : "remote reset"
        "rewrite" : 
            "status" : 504

I am a bit worried how to distinguish between remote reset and other errors at sendLocalReply(or should I check it in router.cc ?- not generic) method because we receive code and body(I can look for reason in body or maybe pass reason to sendLocalReply?). Maybe do you how some suggestion what would be the best solution?
I am not sure if it is good approach but it might be easier to extend this config for task with custom message and json format.

[mattklein123]

@lukidzi that is roughly the type of config that I have been considering also. I think we can do other things in that config such as JSON vs. text, etc.

[mattklein123]

See also https://github.com/envoyproxy/envoy/issues/6166 which is related and I would like to handle in the same config.

[euroelessar]

I am a bit worried how to distinguish between remote reset and other errors at sendLocalReply

Can we use StreamInfo::hasResponseFlag method for it? (assuming the enum is somehow represented in protobuf), e.g. smth like:

"send_local_reply_config":
- "match": 
  "status": [503],
  "response_flags":
    "all":
    - "UPSTREAM_REMOTE_RESET"
  "rewrite": 
    "status": 504

On a side note, we can accommodate #1178 and #6166 by using custom rewrite rules:

- "match":
    "status": [500, 502, 503]
  "rewrite":
    "body":
      "inline_string": "internal error (you definitely did not crash our servers)"
# or
- "match":
    "prefix": "/api"
  "rewrite":
    "json_body": # this field may be required to be dynamic, similar to access log json support
      "error": "%BODY%"

[euroelessar]

Looking at the protobuf definitions, it looks like access log defines the whole new structure to pass response flags around: https://github.com/envoyproxy/envoy/blob/ecd03a4eed07e1cfea9e9844e519b7fffada437a/api/envoy/data/accesslog/v2/accesslog.proto#L151

[mattklein123]

@euroelessar one thing I would like to avoid here is "yet another matching config." I know @mergeconflict conflict has been thinking about this a bit. If we want to implement matching it would be nice to reuse access log filters, tap matchers, or even part of the routing table config. Can you take a look at all of those?

Also, I think this would benefit from a full design doc. When you have investigated can you create a public gdoc that we can iterate on? Thank you!

[mergeconflict]

@euroelessar have a look at my design doc for unified matchers, if you're interested: https://docs.google.com/document/d/14CAt13uAlrCPPAh3dL06JL0FrKRh8VKJfjCg6HV7p3c. I haven't been actively working on this project, but the doc isn't badly out-of-date yet.

[lukidzi]

I've implemented this feature but it will require some changes to reuse existing code. Should I open a PR and wait for some feedback with right solution/suggestions?

[mattklein123]

@lukidzi do you work with @euroelessar? Can the two of you collaborate on a short design doc on the configuration we want?

[lukidzi]

We don't work together. I'll try to create some doc with analysis of existing solutions and how we can reuse them also I'll try to find some common solution for existing code.

[lukidzi]

I've created a doc with config proposition: https://docs.google.com/document/d/19M6rYpk_rwOeVMM93cmxDwSe__KpzA9g-FWVUaxUElo/edit?usp=sharing. Feel free to comment

[mattklein123]

@lukidzi at a high level this is on the right track. I would say let's start to implement and we can discuss in code review. Please start with the API first and let's review that before you do a bunch of implementation. Note that as I already mentioned I want to make sure that https://github.com/envoyproxy/envoy/issues/6166 fits into this also (https://github.com/envoyproxy/envoy/pull/6261). cc @zyfjeff