Feature support connection info in route matching #8453

jimini-lumox · 2019-10-02T06:39:38Z

Description: Add the ability to route match based on client credentials.

This is an output of the changes requested for PR #8248 (#8248 (comment))
To more cleanly support #8248 , it would be better to be able to route based on downstream connection details, instead of hoisting more information into headers.

As an API example, route matching based on presented and/or expired client certificate is supported.
The end goal for #8248 is to route based on 'validated'.

By default the routing rules are unchanged.

Risk Level: Medium
Testing: Currently Manual tests
Docs Changes: API proto changes
Release Notes: N/A

Signed-off-by: Michael Hargreaves <[email protected]>

repokitteh-read-only · 2019-10-02T06:39:43Z

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

🐱

Caused by: #8453 was opened by jimini-lumox.

see: more, trace.

Signed-off-by: Michael Hargreaves <[email protected]>

htuch

Some API comments to start things. Also, have you considered having a filter put this information in dynamic metadata and doing a more generic match on that in the route table? I'm not generally a fan of putting too much business logic for specific cases here unless it's widely interesting.

CC @PiotrSikora

api/envoy/api/v2/route/route.proto

lizan · 2019-10-05T06:46:12Z

Some API comments to start things. Also, have you considered having a filter put this information in dynamic metadata and doing a more generic match on that in the route table? I'm not generally a fan of putting too much business logic for specific cases here unless it's widely interesting.

@htuch My thought on this is consolidating with what RBAC has, we already have huge overlap with there and the properties from the TLS connection will be also interested there. This falls into the goal of consolidating all HttpMatcher in v4.

htuch · 2019-10-07T16:27:50Z

@lizan yeah, RBAC unification makes sense. Is there an issue and/or roadmap somewhere to provide a north star? Is there anything actionable in this review on that front?

lambdai · 2019-10-08T04:24:51Z

source/common/router/config_impl.cc

+  if (criteria.presented()) {
+    const bool peerPresented = stream_info.downstreamSslConnection() &&
+                               stream_info.downstreamSslConnection()->expirationPeerCertificate();
+    matches &= *criteria.presented() == peerPresented;


nits: I think has_value() and value() is preferred if it is optional<bool>

modified to use has_value() & value()

lizan · 2019-10-08T04:52:10Z

@lizan yeah, RBAC unification makes sense. Is there an issue and/or roadmap somewhere to provide a north star? Is there anything actionable in this review on that front?

Part of #5569, this is the one of big item we've been discussing for v4 and UDPA.

Signed-off-by: Michael Hargreaves <[email protected]>

htuch

Great, starting to review implementation, a few comments.
/wait

api/envoy/api/v2/route/route.proto

source/common/router/BUILD

source/common/router/config_impl.cc

Signed-off-by: Michael Hargreaves <[email protected]>

htuch

Thanks, looks mostly good, but some comments to be resolved.
/wait

include/envoy/router/router.h

test/common/router/route_fuzz_test.cc

htuch · 2019-10-16T03:35:33Z

include/envoy/router/router.h

@@ -502,6 +502,16 @@ class MetadataMatchCriteria {
  mergeMatchCriteria(const ProtobufWkt::Struct& metadata_matches) const PURE;
 };

+class TlsContextMatchCriteria;
+using TlsContextMatchCriteriaConstPtr = std::unique_ptr<const TlsContextMatchCriteria>;


Nit: move this after the class definition below, no need to forward declare for this simple class.

I still see the forward declaration above..

Sorry, thought it was referring to same comment as tls_context_match_criteria_impl.h below
Fixed now.

source/common/router/tls_context_match_criteria_impl.cc

source/common/router/tls_context_match_criteria_impl.h

htuch · 2019-10-16T03:42:42Z

test/common/router/config_impl_test.cc

@@ -53,7 +53,9 @@ class TestConfigImpl : public ConfigImpl {
                 bool validate_clusters_default)
      : ConfigImpl(config, factory_context, validate_clusters_default), config_(config) {}

-  RouteConstSharedPtr route(const Http::HeaderMap& headers, uint64_t random_value) const override {
+  RouteConstSharedPtr route(const Http::HeaderMap& headers,


I'm probably missing it, but could you point me to where the tests for the new behaviors are?

I've just now pushed the unit-test for tls-context matching.

Signed-off-by: Michael Hargreaves <[email protected]>

…atching Signed-off-by: Michael Hargreaves <[email protected]>

Signed-off-by: Michael Hargreaves <[email protected]>

htuch

LGTM modulo the two remaining small nits.

htuch · 2019-10-17T22:14:08Z

test/common/router/config_impl_test.cc

@@ -5780,6 +5787,95 @@ name: foo
  }
 }

+TEST_F(RouteMatcherTest, TlsContextMatching) {


Please add a brief comment above this test explaining what it does.

I've added a comment - let me know if it needs more details. thanks

Signed-off-by: Michael Hargreaves <[email protected]>

htuch

Thanks, this is great!

owlbreeze added 5 commits September 30, 2019 23:40

Adding RouteMatcher access to StreamInfo

b41c9a4

Signed-off-by: Michael Hargreaves <[email protected]>

Format corrections

15dbd61

Signed-off-by: Michael Hargreaves <[email protected]>

Add 'expired' and 'presented' credential route matching

955edea

Signed-off-by: Michael Hargreaves <[email protected]>

Move credential matching to function

3c9c3e8

Signed-off-by: Michael Hargreaves <[email protected]>

Formatting fixups

3528df4

Signed-off-by: Michael Hargreaves <[email protected]>

Merged master

878030d

Signed-off-by: Michael Hargreaves <[email protected]>

dio assigned AndresGuedez Oct 2, 2019

htuch reviewed Oct 3, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Show resolved Hide resolved

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/route/route.proto Show resolved Hide resolved

htuch requested a review from PiotrSikora October 3, 2019 22:46

htuch assigned PiotrSikora and htuch and unassigned AndresGuedez Oct 3, 2019

jimini-lumox mentioned this pull request Oct 4, 2019

Feature untrusted client certs #8248

Closed

lambdai reviewed Oct 8, 2019

View reviewed changes

owlbreeze added 3 commits October 9, 2019 11:16

Renamed Credential match to TlsContext match

27df1b4

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed tests

9b9cd95

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed test build

013be53

Signed-off-by: Michael Hargreaves <[email protected]>

jimini-lumox requested a review from zuercher as a code owner October 9, 2019 02:53

owlbreeze added 8 commits October 9, 2019 13:02

Fix format

5c9138a

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed unit-test

dae4bca

Signed-off-by: Michael Hargreaves <[email protected]>

Formatting fixups.

3e263b3

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed unit-test

e83aec4

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed unit-tests

a23d508

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed format

e9a42a5

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed format

a391c38

Signed-off-by: Michael Hargreaves <[email protected]>

Change to use has_value & value for optional

7a79963

Signed-off-by: Michael Hargreaves <[email protected]>

owlbreeze added 2 commits October 11, 2019 09:53

Fix format

a8a4a99

Signed-off-by: Michael Hargreaves <[email protected]>

Fixed copy-paste bug

28d36a3

Signed-off-by: Michael Hargreaves <[email protected]>

htuch suggested changes Oct 11, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Show resolved Hide resolved

source/common/router/BUILD Outdated Show resolved Hide resolved

source/common/router/config_impl.cc Outdated Show resolved Hide resolved

source/common/router/config_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Oct 11, 2019

Changed vars to snake case, and renamed some files

91cd14f

Signed-off-by: Michael Hargreaves <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 13, 2019

owlbreeze added 2 commits October 15, 2019 06:44

Removed 'expired' routing to simplify changes to API.

e856a49

Signed-off-by: Michael Hargreaves <[email protected]>

Removed 'expired' TlsContextMatch'ing from Mock

ac6e312

Signed-off-by: Michael Hargreaves <[email protected]>

htuch reviewed Oct 16, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 16, 2019

owlbreeze added 4 commits October 16, 2019 21:23

Adding Router TlsContextMatcher unit-tests

01df4ef

Signed-off-by: Michael Hargreaves <[email protected]>

Merge branch 'master' into feature-support-connection-info-in-route-m…

cfdc48c

…atching Signed-off-by: Michael Hargreaves <[email protected]>

Updated router TlsContextMatch unit-test

4c5f55b

Signed-off-by: Michael Hargreaves <[email protected]>

fix-format

aa95768

Signed-off-by: Michael Hargreaves <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 16, 2019

fix-formating

71cbe83

Signed-off-by: Michael Hargreaves <[email protected]>

repokitteh-read-only bot added the api label Oct 17, 2019

htuch suggested changes Oct 17, 2019

View reviewed changes

htuch added waiting and removed api labels Oct 17, 2019

owlbreeze added 2 commits October 18, 2019 08:53

Removed unnecessary forward decl

d83db11

Signed-off-by: Michael Hargreaves <[email protected]>

Added comment to test case

a215c76

Signed-off-by: Michael Hargreaves <[email protected]>

repokitteh-read-only bot added api and removed waiting labels Oct 17, 2019

htuch approved these changes Oct 18, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Oct 18, 2019

htuch merged commit 350e4ce into envoyproxy:master Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature support connection info in route matching #8453

Feature support connection info in route matching #8453

jimini-lumox commented Oct 2, 2019

repokitteh-read-only bot commented Oct 2, 2019

htuch left a comment

lizan commented Oct 5, 2019

htuch commented Oct 7, 2019

lambdai Oct 8, 2019

jimini-lumox Oct 11, 2019

lizan commented Oct 8, 2019

htuch left a comment

htuch left a comment

htuch Oct 16, 2019

jimini-lumox Oct 16, 2019

htuch Oct 17, 2019

jimini-lumox Oct 17, 2019

htuch Oct 16, 2019

jimini-lumox Oct 16, 2019

htuch left a comment

htuch Oct 17, 2019

jimini-lumox Oct 17, 2019

htuch left a comment

Feature support connection info in route matching #8453

Feature support connection info in route matching #8453

Conversation

jimini-lumox commented Oct 2, 2019

repokitteh-read-only bot commented Oct 2, 2019

htuch left a comment

Choose a reason for hiding this comment

lizan commented Oct 5, 2019

htuch commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan commented Oct 8, 2019

htuch left a comment

Choose a reason for hiding this comment

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

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

htuch left a comment

Choose a reason for hiding this comment