router: add config to send date header with requests

[rebello95]

Adds a config option to the router for adding a date header to outbound requests, per the RFC.

Consumers can enable this option to have Envoy send the date header with outbound requests and retries (disabled by default).

Note that this header is updated on each subsequent retry.

Risk Level: Low, behind config option defaulting to false
Testing: Unit tests.
Docs Changes: None

Signed-off-by: Michael Rebello me@michaelrebello.com

[repokitteh-read-only]

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: #11915 was opened by rebello95.

see: more, trace.

[murki]

we'll want functionality for customizing the date format to another acceptable ISO format

As per the RFC "HTTP/1.1 clients and servers [...] MUST only generate the RFC 1123 format for representing HTTP-date values in header fields." So I don't know how much sense it would make to support generating the date other formats that deviate from the RFC. Example of the RFC 1123 format:

Sun, 06 Nov 1994 08:49:37 GMT  ; RFC 822, updated by RFC 1123

[rebello95]

@murki great call out. +1 to that comment; I updated the PR description.

[asraa]

Thanks! Some minor comments first.

[rebello95]

Thanks for the review @asraa - I updated per your comments. Also pushed some changes to ensure that the header gets updated on each retry (as was my original intent).

[asraa]

Thanks for the code changes. I think the config knob makes sense. About the retry behavior, I'd like a closer look at. RFC 2616 says

The HTTP-date sent in a Date header SHOULD NOT represent a date and time subsequent to the generation of the message. It SHOULD represent the best available approximation of the date and time of message generation, unless the implementation has no means of generating a reasonably accurate date and time. In theory, the date ought to represent the moment just before the entity is generated. In practice, the date can be generated at any time during the message origination without affecting its semantic value.

which I interpret to mean that on retries the original date is used. I think if the behavior deviates slightly from core it should live in a filter, but I don't think there is retry behavior in filters yet. @PiotrSikora do you have any context about this?

[rebello95]

Regarding retries, I think it depends on whether we consider a "request" to be request + all subsequent retries, or if each retry is its own "request". I'd lean towards the latter, as I consider a retry to be an implementation detail of how a request is being transmitted.

As you mentioned, the RFC does say:

In theory, the date ought to represent the moment just before the entity is generated.

If we consider each retry as its own request, I think updating the date header on each retry is compliant with the RFC.

For context, the use case we have for this change is ensuring the date header represents the "time at which the request was transmitted to the server" in order to calculate some deltas.

I don't think there is retry behavior in filters yet

#10455 is tracking some of this behavior (adding the ability to insert filters ahead of retries). Completely agree with you that this logic could (should?) be moved there once functionality becomes available.

[asraa]

Thanks! It makes sense to reflect the most accurate date on the client-side retry in that case.

Since pluggable retry extensions aren't a thing yet and this can't live in extensions, let's go ahead with this for now.

[asraa]

Has approval from my end, can a senior maintainer take a final pass? @envoyproxy/senior-maintainers

[PiotrSikora]

@PiotrSikora in what sense is each retry request related to the first request? Why can't they be viewed as independent requests, given they might land on completely different upstream endpoints and be generated potentially far apart with long retry delays (e.g. minutes, hours (?)).

Proxy is an intermediate that forwards the request between the client and (one of) the backend server(s). Since this is happening hop-by-hop, it ends up being a chain of request/response pairs, but ultimately, the request that's being forwarded to the backend server is effectively the original request from the client, not a new or independent request, and the response to it will be forwarded back to the client, not consumed by the proxy... If the client disconnects in the middle of sending request, then the request to the backend server will be either terminated or cancelled (when possible).

The retries are simply reattempts to deliver the same original request that was generated by the client, and not by the proxy, so I don't see how they could be perceived as something unrelated to it.

Does this answer your question?

[wrowe]

ultimately, the request that's being forwarded to the backend server is effectively the original request from the client

But the Date response has nothing to do with the request, it is the response content handling or generation metadata. Please review rfc7231 section 7.1.1.2

[PiotrSikora]

But the Date response has nothing to do with the request, it is the response content handling or generation metadata. Please review rfc7231 section 7.1.1.2

Thanks, I'm familiar with that RFC... but this PR is about adding Date field to the outgoing requests, not responses.

[rebello95]

The retries are simply reattempts to deliver the same original request that was generated by the client, and not by the proxy, so I don't see how they could be perceived as something unrelated to it.

I do see your point, but to clarify: The use case for this particular change is for usage within Envoy Mobile, in which case Envoy is the client, rather than acting as a proxy. I.e., it's the origin (client) hop setting this header, rather than an upstream proxy.

[PiotrSikora]

I do see your point, but to clarify: The use case for this particular change is for usage within Envoy Mobile, in which case Envoy is the client, rather than acting as a proxy. I.e., it's the origin (client) hop setting this header, rather than an upstream proxy.

OK, if Envoy Mobile is originating the request, then I guess that's fine (though, since it's a retry and not a new request, I'd personally still retain the original Date, but that's a matter of opinion and my original argument isn't valid here).

The one remaining issue is that this is being added to Envoy Proxy APIs and exposed to it's users, introducing (IMHO) the incorrect behavior to intermediates. Is there a way to gate this feature to be available only in Envoy Mobile?

[htuch]

@rebello95 stats_integration_test on release seems like a real failure. LGTM once that is resolved.
/wait

[rebello95]

@rebello95 stats_integration_test on release seems like a real failure. LGTM once that is resolved.

@htuch Thanks for checking. I ran the integration test locally and it passed, but I do see a note mentioning that this validation is only done on release builds and requires a CI failure to know how to update it. However, I am not familiar with what part of this PR would cause that number to increase - am I missing something?

The one remaining issue is that this is being added to Envoy Proxy APIs and exposed to it's users, introducing (IMHO) the incorrect behavior to intermediates. Is there a way to gate this feature to be available only in Envoy Mobile?

@PiotrSikora Unfortunately not that I'm aware of since retries live in the router filter. As I mentioned above, #10455 is tracking the ability to add additional filter functionality ahead of where retries are done, at which point we could probably move this into Envoy Mobile itself. Does that make sense?

[wrowe]

this PR is about adding Date field to the outgoing requests, not responses

Gotcha, sorry for my confusion. Then the date the user agent presents to envoy, or the initial date when envoy first composed the proxy request should be preserved throughout any retries.

[htuch]

@rebello95 Not sure either; maybe the change to the O(1) headers that included Date? I don't recall seeing clusters having a header map associated. @jmarantz WDYT re stats_integration_test failure?

If you want to run release locally under Docker, you can do ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.release'

[rebello95]

@rebello95 Not sure either; maybe the change to the O(1) headers that included Date? I don't recall seeing clusters having a header map associated. @jmarantz WDYT re stats_integration_test failure?

Hm yea I'm not super familiar with this area. I assume these changes would be a no-op since the new option to add the date header defaults to false, but I could be mistaken. Any insights you can provide @jmarantz would be much appreciated 😃

[mattklein123]

Thanks for working on this. A few high level comments. If we go with this this will need release notes. Thank you!

/wait

[mattklein123]

I think it's fine to add this, but per @PiotrSikora can you maybe add some text here about the potential misuse?

Also, I wonder if we should make this an enum with the default being off, a value being all requests and retries, with the option to later add a third enum that would add it on the initial request but not retries? That might also future proof it better.

[mattklein123]

This config option is forever effectively. If we think this is not the right place for this ultimately, we shouldn't add this at all. Per @PiotrSikora I do wonder whether there is a way you could hack this somehow in envoy mobile if you think we want to move this. The hack would be to grab the created API listener and using dynamic_casts get at the underlying impl and toggle a bool?

[rebello95]

The fact that the config option will become a part of the public API forever is a fair call out. I'm pausing on this for now while I sync up with our team internally to see if there are other options aside from upstreaming this change (including if your workaround suggestion could solve the issue). Thanks.

[PiotrSikora]

One option would be to keep the add_request_date_header option as part of the public API (since it's a valid behavior), which would only add the Date request header to the original request (if absent) and retain the same value across all retries in Envoy Proxy, but which would keep updating the Date request header in Envoy Mobile.

[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!