[extensions/thrift_proxy] Add header matching to thrift router #4239

brirams · 2018-08-23T22:13:53Z

Description
This change adds header matching to the thrift router We do this by pulling in the route proto definition into the thrift route proto and making use of the Http::HeaderUtility class to do the matching for us. As such, we support the same type of header matching that exists for the http router.

The integration tests got interesting in that we can only define headers for the thrift payload generation infra at the transport level so I had to modify the script(s) to take a headers parameter and pass it through the PayloadOptions class.

Note I have an outstanding PR that touches the HeaderMatcher thrift proto definitions here but this takes precedence and will only affect this change in that I'd have to change BUILD files and namespaces.

Risk Level
LOW

Testing

unit and integrations tests, new and old, pass.

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

…ability to specify headers that get included in the thrift payload generation script. also, format it all Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams · 2018-08-23T22:19:54Z

test/extensions/filters/network/thrift_proxy/driver/generate_fixture.sh

-fi
+MULTIPLEX=
+HEADERS=
+while getopts ":s:H:" opt; do


using getopts makes it easier to tell whether a service name or headers are being specified when only one is provided

zuercher

Nice!

zuercher · 2018-08-23T23:02:59Z

api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto

@@ -46,6 +47,8 @@ message RouteMatch {
  // Inverts whatever matching is done in match_specifier. Cannot be combined with wildcard matching
  // as that would result in routes never being matched.
  bool invert = 3;
+
+  repeated envoy.api.v2.route.HeaderMatcher headers = 4;


nit: Bump the next free field comment.

Also should add a comment for this field.

going to lift what's written for the http RouteMatch since it's very apropos.

zuercher · 2018-08-23T23:08:24Z

test/extensions/filters/network/thrift_proxy/driver/generate_fixture.sh

@@ -2,12 +2,12 @@

 # Generates request and response fixtures for integration tests.

-# Usage: generate_fixture.sh <transport> <protocol> [multiplex-service] -- method [param...]
+# Usage: generate_fixture.sh <transport> <protocol> -s [multiplex-service] -H [headers ] method [param...]


nit: rm space before ]

zuercher · 2018-08-23T23:23:50Z

test/extensions/filters/network/thrift_proxy/integration.cc

+      if (i < sz - 1) {
+        ss << ",";
+      }
+    }


How about

std::vector<std::string> headers; std::transform(options.headers_.begin(), options.headers_.end(), std::back_inserter(headers), [](std::pair<std::string, std::string>& header) -> std::string { return header.first + "=" + header.second; }); args.push_back(StringUtil::join(headers, ","));

these are all things I didn't know existed so 👍

Might need to include <algorithm> to get transform and back_inserter, FYI

is already included.

zuercher · 2018-08-23T23:31:56Z

api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto

@@ -46,6 +47,8 @@ message RouteMatch {
  // Inverts whatever matching is done in match_specifier. Cannot be combined with wildcard matching
  // as that would result in routes never being matched.
  bool invert = 3;


Since invert doesn't apply to the header matchers, we should probably update this comment to make clear (in the docs) what this inverts. Specifically remove the reference to "match_specifier" since that doesn't show up in the docs.

zuercher · 2018-08-23T23:34:36Z

test/extensions/filters/network/thrift_proxy/router_test.cc

+      method_name: "method1"
+      headers:
+      - name: "content-type"
+        exact_match: "application/grpc"


Maybe not an HTTP header?

ha! didn't even realize.

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams · 2018-08-24T18:22:00Z

🎱

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams · 2018-08-24T19:19:10Z

whoops -- didn't mean to kick ci twice but ¯_(ツ)_/¯

zuercher

One last nit.

zuercher · 2018-08-24T20:22:36Z

api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto

+  // headers against all the specified headers in the route config. A match will happen if all the
+  // headers in the route are present in the request with the same values (or based on presence if
+  // the value field is not in the config). Note that this only applies when using the Header thrift
+  // transport type.


Probably should just leave it as "this only applies for Thrift transport and/or protocols that support headers."

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

zuercher

Cool. Thanks!

brian-pane · 2018-08-24T23:33:56Z

api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto

@@ -43,9 +44,19 @@ message RouteMatch {
    string service_name = 2;
  }

-  // Inverts whatever matching is done in match_specifier. Cannot be combined with wildcard matching
-  // as that would result in routes never being matched.
+  // Inverts whatever matching is done in the :ref:`method_name


Does this also invert the matching done in the headers?

it does not -- I explicitly mention the method_name and service_name fields in the hopes of making it clear that this doesn't apply to headers(originally to address @zuercher's same concern: #4239 (comment)).

I can certainly add language to make it explicit that it doesn't apply to header matching if you think that'll help.

Got it, thanks. I think would help to add a bit of language for that, just to err on the side of caution.

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

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 <piotrsikora@google.com>

brirams added 3 commits August 23, 2018 15:04

add route dependency and headers field to Routematch

95053c5

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

header matching implementation with unit tests

5775182

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

add an integration test for header matching by threading through the …

cb8cc91

…ability to specify headers that get included in the thrift payload generation script. also, format it all Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams requested a review from zuercher as a code owner August 23, 2018 22:13

brirams mentioned this pull request Aug 23, 2018

[api] Move HeaderMatcher from api.v2.route to type.matcher package #4207

Closed

brirams commented Aug 23, 2018

View reviewed changes

zuercher assigned zuercher and brian-pane Aug 23, 2018

zuercher reviewed Aug 23, 2018

View reviewed changes

review feedback: use std::transform, non-http headers in tests, comments

c164ab1

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams added 2 commits August 24, 2018 12:17

Kick CI

26e6d23

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

Kick CI

6a2b9bb

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

zuercher reviewed Aug 24, 2018

View reviewed changes

comment fix

5784cce

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

zuercher previously approved these changes Aug 24, 2018

View reviewed changes

brian-pane reviewed Aug 24, 2018

View reviewed changes

make it clear that RouteMatch.invert does not apply to header matching

617c1c3

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams dismissed zuercher’s stale review via 617c1c3 August 25, 2018 06:26

zuercher approved these changes Aug 27, 2018

View reviewed changes

zuercher merged commit f5e219e into envoyproxy:master Aug 27, 2018

brirams deleted the brirams/#5555/implement_thrift_header_matching branch August 27, 2018 16:37

[extensions/thrift_proxy] Add header matching to thrift router #4239

[extensions/thrift_proxy] Add header matching to thrift router #4239

Uh oh!

Conversation

brirams commented Aug 23, 2018

Uh oh!

brirams Aug 23, 2018 • 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!

zuercher 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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brirams commented Aug 24, 2018

Uh oh!

brirams commented Aug 24, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

brirams Aug 23, 2018 •

edited

Loading