http conn man: Introduce preserve_upstream_date option #11077

chradcliffe · 2020-05-05T23:59:17Z

Signed-off-by: Craig Radcliffe craig.radcliffe@broadcom.com

Commit Message:
http conn man: Introduce preserve_upstream_date option

Additional Description:
The preserve_upstream_date option allows the HTTP Connection Manager to be configured to pass through the original date header from the upstream response rather than overwriting it. The default behaviour for the date response header remains the same as before -- the header value will be overwritten by Envoy.

Risk Level: Low

Testing:

Unit testing
Manual testing

Docs Changes: N/A
Release Notes: Unsure whether this is required for a flag that defaults to current behaviour

Fixes #11030

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

repokitteh-read-only · 2020-05-05T23:59:22Z

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

🐱

Caused by: #11077 was opened by chradcliffe.

see: more, trace.

alyssawilk · 2020-05-06T12:41:37Z

Looks good overall - I'll do a full review pass once you've got CI sorted out :-)

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

htuch

/lgtm api

alyssawilk · 2020-05-06T19:05:04Z

source/common/http/conn_manager_impl.cc

@@ -1631,7 +1631,9 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
 void ConnectionManagerImpl::ActiveStream::encodeHeadersInternal(ResponseHeaderMap& headers,
                                                                bool end_stream) {
  // Base headers.
-  connection_manager_.config_.dateProvider().setDateHeader(headers);
+  if (!connection_manager_.config_.shouldPreserveUpstreamDate() || !headers.Date()) {


This PR looks great.
My only caveat is that I believe this will result in also preserving the date header when we serve out of cache.
Is your intent to do this (since at the end of the day the source of that date is upstream) or is the intent to only overwrite date headers served live from upstream (preserve date headers when they're relevant, but let cache results have an accurate up-to-date date)? I'd lean towards the latter, since I think that's a more clear interpretation of the config field, at which point I think you have to differentiate between the two.

I think the intention here would be to use a new date for serving cached content. Otherwise, we may end up with stale response dates that might be confusing to downstream components (e.g. the client).

Is there a convention around how to determine whether a cached response is being served?

Yeah, I was looking into that, and there's no an obvious direct check.

You can infer from the rc details being "via_upstream" but that's a string compare and icky. You can infer from the stream info upstreamHost being set, which is probably the easiest workaround.
I'd also be fine with something added to stream info to indicate the source of the reply - cache / Envoy generated / proxied from upstream. @mattklein123 any thoughts here?

cc @jmarantz @toddmgreer

Yeah I would think for the caching case we might want to introduce a stream info flag. This seems useful/required anyway for logging purposes in a production ready caching solution.

I can give it a try in this PR -- the "stream info flag" in this case would be a new value in StreamInfo::ResponseFlag, right?

You got it!

Responses served from cache should have the same date header they had when they got inserted into cache. The date header is defined to represent the time the message was originated, regardless of whether the message happens to spend some time in a cache.

IOW, if I request something then request it again later, and Envoy caches the first response and later serves me that entry, then the two responses I receive should have the same date header. Caches rely on this, and will compute incorrect freshness values if date headers are wrong.

This applies whether we're talking about a response served by Envoy's CacheFilter, or a response served by a separate caching proxy upstream.

The canonical way to know that a response came from an HTTP cache is to check for the presence of an age header. If a response has an age header, it's from a cache and we shouldn't change its date header.

Oh interesting.
That's not what's happening today. So in that case this PR LGTM, but someone should do the inverse and not set the date header if it passed through the cache filter. I'm happy to pick that up since I noticed the caching code fails to set rc details

The date header is defined to represent the time the message was originated, regardless of whether the message happens to spend some time in a cache.

Interesting. I guess Envoy's default behavior is wrong then? In any case, I think we can address this out of the scope of this PR per other discussion.

alyssawilk

@mattklein123 want to take a look or shall I merge?

alyssawilk · 2020-05-07T13:38:00Z

Also I think this needs a master merge, sorry

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

chradcliffe · 2020-05-07T15:53:53Z

/retest

repokitteh-read-only · 2020-05-07T15:53:55Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11077 (comment) was created by @chradcliffe.

see: more, trace.

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

mattklein123

LGTM w/ small comments. Thank you!

/wait

mattklein123 · 2020-05-07T20:12:20Z

test/common/http/conn_manager_impl_test.cc

+  setUpEncoderAndDecoder(false, false);
+  sendRequestHeadersAndData();
+  preserve_upstream_date_ = false;
+  const auto* modifiedHeaders = sendResponseHeaders(


nit: s/modifiedHeaders/modified_headers here and below (I think we can get clang-tidy to check this at some point)

mattklein123 · 2020-05-07T20:12:39Z

test/common/http/conn_manager_impl_test.cc

@@ -897,6 +899,56 @@ TEST_F(HttpConnectionManagerImplTest, RouteShouldUseSantizedPath) {
  conn_manager_->onData(fake_input, false);
 }

+TEST_F(HttpConnectionManagerImplTest, PreseveUpstreamDateDisabledDateNotSet) {


Typo "Preseve" here and below

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

…/preserve-response-date Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

mattklein123

Thanks!

mattklein123 · 2020-05-07T22:58:57Z

/azp run envoy-presubmit

azure-pipelines · 2020-05-07T22:59:06Z

Azure Pipelines successfully started running 1 pipeline(s).

mattklein123 · 2020-05-07T23:52:15Z

/azp run envoy-presubmit

azure-pipelines · 2020-05-07T23:52:23Z

Azure Pipelines successfully started running 1 pipeline(s).

This reverts commit 10c755e. Signed-off-by: Matt Klein <mklein@lyft.com>

…ard (#11132) Commit Message: http conn man: always preserve upstream date response header Additional Description: Reintroduces the change to preserve the upstream date response header (introduced in #11077, reverted in #11116 ) but removes the configuration and adds a runtime guard instead (see #11110 ) Risk Level: Low Testing: Unit testing Docs Changes: N/A Release Notes: yes Runtime guard: http_connection_manager.preserve_upstream_date Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

http conn man: Introduce preserve_upstream_date option

b3c319f

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

repokitteh-read-only bot added the api label May 5, 2020

chradcliffe marked this pull request as ready for review May 6, 2020 00:00

chradcliffe requested review from alyssawilk and mattklein123 as code owners May 6, 2020 00:00

chradcliffe marked this pull request as draft May 6, 2020 01:09

alyssawilk self-assigned this May 6, 2020

Fixing missing override with fuzz test

fc25ede

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

htuch reviewed May 6, 2020

View reviewed changes

repokitteh-read-only bot removed the api label May 6, 2020

chradcliffe marked this pull request as ready for review May 6, 2020 16:04

alyssawilk reviewed May 6, 2020

View reviewed changes

alyssawilk previously approved these changes May 7, 2020

View reviewed changes

Craig Radcliffe added 2 commits May 7, 2020 10:20

Merge branch 'master' into chradcliffe/preserve-response-date

66c55d1

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

Fixing generated protobuf

b9772d4

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

chradcliffe dismissed alyssawilk’s stale review via b9772d4 May 7, 2020 14:28

alyssawilk previously approved these changes May 7, 2020

View reviewed changes

Kick CI

060c097

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

mattklein123 self-assigned this May 7, 2020

mattklein123 requested changes May 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label May 7, 2020

Craig Radcliffe added 2 commits May 7, 2020 17:03

Spelling and style fixes

9f15487

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

Merge remote-tracking branch 'envoy-upstream/master' into chradcliffe…

5316eaa

…/preserve-response-date Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

chradcliffe dismissed alyssawilk’s stale review via 5316eaa May 7, 2020 21:11

repokitteh-read-only bot removed the waiting label May 7, 2020

mattklein123 approved these changes May 7, 2020

View reviewed changes

mattklein123 merged commit 10c755e into envoyproxy:master May 8, 2020

htuch mentioned this pull request May 8, 2020

Automatic overwrite of date header can break downstream caches #11110

Closed

mattklein123 added a commit that referenced this pull request May 8, 2020

Revert "http: Introduce preserve_upstream_date option (#11077)"

c293b94

This reverts commit 10c755e. Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 added a commit that referenced this pull request May 8, 2020

Revert "http: Introduce preserve_upstream_date option (#11077)" (#11116)

8e52a24

This reverts commit 10c755e. Signed-off-by: Matt Klein <mklein@lyft.com>

chradcliffe mentioned this pull request May 11, 2020

http conn man: default preserve_upstream_date to true, add runtime guard #11132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http conn man: Introduce preserve_upstream_date option #11077

http conn man: Introduce preserve_upstream_date option #11077

chradcliffe commented May 5, 2020

repokitteh-read-only bot commented May 5, 2020

alyssawilk commented May 6, 2020

htuch left a comment

alyssawilk May 6, 2020

chradcliffe May 6, 2020

alyssawilk May 6, 2020

alyssawilk May 6, 2020

mattklein123 May 6, 2020

chradcliffe May 6, 2020

alyssawilk May 6, 2020

toddmgreer May 6, 2020 •

edited

Loading

alyssawilk May 7, 2020

mattklein123 May 7, 2020

alyssawilk left a comment

alyssawilk commented May 7, 2020

chradcliffe commented May 7, 2020

repokitteh-read-only bot commented May 7, 2020

mattklein123 left a comment

mattklein123 May 7, 2020

mattklein123 May 7, 2020

mattklein123 left a comment

mattklein123 commented May 7, 2020

azure-pipelines bot commented May 7, 2020

mattklein123 commented May 7, 2020

azure-pipelines bot commented May 7, 2020

http conn man: Introduce preserve_upstream_date option #11077

http conn man: Introduce preserve_upstream_date option #11077

Conversation

chradcliffe commented May 5, 2020

repokitteh-read-only bot commented May 5, 2020

alyssawilk commented May 6, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddmgreer May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented May 7, 2020

chradcliffe commented May 7, 2020

repokitteh-read-only bot commented May 7, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented May 7, 2020

azure-pipelines bot commented May 7, 2020

mattklein123 commented May 7, 2020

azure-pipelines bot commented May 7, 2020

toddmgreer May 6, 2020 •

edited

Loading