WiP: Local reply mapper

[lukidzi]

As mentioned in #7537 I've created this PR to discuss solution. I haven't added json logs config in this PR because I want to do it as separate task.
Rest of the changes apart from api were created just to check if config is loaded properly(for my usage).

I would like to start implementation as soon as we create nice and resuable config.

I need some guidance where should I place feature related files. I thought about either new package common/local_reply or common/http but I'm open for suggestion.

[mattklein123]

Thanks for working on this, some initial API comments for discussion. Very excited to see this being worked on.

/wait

[mattklein123]

Why is this needed?

[lukidzi]

I've added this because ResponseFlags wasn't visible in proto.

[mattklein123]

I really, really, really would like to unify our HTTP match capabilities, but maybe we defer this to v4 as I think any other solution will be too disruptive. @htuch any thoughts here?

[htuch]

I think that we should tackle this in v4, but meanwhile, for new mathers, can we align them behind either access log or TAP matchers as the basis (even if only subsetted)? I'd prefer not to add yet another matcher format (and surrounding code cruft for parsing and executing match expressions).

[lizan]

My high level question here is do we really need a match/rewrite semantics here? Since local reply is the output from Envoy (either from core HCM, or extensions), can we change the code of local reply to emit some structured data, and map it to the actual response based on config? A Rewrite here seems based on how current local reply works.

[lukidzi]

I am not sure if we want always to map some response. I thought that it would be nice to just map specifice responses to another one like it has been written in #7537.

[mattklein123]

IMO it's a bit easier to allow the operator to have generic rules for local replies versus having to account for every possible location where a reply is generated, but I could definitely be convinced otherwise.

[lizan]

I understand the context of #7537, but I'd like this to be generic enough to make it possible to convert all local reply to JSON. Particularly can Rewrite be some sort of formatter instead?

Rewrite proposed here seems based on how current local reply works (i.e. take what current sendLocalReply send, and rewrite the response header/body), instead can we just make sendLocalReply emit what formatter says for those?

[mattklein123]

Yeah agreed this makes sense to rethink rewrite is some combination of both rewriting fields as well as a final format phase?

[repokitteh-read-only]

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

🐱

Caused by: #8126 was synchronize by lukidzi.

see: more, trace.

[lukidzi]

/wait

[lukidzi]

/wait

[lizan]

Tagging @zyfjeff worked on similar thing previously #6261

[lukidzi]

Ok. Thanks for feedback I'll try to come up with new proposition.

[lukidzi]

/wait

[lukidzi]

I've removed changes with matcher/rewrite and added formatter which reuse access_log code to create json format. Configuration looks same as for access logs. This is just a concept and I will be glad to hear your opinions.

[mattklein123]

I think I'm missing something. How does this configuration work exactly?

/wait-any

[lukidzi]

This configuration allows to define different formats e.g: json format, text format, or default.

Text format - requires pattern with key words from access log formatter: https://www.envoyproxy.io/docs/envoy/latest/configuration/observability/access_log#config-access-log-format-dictionaries

Default - won't change current behaviour.

Json format - you can specify config key:value(same as json_format for access logs) where key is json field name and value can be some constant string, keywords from above url which will be resolved to values like for access logs or %RESP_BODY% which will be resolved to response body, example config which works with this code(I've implemented json_format to check if it works, rest isn't implemented):

              local_reply_config:
                json_format:
                      foo: "test"
                      timestamp: "%START_TIME(%FT%T.%3fZ)%"
                      level: TRACE
                      logger: envoy
                      request_protocol: "%PROTOCOL%"
                      request_method: "%REQ(:METHOD)%"
                      request_authority: "%REQ(:authority)%"
                      request_path: "%REQ(:PATH)%"
                      response_body: "%RESP_BODY%"

I reused accesslog config/formatter code to achieve that.

Example response:

curl localhost:31001/status/info -v | jq                                                                                                                                                                       
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0*   Trying ::1...
* TCP_NODELAY set
* Connection failed
* connect to ::1 port 31001 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 31001 (#0)
> GET /status/info HTTP/1.1
> Host: localhost:31001
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 503 Service Unavailable
< content-length: 318
< content-type: application/json
< date: Thu, 12 Sep 2019 18:17:00 GMT
< server: envoy
<
{ [318 bytes data]
100   318  100   318    0     0   2516      0 --:--:-- --:--:-- --:--:--  2523
* Connection #0 to host localhost left intact
{
  "response_body": "upstream connect error or disconnect/reset before headers. reset reason: connection failure",
  "request_authority": "localhost:31001",
  "request_path": "/status/info",
  "request_protocol": "HTTP/1.1",
  "timestamp": "2019-09-12T18:17:00.785Z",
  "level": "TRACE",
  "foo": "test",
  "logger": "envoy",
  "request_method": "GET"
}

[mattklein123]

I see this makes sense and is pretty slick. But don't we still then potentially need matching and rewriting as a parallel set of options? cc @ahedberg @lizan for comment

/wait-any

[lukidzi]

I've added mapper config to allows response_code mapping based on response_flags. I've reused AccessLogFilter to achieve that. Here is sample config which I tested:

              local_reply_config:
                mapper:
                  matcher:
                    response_flag_filter:
                      flags:
                        - LH
                        - UH
                  rewriter:
                    status_code: 504
                json_format:
                      foo: "test"
                      timestamp: "%START_TIME(%FT%T.%3fZ)%"
                      level: TRACE
                      logger: envoy
                      request_protocol: "%PROTOCOL%"
                      request_method: "%REQ(:METHOD)%"
                      request_authority: "%REQ(:authority)%"
                      request_path: "%REQ(:PATH)%"
                      response_body: "%RESP_BODY%"

[lizan]

the config looks reasonable, any chance the matcher part can be unified with access logger filter?

for the rewriter part we may need support grpc-status / grpc-message / grpc-status-details-bin, but you may leave that as a TODO for now.

[lukidzi]

Changed to AccessLogFilter. If it looks ok for you I will try to implement this and if I have problems I will ask for some guidance.

[mattklein123]

@lukidzi can you potentially just cleanup this PR and push only the final API and docs? I would like us to agree on that before implementation. Thank you!

[ahedberg]

Sorry for the delay. I think the current design should let us achieve what I was requesting in #8214. cc @alyssawilk

[lukidzi]

Added docs and removed rest of changes. If I should be more specific and place them somewhere else I will fix it in next commit. Thanks !

[alyssawilk]

Yeah, I think this hits 2 things we want - custom error codes and custom error messages.
My one concern is we might have the knobs we need, but if the flags aren't enough maybe we can add in the response details into matcher options.

[mattklein123]

In the final version of the PR can you flesh these docs out to cross-link to the format rules, etc.? It's not super clear how the user is supposed to fill these fields out. Thank you!

/wait

[htuch]

Looks great, thanks for iterating on this.
/wait

[htuch]

Can this just be uint32?

[lukidzi]

I think it will be good to keep this field optional if in future someone want to add e.g: headers, body.

[htuch]

Nit: "The client will iterate through mappers..`

[htuch]

Can you rephrase the comments in this file to make it generic, i.e. it might be used in request, response, aribtrary config, etc. It's just a way of specifying some data that might be either structured JSON or flat plain text.

[lukidzi]

/wait

[lukidzi]

Do you think it is good time to start the implementation part or configuration requires some changes?

[mattklein123]

IMO this API looks very good and I think we can start on the implementation. I think there are more docs that are needed but we can cover that later as part of the main review. @htuch any further fundamental API comments?

/wait

[htuch]

@lukidzi @mattklein123 API proposal LGTM.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!