-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: enhance v3 CommonTlsContext for agentless support #11061
Changes from all commits
f74cb76
5e8f97b
097d099
ed9d51f
afffb45
90c2424
6bc63f8
cd60ee9
2d93cfe
d09a366
71b3508
94ce3fb
f3be260
8e09f75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,15 @@ syntax = "proto3"; | |
|
||
package envoy.extensions.transport_sockets.tls.v3; | ||
|
||
import "envoy/config/core/v3/extension.proto"; | ||
import "envoy/extensions/transport_sockets/tls/v3/common.proto"; | ||
import "envoy/extensions/transport_sockets/tls/v3/secret.proto"; | ||
|
||
import "google/protobuf/any.proto"; | ||
import "google/protobuf/duration.proto"; | ||
import "google/protobuf/wrappers.proto"; | ||
|
||
import "udpa/annotations/migrate.proto"; | ||
import "udpa/annotations/status.proto"; | ||
import "udpa/annotations/versioning.proto"; | ||
import "validate/validate.proto"; | ||
|
@@ -96,10 +99,30 @@ message DownstreamTlsContext { | |
} | ||
|
||
// TLS context shared by both client and server TLS contexts. | ||
// [#next-free-field: 9] | ||
// [#next-free-field: 11] | ||
message CommonTlsContext { | ||
option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.CommonTlsContext"; | ||
|
||
// Config for Certificate provider to get certificates. This provider should allow certificates to be | ||
// fetched/refreshed over the network asynchronously with respect to the TLS handshake. | ||
message CertificateProvider { | ||
// opaque name used to specify certificate instances or types. For example, "ROOTCA" to specify | ||
// a root-certificate (validation context) or "TLS" to specify a new tls-certificate. | ||
string name = 1 [(validate.rules).string = {min_bytes: 1}]; | ||
|
||
// Provider specific config. | ||
// Note: an implementation is expected to dedup multiple instances of the same config | ||
// to maintain a single certificate-provider instance. The sharing can happen, for | ||
// example, among multiple clusters or between the tls_certificate and validation_context | ||
// certificate providers of a cluster. | ||
// This config could be supplied inline or (in future) a named xDS resource. | ||
oneof config { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a required validation to the oneof? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
option (validate.required) = true; | ||
|
||
config.core.v3.TypedExtensionConfig typed_config = 2; | ||
} | ||
} | ||
|
||
message CombinedCertificateValidationContext { | ||
option (udpa.annotations.versioning).previous_message_type = | ||
"envoy.api.v2.auth.CommonTlsContext.CombinedCertificateValidationContext"; | ||
|
@@ -108,9 +131,19 @@ message CommonTlsContext { | |
CertificateValidationContext default_validation_context = 1 | ||
[(validate.rules).message = {required: true}]; | ||
|
||
// Config for fetching validation context via SDS API. | ||
SdsSecretConfig validation_context_sds_secret_config = 2 | ||
[(validate.rules).message = {required: true}]; | ||
// Config for fetching validation context via SDS API. Note SDS API allows certificates to be | ||
// fetched/refreshed over the network asynchronously with respect to the TLS handshake. | ||
// Only to be used when validation_context_certificate_provider is not used. | ||
SdsSecretConfig validation_context_sds_secret_config = 2 [ | ||
(validate.rules).message = {required: true}, | ||
(udpa.annotations.field_migrate).oneof_promotion = "dynamic_validation_context" | ||
]; | ||
|
||
// Certificate provider for fetching validation context - only to be used when | ||
// validation_context_sds_secret_config is not used. | ||
// [#not-implemented-hide:] | ||
CertificateProvider validation_context_certificate_provider = 3 | ||
[(udpa.annotations.field_migrate).oneof_promotion = "dynamic_validation_context"]; | ||
} | ||
|
||
reserved 5; | ||
|
@@ -126,15 +159,21 @@ message CommonTlsContext { | |
// used for clients that support ECDSA. | ||
repeated TlsCertificate tls_certificates = 2; | ||
|
||
// Configs for fetching TLS certificates via SDS API. | ||
// Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be | ||
// fetched/refreshed over the network asynchronously with respect to the TLS handshake. | ||
repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6 | ||
[(validate.rules).repeated = {max_items: 1}]; | ||
|
||
// Certificate provider for fetching TLS certificates. | ||
// [#not-implemented-hide:] | ||
CertificateProvider tls_certificate_certificate_provider = 9; | ||
|
||
oneof validation_context_type { | ||
// How to validate peer certificates. | ||
CertificateValidationContext validation_context = 3; | ||
|
||
// Config for fetching validation context via SDS API. | ||
// Config for fetching validation context via SDS API. Note SDS API allows certificates to be | ||
// fetched/refreshed over the network asynchronously with respect to the TLS handshake. | ||
SdsSecretConfig validation_context_sds_secret_config = 7; | ||
|
||
// Combined certificate validation context holds a default CertificateValidationContext | ||
|
@@ -145,6 +184,10 @@ message CommonTlsContext { | |
// CertificateValidationContext, and concatenates repeated fields to default | ||
// CertificateValidationContext, and logical OR is applied to boolean fields. | ||
CombinedCertificateValidationContext combined_validation_context = 8; | ||
|
||
// Certificate provider for fetching validation context. | ||
// [#not-implemented-hide:] | ||
CertificateProvider validation_context_certificate_provider = 10; | ||
} | ||
|
||
// Supplies the list of ALPN protocols that the listener should expose. In | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to provide hooks here to allow the config to be fetched as a separate xDS resource type? I am thinking that if there are a large number of Cluster and Listener resources that contain the same CertificateProvider, if we want to change the CertificateProvider, it would be nice to just send a single update for the CertificateProvider instead of sending an update for every single Cluster and Listener that is affected. But I'm also not sure how common this use-case might be.
I'd be interested in feedback from @mattklein123 and @htuch on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @htuch answered it here #11061 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, didn't see that earlier thread. Sorry for the duplication.
I am fine with actually adding this in a separate PR, but I do think it's worth considering how we would structure the API to make that work. It would be nice to set this up in a way that doesn't add unnecessary roadblocks to making that change later.
For example, let's say that this PR gets merged as-is right now. If we later wanted to add support for fetching these configs as a separate resource type, what would we need to change? It seems like we'd need the ability to specify a
ConfigSource
and a resource name to request. And presumably, we'd want to be able to specify either an inline config or an external xDS resource to use, so there'd have to be aoneof
. So we'd probably want something like this here:Does that look right? Would we be okay handling this with a
oneof_promotion
annotation, or should we preemptively add theoneof
in this PR, even if it currently contains only one option?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, generally right but your
typed_config
is ofconfig.core.v3.TypedExtensionConfig
type and I was thinking instead:The
CPDS.name
is the unique resource name that returns theCertificateProvider
resource and theCertificateProvider.name
is then used as I have described above. If everyone (other than me:-)) feels that name is not at all needed then I'll get rid ofCertificateProvider
struct and just useconfig.core.v3.TypedExtensionConfig
instead.Regarding
oneof
, it is best to add the oneof now (instead ofoneof_promotion
) if there is the issue of backward compatibility (which I think there is) even if we don't defineCPDS
yet. Theoneof_promotion
creates oneof for v4alpha but we may end up making this change before we move from v3 to v4alpha.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the underlying question here is whether the resource returned by CDPS would be the whole
CertificateProvider
message or only the typed config for the plugin instance. The main difference is whether the name of the certificate being requested lives here where CDPS would be called or in the CDPS resource. If it is in the CDPS resource, then we will need a separate CDPS resource for every named certificate we will use, even if the entire rest of the config is the same. That seems inefficient.Let's say that you have two CertificiateProvider entries in a Cluster. They both have the same config, but they differ by the name of the certificate that we are asking for: one sets the name to "ROOTCA" and the other sets it to "TLS". I think you would want a single CDPS resource for this plugin, and the places where we refer to this plugin would specify which certificate name to request from the plugin. The advantage of this approach becomes even more clear in a situation where different clusters use different certificates, but they all come from the same plugin instance.
Note that this is actually a question about what the plugin API is going to look like. Will a plugin instance allow registering a watcher for a particular name (e.g.,
void StartWatch(std::string name, Watcher* watcher);
), or will it assume that all watchers are for the same name (e.g.,void StartWatch(Watcher* watcher);
)? If the name of the certificate to request is a first-class citizen in this API, then I think it should be the former.I think this also relates to the question of requiring plugin implementations to internally share instances. When we were discussing (offline from this PR) the question of how this would work in gRPC, we said that we wanted to be able to reuse plugin instances in the framework, so that individual plugin implementations don't each need to do this. I suspect that Envoy will want that same consideration, but @htuch and @mattklein123 can weigh in on this.
I think my main point here is that we need to flesh out some of the details of how this is going to be implemented in both gRPC and Envoy in order to know how to structure this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to defer to all of you on what you think is best, but if we think that we want to add referential behavior soon(ish) I would absolutely preemptively add a oneof now even if it only has a single value currently. We do this all over the place to future proof similar situations.
This part is an implementation detail obviously, but yes, if possible we would de-dup plugins probably based on config via the singleton manager. We also use this pattern elsewhere. (@htuch does not like this from an API perspective, but it's a way of de-duping without a generic or API specific reference system.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we've adopted this as a pattern in a few places now (dedup in the client based on config), I think it's totally reasonable to make this standard for v3 at least. If we find that there are some exceptions that this pattern of dedup really causes confusion for or is not effective in practice, we can change as we move towards v4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, if we're comfortable de-duping based on the config, then the way this is now is fine. Let's just add a
oneof
around thetyped_config
field, so that we have the option of adding CPDS later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanjay, we still need to add the
oneof
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's in my TODO list