Add a transport socket match in cluster. #8100

incfly · 2019-08-30T00:51:03Z

Signed-off-by: Jianfei Hu jianfeih@google.com

API for #8016

Context

Customers adopting service mesh likes mTLS ability. However, rolling it out without breaking existing traffic is hard. This is because mTLS is configured on per cluster basis. In reality, a service consists of multiple endpoints, mixed with having Envoy sidecar and without-sidecar endpoints. Client envoy can't send mTLS traffic until all server migrated to having Envoy sidecar.

This API tries to solve the issue by allowing mTLS/transport socket to configured at finer granularity, e.g. endpoint level. The endpoint has metadata label information, which will be used to decide which transport socket configuration to use from a map specified in the cluster.

So the outcome is that, xDS management server is able to configure client envoy talks to endpoints with sidecar in mTLS and plain text to endpoints without sidecar, for a single cluster.

Description:
Risk Level: N/A (API change only)
Release Notes: Cluster API change to use different transport socket based on endpoint label.

Signed-off-by: Jianfei Hu <jianfeih@google.com>

api/envoy/api/v2/cds.proto

incfly · 2019-08-30T00:55:13Z

/cc @lizan @rshriram @PiotrSikora

api/envoy/api/v2/cds.proto

htuch · 2019-08-30T15:12:34Z

I'm generally opposed to using unstructured metadata for this purpose. If we are going to go down this path (see above comment about needing to understand design rationale and motivation), then we need to add an explicit label field to EDS endpoints.

htuch · 2019-08-30T15:17:42Z

@incfly FYI, for changes to Envoy's APIs, you should tag @envoyproxy/api-shepherds on any design review.

rshriram · 2019-08-30T19:11:19Z

So @htuch , how about this:

cluster:
  tlsContextMap:
    mtls: …, default
    plaintext: …
  endpoints:
    1.1.1.1, tlsContext-to-use: mtls
    2.2.2.2 tlsContext-to-use: plaintext

effectively, we ship a bunch of named TLS contexts to the cluster and then in the EDS, we can ship the TLS context to use for a given endpoint. This has the same effect of using label selectors in the TLS context.

lizan · 2019-08-30T19:20:41Z

@rshriram lets iterate on the doc a bit more, I like labels more for its flexibility, a tlscontextmap (or transport socket map) potentially explodes when you have multiple factor to select. (supports mTLS/TLS/cleartext)

incfly · 2019-08-30T20:19:29Z

@lizan not sure if I get it.

The value of the endpoint.transport_socket_selector (single hardcoded name) is used to retrieve entries from the map. So this is selection based on single factor, and deterministically, right?

By label do you refer to actual endpoint.metadata or it does not matter, can be a field in message Endpoint as well?

Signed-off-by: Jianfei Hu <jianfeih@google.com>

repokitteh-read-only · 2019-09-03T06:40:13Z

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

🐱

Caused by: #8100 was synchronize by incfly.

see: more, trace.

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly · 2019-09-03T17:22:54Z

PR has been modified to match the proposal, using actual fields in endpoints plus match semantics for transport socket. @htuch @lizan
PTAL, thanks.

api/envoy/api/v2/cds.proto

lizan · 2019-09-03T19:33:30Z

api/envoy/api/v2/endpoint/endpoint.proto

@@ -74,6 +80,8 @@ message LbEndpoint {
  // to subset the endpoints considered in cluster load balancing.
  core.Metadata metadata = 3;

+  repeated Label labels = 6;


So @htuch / @lizan I have been giving this some thought. I think its better to have a named TLS context rather than a label selector because named TLS contexts refer to exactly one context to use. Label selectors could select multiple contexts and then we would have to define some first match wins semantics which gets clunky.

I'd like to this be first match wins (similar to FCM), the context map couples EDS with CDS too tightly, and the matcher semantics here is more flexible to match a subset has two labels etc.

Yeah, let's just have a string field here pointing at a specific TLS context. This will be the clearest I reckon.

I'm fine with that.

Switching back into this review after some time out, it seems that we still have a label match scheme. If the consensus is heading towards this being necessary, there are two things I'd ask:

Document a clear use case of why we have this in endpoint_transport_socket_matchers comments.

Use the existing metadata; I originally pushed back on this as I felt we could just specify a transport socket name here, but now that we are going to a "label" scheme, we already have a source of this information, the endpoint metadata. There should be well-known key name for this, envoy.transport_socket.

Done. Added a use case in the end of this field comments. @lizan fyi.

I also renamed the field by removing "endpoint_", since as @rshriram points out, it's possible to add metadata for DNS based endpoint as well.

Signed-off-by: Jianfei Hu <jianfeih@google.com>

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

api/envoy/api/v2/cds.proto

api/envoy/api/v2/endpoint/endpoint.proto

rshriram · 2019-09-11T19:37:16Z

ping. any updates? Its been two weeks.

Signed-off-by: Jianfei Hu <jianfeih@google.com>

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly · 2019-09-12T17:20:35Z

/retest

repokitteh-read-only · 2019-09-12T17:20:40Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #8100 (comment) was created by @incfly.

see: more, trace.

Signed-off-by: Jianfei Hu <jianfeih@google.com>

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly · 2019-09-19T00:20:14Z

/retest

repokitteh-read-only · 2019-09-19T00:20:17Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8100 (comment) was created by @incfly.

see: more, trace.

incfly · 2019-09-19T01:27:38Z

All tests passed now, PTAL, thanks.

incfly · 2019-09-19T05:13:03Z

api/envoy/api/v2/cds.proto

+    // Optional endpoint metadata match criteria.
+    // The connection to the endpoint with metadata matching what is set in this field
+    // will use the transport socket configuration specified here.
+    // The endpoints metadata entry in *envoy.transport_socket* is used to match


@htuch @lizan

I'm reading well_known_names.h, and see we already have envoy.transport_sockets.xxx (alts) for socket name, this makes me think using envoy.transport_socket to extract endpoint info can cause confusion.
https://github.com/envoyproxy/envoy/blob/master/source/extensions/transport_sockets/well_known_names.h#L17

how about renaming this to envoy.transport_socket_match or envoy.transport_socket_selector?

wdty?

mattklein123

Thanks, generally LGTM with a few small comments.

/wait

api/envoy/api/v2/cds.proto

Signed-off-by: Jianfei Hu <jianfeih@google.com>

mattklein123

Thanks this LGTM. I will defer to others for further review.

api/envoy/api/v2/cds.proto

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly · 2019-09-20T18:05:20Z

@htuch Could you take a look to see anything else we need to update before merging?

htuch

Thanks, the explanation is now more helpful. LGTM modulo nit and merging master.

api/envoy/api/v2/cds.proto

Signed-off-by: Jianfei Hu <jianfeih@google.com>

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

htuch

Thanks!

#8309 and #8100 collided Risk Level: Low (cleanup) Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

API for envoyproxy#8016 Customers adopting service mesh likes mTLS ability. However, rolling it out without breaking existing traffic is hard. This is because mTLS is configured on per cluster basis. In reality, a service consists of multiple endpoints, mixed with having Envoy sidecar and without-sidecar endpoints. Client envoy can't send mTLS traffic until all server migrated to having Envoy sidecar. This API tries to solve the issue by allowing mTLS/transport socket to configured at finer granularity, e.g. endpoint level. The endpoint has metadata label information, which will be used to decide which transport socket configuration to use from a map specified in the cluster. So the outcome is that, xDS management server is able to configure client envoy talks to endpoints with sidecar in mTLS and plain text to endpoints without sidecar, for a single cluster. Description: Risk Level: N/A (API change only) Release Notes: Cluster API change to use different transport socket based on endpoint label. Signed-off-by: Jianfei Hu <jianfeih@google.com>

envoyproxy#8309 and envoyproxy#8100 collided Risk Level: Low (cleanup) Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

cds add transport socket matcher

bb98899

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly commented Aug 30, 2019

View reviewed changes

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

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

lizan requested a review from htuch August 30, 2019 01:00

htuch suggested changes Aug 30, 2019

View reviewed changes

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

htuch self-assigned this Aug 30, 2019

use endpiont label and udpate matcher from map to list.

7861d48

Signed-off-by: Jianfei Hu <jianfeih@google.com>

doc fix.

55d947a

Signed-off-by: Jianfei Hu <jianfeih@google.com>

lizan reviewed Sep 3, 2019

View reviewed changes

Jianfei Hu added 4 commits September 4, 2019 01:42

address comments, using single key to resolve.

0243b3b

Signed-off-by: Jianfei Hu <jianfeih@google.com>

format fix.

7807838

Signed-off-by: Jianfei Hu <jianfeih@google.com>

update doc.

cfc98ab

Signed-off-by: Jianfei Hu <jianfeih@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into tra…

f8fb9b5

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

rshriram reviewed Sep 4, 2019

View reviewed changes

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

rshriram reviewed Sep 4, 2019

View reviewed changes

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

Jianfei Hu added 4 commits September 12, 2019 05:10

restore back to label based matcher api.

20dba49

Signed-off-by: Jianfei Hu <jianfeih@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into tra…

823e1dc

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

add label

1508af2

Signed-off-by: Jianfei Hu <jianfeih@google.com>

remove gogo proto not sure why.

1b52421

Signed-off-by: Jianfei Hu <jianfeih@google.com>

adjust the memory expectation in stats_integration_test.cc

d3dea4b

Signed-off-by: Jianfei Hu <jianfeih@google.com>

repokitteh-read-only bot added the waiting label Sep 17, 2019

mattklein123 self-assigned this Sep 17, 2019

update the comment to be more verbose.

91e7e3d

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly dismissed lizan’s stale review via 91e7e3d September 18, 2019 19:07

repokitteh-read-only bot removed the waiting label Sep 18, 2019

Jianfei Hu added 3 commits September 18, 2019 19:08

fix the nitpick

86c943d

Signed-off-by: Jianfei Hu <jianfeih@google.com>

format fix.

6d864ee

Signed-off-by: Jianfei Hu <jianfeih@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into tra…

6b26039

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly commented Sep 19, 2019

View reviewed changes

mattklein123 requested changes Sep 19, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 19, 2019

fix cds.proto comments.

14db82c

Signed-off-by: Jianfei Hu <jianfeih@google.com>

repokitteh-read-only bot removed the waiting label Sep 19, 2019

mattklein123 previously approved these changes Sep 19, 2019

View reviewed changes

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

add validation rule.

29ada60

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly dismissed mattklein123’s stale review via 29ada60 September 20, 2019 17:48

htuch previously approved these changes Sep 20, 2019

View reviewed changes

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

fix type of accpetPlaintext

127f896

Signed-off-by: Jianfei Hu <jianfeih@google.com>

incfly dismissed htuch’s stale review via 127f896 September 20, 2019 18:18

Merge branch 'master' of https://github.com/envoyproxy/envoy into tra…

69ef905

…nsport-socket-matcher Signed-off-by: Jianfei Hu <jianfeih@google.com>

htuch approved these changes Sep 23, 2019

View reviewed changes

htuch merged commit 1f7f90f into envoyproxy:master Sep 23, 2019

alyssawilk mentioned this pull request Sep 24, 2019

api: fixing format build #8345

Merged

htuch pushed a commit that referenced this pull request Sep 24, 2019

api: fixing format build (#8345)

e07dc53

#8309 and #8100 collided Risk Level: Low (cleanup) Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

api: fixing format build (envoyproxy#8345)

6bb0c09

envoyproxy#8309 and envoyproxy#8100 collided Risk Level: Low (cleanup) Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a transport socket match in cluster. #8100

Add a transport socket match in cluster. #8100

incfly commented Aug 30, 2019 •

edited

Loading

incfly commented Aug 30, 2019

htuch commented Aug 30, 2019

htuch commented Aug 30, 2019

rshriram commented Aug 30, 2019

lizan commented Aug 30, 2019

incfly commented Aug 30, 2019

repokitteh-read-only bot commented Sep 3, 2019

incfly commented Sep 3, 2019

lizan Sep 3, 2019

rshriram Sep 3, 2019

lizan Sep 3, 2019 •

edited

Loading

htuch Sep 3, 2019

incfly Sep 4, 2019

incfly Sep 4, 2019

htuch Sep 16, 2019

incfly Sep 17, 2019

rshriram commented Sep 11, 2019

incfly commented Sep 12, 2019

repokitteh-read-only bot commented Sep 12, 2019

incfly commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

incfly commented Sep 19, 2019

incfly Sep 19, 2019

mattklein123 left a comment

mattklein123 left a comment

incfly commented Sep 20, 2019

htuch left a comment

htuch left a comment

Add a transport socket match in cluster. #8100

Add a transport socket match in cluster. #8100

Conversation

incfly commented Aug 30, 2019 • edited Loading

incfly commented Aug 30, 2019

htuch commented Aug 30, 2019

htuch commented Aug 30, 2019

rshriram commented Aug 30, 2019

lizan commented Aug 30, 2019

incfly commented Aug 30, 2019

repokitteh-read-only bot commented Sep 3, 2019

incfly commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan Sep 3, 2019 • edited Loading

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

rshriram commented Sep 11, 2019

incfly commented Sep 12, 2019

repokitteh-read-only bot commented Sep 12, 2019

incfly commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

incfly commented Sep 19, 2019

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

incfly commented Sep 20, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

incfly commented Aug 30, 2019 •

edited

Loading

lizan Sep 3, 2019 •

edited

Loading