Update Envoy to f95f5391b0b8683081ec786ea946026594955fc6#562
Update Envoy to f95f5391b0b8683081ec786ea946026594955fc6#562dubious90 merged 6 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@jiajunye can you please review this and assign back to me once done? Also let me know if you have any questions. |
jiajunye
added
waiting-for-changes
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
added
waiting-for-review
|
LGTM |
dubious90
left a comment
dubious90
left a comment
There was a problem hiding this comment.
Mostly looks good. Just two nits.
| name = "envoy", | ||
| sha256 = ENVOY_SHA, | ||
| strip_prefix = "envoy-%s" % ENVOY_COMMIT, | ||
| # // clang-format off: Envoy's format check: Only repository_locations.bzl may contains URL references |
There was a problem hiding this comment.
This seems to be the first instance where us just using envoy's .bazelrc isn't great for us. No action required here on this PR, but we should keep an eye on it.
source/client/stream_decoder.cc
Outdated
| const Envoy::Http::HeaderMap::GetResult& timing_header = | ||
| response_headers_->get(timing_header_name); | ||
| if (!timing_header.empty()) { | ||
| absl::string_view timing_value = timing_header[0]->value().getStringView(); |
There was a problem hiding this comment.
This could possibly use a comment or maybe even an assertion of sorts. It looks like the envoy function was changed to now allow multiple HeaderEntries to be returned rather than one?
I gather that our use case would always expect only one - would it be reasonable to add a failure here if we received more than one?
There was a problem hiding this comment.
I added a sparse warning log line in dc0b618 and also testing for that case to make sure we don't track timings when there's ambiguity because of multiple header/value pairs.
| const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig); | ||
| if (request_config_header) { | ||
| const auto& request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig); | ||
| if (request_config_header.size() == 1) { |
There was a problem hiding this comment.
should we have an else condition here now? again, taking into consideration receiving more than 1
There was a problem hiding this comment.
Yes, you're right. Done in dc0b618 + explicit tests for the case.
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
test/server/http_filter_base_test.cc
Outdated
|
|
||
| TEST_P(HttpFilterBaseIntegrationTest, SingleValidPlusEmptyConfigurationHeadersFails) { | ||
| // Make sure we fail when both a valid configuration plus an empty configuration header is send. | ||
| // Note that we could be more flexible and look for the first request header that has a value, |
There was a problem hiding this comment.
This note seems like it belongs more in the function itself rather than in this test.
Also, I might encourage a rephrase of why our approach is reasonable. Without understanding of a real use case for them, we are assuming that any existence of duplicate headers here is in error.
There was a problem hiding this comment.
Amended this; also updated the Envoy dep to f95f5391b0b8683081ec786ea946026594955fc6 in here, as that didn't need require any changes on our side modulo updating the revision/sha in .bazelrc.
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
changed the title
|
/retest |
|
🔨 rebuilding |
3 participants
Notable changes:
Envoy::Http::StreamResetReason:: ConnectError& amend related testEnvoy::Http::HeaderMap::get()now returns aEnvoy::Http::HeaderMap::GetResult.bazel/repositories.bzlSigned-off-by: Otto van der Schaaf oschaaf@we-amp.com