router: Add ability of custom headers to rely on per-request data #4219

curiouserrandy · 2018-08-21T21:12:15Z

Description: This PR adds an per-request state object of type FilterState (was DynamicMetadata) to the RequestInfo and makes objects of type StringAccessor stored in that object available to custom headers.

Risk Level: Low (only adding functionality)

Testing: Unit tests.

Docs Changes: Added documentation for the %PER_REQUEST_STATE% variable for custom headers.

Release Notes: Added %PER_REQUEST_STATE% to variables available for custom headers.

Fixes #3559 #151

Signed-off-by: Randy Smith <[email protected]>

This reverts commit 9ab78c8. Signed-off-by: Randy Smith <[email protected]>

Signed-off-by: Randy Smith <[email protected]>

curiouserrandy · 2018-08-21T21:12:36Z

@zuercher : I think you're still the right person for this? But feel free to assign someone else if I'm wrong.

zuercher

I think that looks pretty good. I had a question and brief comments to start. Will look more closely tomorow.

zuercher · 2018-08-22T04:28:39Z

source/common/router/header_formatter.cc

+    if (!dynamic_metadata.hasData<StringAccessor>(param)) {
+      throw EnvoyException(fmt::format("Invalid header information: PER_REQUEST_STATE value \"{}\" "
+                                       "exists but is not string accessible",
+                                       param));


Throwing an exception here seems pretty aggressive. I'm not even sure it'll get converted into a 5xx since the exceptions that the ConnectionManagerImpl catches are subclasses of EnvoyException. Is it not sufficient write to the debug log and return an empty string?

Sure. One of the things I was looking for was guidance on error handling; I should have said. I will convert.

zuercher · 2018-08-22T04:33:15Z

include/envoy/router/string_accessor.h

+ * allows lazy evaluation if needed. All values meant to be accessible to the
+ * custom request/response header mechanism must use this type.
+ */
+class StringAccessor : public ::Envoy::RequestInfo::FilterState::Object {


Our style is to leave off the ::Envoy: in these cases.

Ok, I'll fix that.

Just so I can more usefully comment when I start doing envoy code reviews: Is there a section of the style guide that mandates this, or is this background custom?

I don't think there's a specific prohibition.

zuercher · 2018-08-22T04:34:13Z

source/common/router/header_formatter.cc

+                                       param));
+    }
+
+    return static_cast<std::string>(dynamic_metadata.getData<StringAccessor>(param).asString());


Should this just be std::string(dynamic_metadata.getData<StringAccessor>(param).asString())?

Happy to change it (and I will) but what's your preference for construct over cast? Just fewer characters?

I think it's clearer that you're constructing a new std::string.

Signed-off-by: Randy Smith <[email protected]>

zuercher

Thanks! Almost there. There are some uses of "dynamic metadata" that I think should get switched to the new terminology to avoid confusion.

zuercher · 2018-08-22T17:32:45Z

test/common/request_info/filter_state_impl_test.cc


-  void resetDynamicMetadata() { dynamic_metadata_ = std::make_unique<DynamicMetadataImpl>(); }
-  DynamicMetadata& dynamic_metadata() { return *dynamic_metadata_; }
+  void resetDynamicMetadata() { dynamic_metadata_ = std::make_unique<FilterStateImpl>(); }


Rename these methods/fields as well?

Done. (Really sorry about missing all these, by the way--a lesson to be more creating in use of regular expressions.)

zuercher · 2018-08-22T17:35:52Z

source/common/router/header_formatter.cc

+
+  std::string param(modified_param_str);
+  return [param](const Envoy::RequestInfo::RequestInfo& request_info) -> std::string {
+    const Envoy::RequestInfo::FilterState& dynamic_metadata = request_info.perRequestState();


rename var?

zuercher · 2018-08-22T17:36:37Z

test/common/router/header_formatter_test.cc

@@ -167,6 +169,67 @@ TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariableMis
  testFormatting(request_info, "UPSTREAM_METADATA([\"namespace\", \"key\"])", "");
 }

+TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithDynamicMetadataVariable) {


Rename tests/vars?

zuercher · 2018-08-22T17:37:12Z

test/common/access_log/test_util.h

@@ -156,6 +157,11 @@ class TestRequestInfo : public RequestInfo::RequestInfo {
    (*metadata_.mutable_filter_metadata())[name].MergeFrom(value);
  };

+  const ::Envoy::RequestInfo::FilterState& perRequestState() const override {


Missed these.

Sorry, not following. These look right to me--what change are you requesting?

Please remove the ::Envoy:: prefix.

Huh. Coudla sworn that qualifier wasn't there when I was looking at your comment originally, but that doesn't make sense since github binds comments to hashes. But removing them won't work--the dependent files won't then compile:

ERROR: /usr/local/google/home/rdsmith/Checkouts/envoy/test/common/access_log/BUIL
D:31:1: C++ compilation of rule '//test/common/access_log:access_log_formatter_fu
zz_test_lib' failed (Exit 1)
In file included from ./test/fuzz/utility.h:5:0,
from test/common/access_log/access_log_formatter_fuzz_test.cc:5:
./test/common/access_log/test_util.h:160:22: error: 'FilterState' in 'class Envoy
::RequestInfo::RequestInfo' does not name a type
const RequestInfo::FilterState& perRequestState() const override {
^~~~~~~~~~~
./test/common/access_log/test_util.h:163:16: error: 'FilterState' in 'class Envoy
::RequestInfo::RequestInfo' does not name a type
RequestInfo::FilterState& perRequestState() override { return per_request_stat
e_; }
^~~~~~~~~~~
./test/common/access_log/test_util.h:187:16: error: 'FilterStateImpl' in 'class E
nvoy::RequestInfo::RequestInfo' does not name a type
RequestInfo::FilterStateImpl per_request_state_{};
^~~~~~~~~~~~~~~

(While I hadn't understood your comment, I had grepped for ::Envoy in my diff and removed them all, then re-added the ones that caused compilation errors.)

Oh I see, it's because RequestInfo here refers to the base class. Can you drop the leading colons (e.g., Envoy::RequestInfo::FilterState) to match the other references to the RequestInfo namespace in this file.

Ah, didn't think to try that. Done.

Signed-off-by: Randy Smith <[email protected]>

curiouserrandy · 2018-08-22T18:32:36Z

New revision uploaded. I wanted to call out that in addition to your requested changes I also reworked when I was using "filter state" and when I was using "per request state" (filter state is the underlying class, and per request state is how it's used currently--I'm leaving the door open for per connection state in the future).

Signed-off-by: Randy Smith <[email protected]>

zuercher · 2018-08-23T17:13:41Z

@junr03 PTAL as well

zuercher

Thanks!

curiouserrandy · 2018-08-27T13:58:57Z

What's the current status of this review?

curiouserrandy · 2018-08-27T14:03:01Z

(@junr03 , I think?)

curiouserrandy · 2018-08-28T14:53:50Z

@zuercher : There are several people at Google waiting for this PR to land. Any chance you can get a second opinion from someone other than @junr03 ? They appear to be AFI(nternet).

junr03

small comment, other than that lgtm

junr03 · 2018-08-28T16:59:45Z

test/common/router/header_formatter_test.cc

+
+namespace {
+
+class IntAccessor : public RequestInfo::FilterState::Object {


I wonder if it is worth it to move this out to test_common, and have it shared here and in the request_info_impl test.

I pulled it out into its own file in test/common/request_info (since as shown by the namespace it's a RequestInfo context concept) and referenced that from header_formatter_test. Let me know what you think.

yeah, sounds good to me. Just a small naming change

Signed-off-by: Randy Smith <[email protected]>

junr03 · 2018-08-28T18:52:55Z

test/common/request_info/BUILD

@@ -21,6 +22,7 @@ envoy_cc_test(
    name = "request_info_impl_test",
    srcs = ["request_info_impl_test.cc"],
    deps = [
+        ":test_int_accessor_interface",


s/interface/lib/

Done.

Having said that, would you be willing to give me a sense of when envoy wants targets named _interface vs. _lib? I assumed it was if the target was header only, but it sounds like I got that wrong.

yep, interface is reserved for virtual classes (the vast majority, if not all under this tree https://github.com/envoyproxy/envoy/tree/master/include/envoy, lib are for concrete implementations, even if just in a header file.

Signed-off-by: Randy Smith <[email protected]>

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <[email protected]>

curiouserrandy added 14 commits August 12, 2018 21:20

Just parser change

9ab78c8

Signed-off-by: Randy Smith <[email protected]>

Add DynamicMetadata to user defined header configuration possibilities.

343cbe9

Signed-off-by: Randy Smith <[email protected]>

Fixed compilation errors

b467662

Signed-off-by: Randy Smith <[email protected]>

Added string accessor

94c5049

Signed-off-by: Randy Smith <[email protected]>

Well, it frigging builds. Who knew?

6ad4077

Signed-off-by: Randy Smith <[email protected]>

Switched access name

ba1fd0c

Signed-off-by: Randy Smith <[email protected]>

Got all tests to pass (I think)

ba70cee

Signed-off-by: Randy Smith <[email protected]>

format fixes

25e0f13

Signed-off-by: Randy Smith <[email protected]>

Revert "Just parser change"

2df99c6

This reverts commit 9ab78c8. Signed-off-by: Randy Smith <[email protected]>

Tests pass

26eabba

Signed-off-by: Randy Smith <[email protected]>

Fix docs

7fb452d

Signed-off-by: Randy Smith <[email protected]>

Fixed formatting

4b40256

Signed-off-by: Randy Smith <[email protected]>

Merge branch 'master' into DynamicUDHOnly

6923a3c

Signed-off-by: Randy Smith <[email protected]>

Results of self code review

1ea92fa

Signed-off-by: Randy Smith <[email protected]>

danielhochman assigned junr03 Aug 21, 2018

zuercher reviewed Aug 22, 2018

View reviewed changes

curiouserrandy added 2 commits August 22, 2018 12:15

Incorporated comments

8f3c882

Signed-off-by: Randy Smith <[email protected]>

Merge branch 'master' into DynamicUDHOnly

46a6c9d

Signed-off-by: Randy Smith <[email protected]>

zuercher reviewed Aug 22, 2018

View reviewed changes

zuercher self-assigned this Aug 22, 2018

Incorporated comments

b3ab11c

Signed-off-by: Randy Smith <[email protected]>

Switch namespace qualification over to Envoy customary

b07efd5

Signed-off-by: Randy Smith <[email protected]>

zuercher previously approved these changes Aug 23, 2018

View reviewed changes

junr03 previously approved these changes Aug 28, 2018

View reviewed changes

Share IntAccessor between tests.

de7a118

Signed-off-by: Randy Smith <[email protected]>

curiouserrandy dismissed stale reviews from junr03 and zuercher via de7a118 August 28, 2018 18:42

junr03 reviewed Aug 28, 2018

View reviewed changes

curiouserrandy added 3 commits August 28, 2018 14:57

Changed name

a2124e9

Signed-off-by: Randy Smith <[email protected]>

Merge branch 'master' into DynamicUDHOnly

ed16404

Signed-off-by: Randy Smith <[email protected]>

Fix format

f1f2499

Signed-off-by: Randy Smith <[email protected]>

zuercher approved these changes Aug 28, 2018

View reviewed changes

zuercher merged commit 329e591 into envoyproxy:master Aug 28, 2018

curiouserrandy mentioned this pull request Sep 5, 2018

listener-filter: refactor tls_inspector so its logic could be shared with custom filters #4331

Closed


		namespace {

		class IntAccessor : public RequestInfo::FilterState::Object {

router: Add ability of custom headers to rely on per-request data #4219

router: Add ability of custom headers to rely on per-request data #4219

Conversation

curiouserrandy commented Aug 21, 2018

curiouserrandy commented Aug 21, 2018

zuercher 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

Choose a reason for hiding this comment

zuercher 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuercher Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiouserrandy commented Aug 22, 2018

zuercher commented Aug 23, 2018

zuercher left a comment

Choose a reason for hiding this comment

curiouserrandy commented Aug 27, 2018

curiouserrandy commented Aug 27, 2018

curiouserrandy commented Aug 28, 2018

junr03 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

zuercher Aug 23, 2018 •

edited

Loading