router: add config to reject requests with invalid envoy headers by xyu-stripe · Pull Request #7323 · envoyproxy/envoy

xyu-stripe · 2019-06-19T14:24:26Z

Description:
Before this change, Envoy would ignore invalid x-envoy-* header values from a
client request (e.g. x-envoy-max-retries: 3.0), silently falling back to the
default value for that header. Introduce a strict_check_headers config option
for envoy.router that adds optional support to explicitly reject requests with
invalid values for the following headers:

x-envoy-upstream-rq-timeout-ms
x-envoy-upstream-rq-per-try-timeout-ms
x-envoy-max-retries
x-envoy-retry-on
x-envoy-retry-grpc-on

On rejection, Envoy responds with HTTP status 400 and sets a new response flag
IH to indicate the reason was due to an invalid header.

Risk Level: Low/medium

Testing: unit tests

unit test: FilterUtility::StrictHeaderChecker
test that router rejects request with HTTP status 400 + sets the IH response flag
test that config validation rejects unsupported values
manual end-to-end test client -> envoy -> upstream server to verify that
Envoy returns a 400 and sets the response flag in the logs

Docs Changes:

add inline docs to router.proto for strict_check_headers
add inline docs to accesslog.proto for IH response flag

Release Notes: updated for router and accesslog
Fixes #6482

Description: Before this change, Envoy would silently ignore the `x-envoy-*` header if a client specifies an invalid value for this header (e.g. `x-envoy-max-retries: 3.0`). Introduce a `strict_check_headers` config option for `envoy.router` that adds optional support to reject requests with invalid values for the following headers: - x-envoy-upstream-rq-timeout-ms - x-envoy-upstream-rq-per-try-timeout-ms - x-envoy-max-retries - x-envoy-retry-on - x-envoy-retry-grpc-on On rejection, Envoy responds with HTTP status 400 and sets a new response flag `IH` to indicate the reason was due to an invalid header. Risk Level: Low/medium Testing: unit tests - unit test: `FilterUtility::StrictHeaderChecker` - test that router rejects request with HTTP status 400 + setting the `IH` response flag - test that config validation rejects unsupported values - manual end-to-end test `client -> envoy -> upstream server` to verify that Envoy returns a 400 and sets the response flag in the logs Docs Changes: - add inline docs to `router.proto` for `strict_check_headers` - add inline docs to `accesslog.proto` for `IH` response flag Release Notes: updated for router and accesslog Fixes envoyproxy#6482 Signed-off-by: Xiao Yu <xyu@stripe.com>

xyu-stripe · 2019-06-19T16:47:29Z

(6/20)
ci-release seems to indicate that this change increased cluster memory consumption by 64 bytes -- a suspiciously round number...

This isn't reproducible on macosx via bazel test --nocache_test_results //test/integration:stats_integration_test
Next, I'll try ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.release //test/integration:stats_integration_test'

Test failure output:

[ RUN      ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithStats/IPv4
test/integration/stats_integration_test.cc:211: Failure
Expected equality of these values:
  m_per_cluster
    Which is: 49476
  49412
[  FAILED  ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithStats/IPv4, where GetParam() = 4-byte object <00-00 00-00> (1146 ms)
[----------] 1 test from IpVersions/ClusterMemoryTestRunner (1146 ms total)

xyu-stripe · 2019-06-19T16:51:18Z

/retest

repokitteh-read-only · 2019-06-19T16:51:22Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #7323 (comment) was created by @xyu-stripe.

see: more, trace.

xyu-stripe · 2019-06-19T17:58:22Z

/retest

repokitteh-read-only · 2019-06-19T17:58:27Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #7323 (comment) was created by @xyu-stripe.

see: more, trace.

snowp

Thanks, this seems like a reasonable contribution.

I think you got this answered in Slack but it seems like this change caused an increase the amount of memory used per cluster, so we'll want to understand where that's coming from and ideally avoid this increase unless this feature is explicitly enabled.

api/envoy/config/filter/http/router/v2/router.proto

snowp · 2019-06-20T22:41:33Z

include/envoy/stream_info/stream_info.h

  UpstreamRetryLimitExceeded = 0x8000,
  // Request hit the stream idle timeout, triggering a 408.
  StreamIdleTimeout = 0x10000,
+  // Request specified headers that were invalid under strict header checking


nit: end comments with period

snowp · 2019-06-20T22:42:02Z

source/common/json/config_schemas.cc

    "properties" : {
      "dynamic_stats" : {"type" : "boolean"},
-      "start_child_span" : {"type" : "boolean"}
+      "start_child_span" : {"type" : "boolean"},


the JSON API is deprecated, no need to add anything here

Oh that's great to know! Will remove this.
Curious where I could have found this -- I don't think I saw anything in the release notes about the JSON API.

This is used for the V1 JSON conversion, V1 was deprecated in 1.9: https://github.com/envoyproxy/envoy/blob/master/docs/root/intro/version_history.rst#190-dec-20-2018

The V2 JSON api is just using the protobuf provided utilities for serialization, so this code isn't needed anymore

source/common/router/retry_state_impl.cc

snowp · 2019-06-20T22:46:17Z

source/common/router/router.h

+                                        const Http::LowerCaseString& target_header);
+
+  private:
+    static HeaderCheckResult has_valid_retry_fields(Http::HeaderEntry* header_entry,


camelCase for function names

snowp · 2019-06-20T22:50:44Z

source/common/router/router.cc

+FilterUtility::StrictHeaderChecker::test(Http::HeaderMap& headers,
+                                         const Http::LowerCaseString& target_header) {
+  HeaderCheckResult r;
+  auto all = Http::Headers::get();


I would just inline the occurrences of Http::Headers here, the indirection is making this a bit harder to read imo (and we're making an extraneous copy here).

source/common/router/router.cc

snowp · 2019-06-20T22:53:14Z

source/common/router/router.cc


+  if (!config_.strict_check_headers_.empty()) {
+    for (const auto& header : config_.strict_check_headers_) {
+      auto res = FilterUtility::StrictHeaderChecker::test(headers, header);


nit: const where possible (here and for the body below)

snowp · 2019-06-20T22:55:05Z

source/common/router/router.h

+                                                    uint32_t (*parseFn)(absl::string_view)) {
+      HeaderCheckResult r;
+      if (header_entry) {
+        auto flags = parseFn(header_entry->value().getStringView());


nit: const

snowp · 2019-06-20T22:55:25Z

source/common/router/router.h

+
+  private:
+    static HeaderCheckResult has_valid_retry_fields(Http::HeaderEntry* header_entry,
+                                                    uint32_t (*parseFn)(absl::string_view)) {


using std::function tends to make passing functions around more readable

How about the following:
using ParseRetryFlagsFunc = std::function<uint32_t(absl::string_view header)>;

I don't love the name, so would be open to consider other suggestions!

xyu-stripe

@snowp Thanks for your feedback! I have a few follow-up questions, but am good to re-push after I hear from you.

Re: MemoryLargeClusterSizeWithStats test failure indicating a cluster size increase by 64 bytes...
It seems that the culprit is adding a new std::vector<Http::LowerCaseString> member to Envoy::Router::FilterConfig

Per @bobby-stripe and @jmarantz's suggestion: I made EXPECT_MEMORY_EQ log the value in the skipped and non-skipped cases and verified m_per_cluster locally on macosx:

Notes:
* "baseline"  : before this PR
* "char[N]"   : add a `char buf_[N]` member to Envoy::FilterConfig
* "+value"    : change in consumed bytes compared to baseline

      baseline   char[21] char[22]  PR#7323
      --------   -------- --------  -----------
ipv4: 49391      +0       +64       49453 (+64)
ipv6: 49572      +0       +64       49636 (+64)

How might we move forward with this? Should we move this state into the StrictHeaderChecker ctor and only instantiate one if the config.strict_check_headers() is non-empty? Open to suggestions on clean ways to approach this.

snowp · 2019-06-21T17:56:10Z

I think one thing you could try is to use absl::optional and have it be absl::nullopt when the list is empty, iirc that will incur zero heap memory usage when its unset.

- JSON API is deprecated: remove `strict_check_headers` from `source/common/json/config_schemas.cc` and related test - docs: document the specific demands of strict header validation - router.{cc, h}: - `const` where applicable - use `std::function` - inline `Http::Headers::get()` Signed-off-by: Xiao Yu <xyu@stripe.com>

… + assert - clear RETRY_UNKNOWN_FIELD_ERROR when parsing `retry_on` config field - `StrictHeaderChecker::test` assert failure if target header is unsupported Signed-off-by: Xiao Yu <xyu@stripe.com>

…ithStats - Use std::unique_ptr for Envoy::Router::FilterConfig::strict_check_headers_ and only allocate it when we parse values from the config. Signed-off-by: Xiao Yu <xyu@stripe.com>

Failing error: [readability-container-size-empty,-warnings-as-errors] - source/common/router/router.h:163:9: error: the 'empty' method should be used to check for emptiness instead of 'size' - call strict_check_headers.empty() rather than Protobuf::RepeatedPtrField::size() Signed-off-by: Xiao Yu <xyu@stripe.com>

xyu-stripe · 2019-06-23T17:52:40Z

@snowp Thanks for your feedback -- repushed to address your feedback.

This latest push makes ci/release pass by making Envoy::Router::FilterConfig::strict_check_headers_ a unique_ptr (as in this diff). If I'm reading the results from the simple m_per_cluster test correctly, then on my machine adding 21 additional bytes to FilterConfig bumps cluster memory consumption into the next 64-byte aligned chunk.

You had suggested:

I think one thing you could try is to use absl::optional and have it be absl::nullopt when the list is empty, iirc that will incur zero heap memory usage when its unset.

I tried this approach here, and did not observe a change to the per-cluster consumed bytes (i.e. still +64 bytes in local macosx tests)
While Abseil tip #123 does indicate that absl::optional doesn't incur any heap allocation overhead (i.e. header for allocated blocks etc.), it seems to indicate that nullopt and optional are equivalent in the empty case (emphasis mine):

Because the object is stored inline, the usual caveats about allocating large objects on the stack apply, just like for a bare object. Also be aware that an empty absl::optional uses as much memory as a populated one.

I think the default decision is whether to keep strict_check_headers_ as a unique_ptr or to bump the threshold for the test by 64 bytes? If we go the unique_ptr route:

MemoryLargeClusterSizeWithStats doesn't actually instantiate configs that use strict_check_headers, so for this test we only incur the cost to store the pointer, which doesn't bump memory usage up.
The downside is now strict_check_headers_ is a bit harder to use and requires us to check for nullness + actually incurs heap allocation on use. WDYT?

snowp · 2019-06-24T16:14:29Z

Another option might be to store the strict headers in a bitmap (similar to how the retry flags are tracked). That way we're tracking which flags are set with minimal memory overhead (no redundant string copies). It does come at some code complexity, but it might be the right thing to avoid storing a new list of strings per router?

xyu-stripe · 2019-06-24T16:55:46Z

Another option might be to store the strict headers in a bitmap (similar to how the retry flags are tracked). That way we're tracking which flags are set with minimal memory overhead (no redundant string copies). It does come at some code complexity, but it might be the right thing to avoid storing a new list of strings per router?

I don’t feel strongly about this implementation choice and am happy to do whatever makes the most sense. I don’t think I have enough context to make this judgment call though — do you think it would make sense to tag someone who could help us tie-break on approach here? I'm glad the memory consumption test is serving its purpose of helping us consciously think about memory footprint and to consider alternatives here!

snowp · 2019-06-24T16:59:01Z

The memory increase is making this change somewhat involved, so just trying to get some consensus on approach before investing too much time into one solution:

For context: the PR originally stored the headers to sanitize as a std::vector<LowercaseString> on the router, which caused the memory test to fail.

To solve this two options have been suggested:

store a unique_ptr to the vector. This lets us store nullptr when the feature isn't used, which keeps the memory down in the default case.
store a bitset of headers to sanitize (similar to how retry reason or health flags are stored). This comes with come additional complexity around mapping strings to bitset values and back, but would keep both default and overall memory used down, as no redundant string copies are kept.

@mattklein123 @jmarantz thoughts here?

mattklein123 · 2019-06-24T20:42:04Z

IMO (2) sounds good to me, but (1) is easy and also fine if we want to start there with a TODO.

xyu-stripe · 2019-06-25T05:43:15Z

IMO (2) sounds good to me, but (1) is easy and also fine if we want to start there with a TODO.

Since the feature is off by default, I propose we start with (1) + add a TODO comment. Once this PR is merged, I can make a small follow-up PR to optimize the representation via (2). WDYT? @snowp @mattklein123

mattklein123 · 2019-06-25T13:50:42Z

👍

jmarantz · 2019-06-25T14:29:59Z

re unique_ptr vs bitset...there are less than 10 supported headers, so I like the idea of a bitset (or maybe even a uint8_t and some bit-mask constants is likely enough). But am also fine to check in the unique_ptr<vector> and iterate.

…bitset - add TODO comment - alias the unique_ptr type Signed-off-by: Xiao Yu <xyu@stripe.com>

xyu-stripe · 2019-06-25T20:20:46Z

@snowp Re-pushed to add a TODO comment to change to a bitset (will do in next PR).
Looks like ci and DCO checks are sorted now. PTAL?

snowp

Thanks for iterating on this!

snowp · 2019-06-26T14:57:31Z

api/envoy/config/filter/http/router/v2/BUILD

@@ -1,4 +1,4 @@
-load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal")
+load("@envoy_api//bazel:api_build_system.bzl", "api_go_proto_library", "api_proto_library_internal")


Is this change needed?

Added this because we want to validate values set for strict_check_headers in the config in api/envoy/config/filter/http/router/v2/router.proto#L14, which requires inclusion of com_envoyproxy_protoc_gen_validate/validate/validate.proto.

I'm no bazel expert but I thought this was only necessary if you directly referenced the api_go_proto_library rule in this BUILD file which it doesn't seem like you're doing? I could be totally wrong here, so feel free to leave it if it's necessary for the build

Will repush with this removed and add it back if it turns out to actually result in failed builds.

Hmm.. I rebuilt //source/exe:envoy-static (not from a clean build though) with this change removed and things seemed to work. There are other places in api/envoy/config/filter/http/** that reference proto validation without this declaration in their BUILD files.

snowp · 2019-06-26T18:10:44Z

docs/root/intro/version_history.rst

 * access log: added a new field for route name to file and gRPC access logger.
 * access log: added a new field for response code details in :ref:`file access logger<config_access_log_format_response_code_details>` and :ref:`gRPC access logger<envoy_api_field_data.accesslog.v2.HTTPResponseProperties.response_code_details>`.
 * access log: added several new variables for exposing information about the downstream TLS connection to :ref:`file access logger<config_access_log_format_response_code_details>` and :ref:`gRPC access logger<envoy_api_field_data.accesslog.v2.AccessLogCommon.tls_properties>`.
+* access log: added a new flag for request rejected due to failed strict header check


nit: end with period

snowp · 2019-06-26T18:15:52Z

source/common/router/router.h

+                                                 const ParseRetryFlagsFunc& parseFn) {
+      HeaderCheckResult r;
+      if (header_entry) {
+        const auto flags = parseFn(header_entry->value().getStringView());


Thinking about this some more, maybe it would make more sense to have the parseFn return std::pair<uint32_t, bool> (or something to that effect, potentially a named struct), containing both the retry flags and whether there were any unknown fields? That way we avoid having to clear the bit in the other case and the interface might be a bit self explanatory?

Sorry for the churn here, hopefully we can get this to a place where we're all happy with it

No worries re: churn. I think a named struct would be clearer at calling sites, but going to repush with std::pair and see how it reads during review. I agree that the clearing of the UNKNOWN flag is an implementation detail callers shouldn't need to remember.

snowp · 2019-06-26T18:18:16Z

source/common/router/router.cc

+const FilterUtility::StrictHeaderChecker::HeaderCheckResult
+FilterUtility::StrictHeaderChecker::test(Http::HeaderMap& headers,
+                                         const Http::LowerCaseString& target_header) {
+  HeaderCheckResult r;


This doesn't seem to be used besides returning? Might be easier to just inline it e.g. return HeaderCheckResult();

Good catch; this is was from a time when we always returned "valid" to ignore a target_header didn't have an associated validator. This in combination with the assert + NOT_REACHED_GCOVR_EXCL_LINE below means we ought to be able to remove both this declaration and its return statement.

snowp · 2019-06-26T18:19:43Z

source/common/router/router.cc

+                               &Router::RetryStateImpl::parseRetryGrpcOn);
+  } else {
+    // Should only validate headers for which we have implemented a validator
+    ASSERT(false);


you can use NOT_REACHED_GCOVR_EXCL_LINE to indicate that we'll never hit this and exclude it from coverage

snowp · 2019-06-26T18:20:34Z

source/common/router/router.cc

  ENVOY_STREAM_LOG(debug, "cluster '{}' match for URL '{}'", *callbacks_,
                   route_entry_->clusterName(), headers.Path()->value().getStringView());

+  if (config_.strict_check_headers_ != nullptr && !config_.strict_check_headers_->empty()) {


I think you can omit the empty check since the for loop will just noop in that case

snowp · 2019-06-26T18:22:06Z

source/common/router/router.h

+      const Http::HeaderEntry* entry_;
+    };
+
+    static const HeaderCheckResult test(Http::HeaderMap& headers,


Mind adding a comment to this explaining what it does? A more descriptive name might be useful too, maybe checkHeader?

snowp · 2019-06-26T18:23:28Z

source/common/router/router.h

    }
  }
+  using HeaderVector = std::vector<Http::LowerCaseString>;
+  using StrictCheckHeadersPtr = std::unique_ptr<HeaderVector>;


since this is just a unique ptr to HeaderVector, i'd probably call it just HeaderVectorPtr

snowp · 2019-06-26T18:29:08Z

test/common/router/router_test.cc

+                                         "x-envoy-max-retries", "x-envoy-retry-on",
+                                         "x-envoy-retry-grpc-on"));
+
+// Each test param instantiates a router that strict-checks one particular header.


Maybe add a test where multiple headers fail the check?

…s, all_valid) Signed-off-by: Xiao Yu <xyu@stripe.com>

…nits Signed-off-by: Xiao Yu <xyu@stripe.com>

Signed-off-by: Xiao Yu <xyu@stripe.com>

xyu-stripe · 2019-07-01T04:58:29Z

@snowp PTAL? This latest push addresses the following:

make RetryStateImpl::{parseRetryOn, parseRetryGrpcOn} explicitly return std::pair<uint32_t policies, bool no_invalid> + add documenting comments
Rename StrictHeaderChecker::test -> checkHeaders + document
Test coverage where >1 header values are invalid: added value-parameterized test to assert failure for all pairs of supported headers where each pair has invalid values.

snowp

Thanks, this is coming along nicely

snowp · 2019-07-01T15:26:40Z

api/envoy/config/filter/http/router/v2/BUILD

@@ -1,4 +1,4 @@
-load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal")
+load("@envoy_api//bazel:api_build_system.bzl", "api_go_proto_library", "api_proto_library_internal")


I'm no bazel expert but I thought this was only necessary if you directly referenced the api_go_proto_library rule in this BUILD file which it doesn't seem like you're doing? I could be totally wrong here, so feel free to leave it if it's necessary for the build

snowp · 2019-07-01T15:30:52Z

source/common/router/retry_state_impl.h

  bool wouldRetryFromReset(const Http::StreamResetReason reset_reason);
  RetryStatus shouldRetry(bool would_retry, DoRetryCallback callback);

+  static uint32_t parseRetryOn_(absl::string_view config, bool flag_parse_failures);


Are these used for anything anymore?

Oops good catch. Removed.

snowp · 2019-07-01T15:32:09Z

source/common/router/router.cc

+  }
+  NOT_REACHED_GCOVR_EXCL_LINE
+  // Should only validate headers for which we have implemented a validator.
+  ASSERT(false);


NOT_REACH_GCOVR_EXCL_LINE already calls abort(), so no need for this assert

snowp · 2019-07-01T15:38:39Z

api/envoy/data/accesslog/v2/accesslog.proto

      EXTERNAL_SERVICE = 1;
+      // The request was denied because a header value failed a :ref:`strict header check
+      // <envoy_api_field_config.filter.http.router.v2.Router.strict_check_headers>` failed.
+      STRICT_HEADER_CHECK_FAILED = 2;


Not sure if this belongs here, this seems to be dealing with authorization of an endpoint, not about whether the request is valid. If there isn't a similar flag for "invalid request reason" we can probably just drop this

I think you're right. It looks like I should instead move this to be a bool field to match treatment of the other response flags in accesslog.proto

…d `parseRetry*_` Signed-off-by: Xiao Yu <xyu@stripe.com>

…lag, not an authorization reason Signed-off-by: Xiao Yu <xyu@stripe.com>

xyu-stripe · 2019-07-01T16:54:59Z

@snowp PTAL? Thanks for your continued patience and feedback! Repushed to address the following:

removed api_go_proto_library from api/envoy/config/filter/http/router/v2/BUILD
remove cruft e.g. extraneous assert and unused parseRetryOn*_ fn signatures
InvalidEnvoyRequestHeaders reworked in accesslog.proto to be a ResponseFlag boolean field

xyu-stripe · 2019-07-01T20:21:41Z

/retest

(test/common/network:{udp_listener_impl_test, listener_impl_test} both pass on macosx for me)

repokitteh-read-only · 2019-07-01T20:21:44Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #7323 (comment) was created by @xyu-stripe.

see: more, trace.

snowp

Nice, this LGTM

xyu-stripe · 2019-07-01T22:46:46Z

/azp run

azure-pipelines · 2019-07-01T22:46:51Z

Commenter does not have sufficient privileges for PR 7323 in repo envoyproxy/envoy

xyu-stripe · 2019-07-01T23:28:27Z

Some details on envoy-macosx build failures:

7/1 09:38: 1st attempt

listener_impl_test:     TIMEOUT
udp_listener_impl_test: TIMEOUT

7/1 13:42: 2nd attempt

listener_impl_test:     TIMEOUT
udp_listener_impl_test: PASS

When I run the test locally I observe that udp_listener_impl_test fails 4 of 10 runs!

bazel test --test_output=streamed //test/common/network:udp_listener_impl_test --test_arg='--gtest_repeat=10'

Is this a known source of flakiness? Is there a way we could have /azp run work for folks without repo write access?

mattklein123

Thank you. Really nicely done. Great work!

jmarantz mentioned this pull request Jun 20, 2019

stats: doc memory-consumption tests and how to fix #7336

Closed

snowp self-assigned this Jun 20, 2019

snowp suggested changes Jun 20, 2019

View reviewed changes

xyu-stripe commented Jun 21, 2019

View reviewed changes

xyu-stripe added 4 commits June 22, 2019 23:10

code review: simplify usage of RetryPolicy::RETRY_UNKNOWN_FIELD_ERROR…

491cd96

… + assert - clear RETRY_UNKNOWN_FIELD_ERROR when parsing `retry_on` config field - `StrictHeaderChecker::test` assert failure if target header is unsupported Signed-off-by: Xiao Yu <xyu@stripe.com>

code review: fix 64-byte cluster increase per MemoryLargeClusterSizeW…

072639d

…ithStats - Use std::unique_ptr for Envoy::Router::FilterConfig::strict_check_headers_ and only allocate it when we parse values from the config. Signed-off-by: Xiao Yu <xyu@stripe.com>

code review: add TODO comment to switch strict_check_headers_ to a …

ed2a492

…bitset - add TODO comment - alias the unique_ptr type Signed-off-by: Xiao Yu <xyu@stripe.com>

snowp suggested changes Jun 26, 2019

View reviewed changes

xyu-stripe added 3 commits June 29, 2019 18:16

code review: more explicit interface for parseRetryOn -> return (flag…

6d381c3

…s, all_valid) Signed-off-by: Xiao Yu <xyu@stripe.com>

code review: rename StrictHeaderChecker::test -> checkHeaders + docs …

79ecd36

…nits Signed-off-by: Xiao Yu <xyu@stripe.com>

code review: add test coverage for multiple invalid input header values

6eec18c

Signed-off-by: Xiao Yu <xyu@stripe.com>

snowp suggested changes Jul 1, 2019

View reviewed changes

xyu-stripe added 2 commits July 1, 2019 09:30

code review: remove assert, api_go_proto_library reference, and unuse…

9bc4cfe

…d `parseRetry*_` Signed-off-by: Xiao Yu <xyu@stripe.com>

code review: set invalid_envoy_request_headers as a plain ResponseF…

a0f8331

…lag, not an authorization reason Signed-off-by: Xiao Yu <xyu@stripe.com>

snowp approved these changes Jul 1, 2019

View reviewed changes

snowp assigned mattklein123 Jul 1, 2019

mattklein123 approved these changes Jul 2, 2019

View reviewed changes

mattklein123 merged commit ecd03a4 into envoyproxy:master Jul 2, 2019

		@@ -1,4 +1,4 @@
		load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal")
		load("@envoy_api//bazel:api_build_system.bzl", "api_go_proto_library", "api_proto_library_internal")

Conversation

xyu-stripe commented Jun 19, 2019

Uh oh!

xyu-stripe commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xyu-stripe commented Jun 19, 2019

Uh oh!

repokitteh-read-only bot commented Jun 19, 2019

Uh oh!

xyu-stripe commented Jun 19, 2019

Uh oh!

repokitteh-read-only bot commented Jun 19, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xyu-stripe Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xyu-stripe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xyu-stripe commented Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp commented Jun 24, 2019

Uh oh!

xyu-stripe commented Jun 24, 2019

Uh oh!

snowp commented Jun 24, 2019

Uh oh!

mattklein123 commented Jun 24, 2019

Uh oh!

xyu-stripe commented Jun 25, 2019

Uh oh!

mattklein123 commented Jun 25, 2019

Uh oh!

jmarantz commented Jun 25, 2019

Uh oh!

xyu-stripe commented Jun 25, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xyu-stripe commented Jun 19, 2019 •

edited

Loading

xyu-stripe Jun 21, 2019 •

edited

Loading

xyu-stripe left a comment •

edited

Loading

snowp commented Jun 21, 2019 •

edited

Loading

xyu-stripe commented Jun 23, 2019 •

edited

Loading