api: Add CertificateProviderInstance to CommonTlsContext.

[markdroth]

Signed-off-by: Mark D. Roth roth@google.com

Commit Message: api: Add CertificateProviderInstance to CommonTlsContext.
Additional Description: This is similar to the CertificateProvider API added in #11061, but it allows the actual plugin instances to be configured on the client side.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A

CC @htuch
@jiangtaoli2016 @yihuazhang
@jaychenatr @karthikbox
@costinm @louiscryan
@easwars @sanjaypujare @ejona86 @dfawley @yashykt @srini100

[repokitteh-read-only]

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

🐱

Caused by: #12237 was opened by markdroth.

see: more, trace.

[costinm]

"TLS" may not be a good example - "example.com" ?

[markdroth]

Done.

[sanjaypujare]

to specify a new tls-certificate.

Why "new"? If the same plugin_instance_name and certificate_name combination is used for a different cluster (or such) will the provider provide the same certificate or has to mint a new one?

[sanjaypujare]

Would be good to mention this is optional depending on the plugin_instance i.e. some plugins may require it and others may ignore it.

[markdroth]

I copied this text from what you added in #11061. :) But I've changed the wording.

[sanjaypujare]

"and validation_context_certificate_provider is not used"?

[markdroth]

Done.

[sanjaypujare]

Previously someone commented on the use of the word "plugin" in this context - IIRC the preferred word is "extension". Will need to be changed in the name of the field and the comments. But I am okay with this word.

[markdroth]

Done.

[sanjaypujare]

LGTM but left some suggestions for the content in the comments.

[jiangtaoli2016]

Since CertificateProvider has not been implemented with [#not-implemented-hide:]
Why not replace/merge CertificateProvider with CertificateProviderInstance?

If someone uses old CertificateProvider, it infers that there is only one CertificateProvider. CertificateProvider is a special case of CertificateProviderInstance with the default instance.

[sanjaypujare]

@jiangtaoli2016 we cannot replace CertificateProvider since that option (of supplying the config from the control plane) should be supported.

We discussed merging CertificateProviderInstance into CertificateProvider but decided in favor of keeping them separate for a clean-looking API.

If someone uses old CertificateProvider, it infers that there is only one CertificateProvider.

Not true. Treat them as separate entities in their own "space". The control plane can still send multiple CertificateProvider configs each with distinct plugin-name and/or config.

CertificateProvider is a special case of CertificateProviderInstance with the default instance.

Actually CertificateProviderInstance can be considered as a special case of CertificateProvider where the control plane tells the plugin to pick its config locally from a bootstrap file. But all of this has been discussed and we have decided to keep them separate.

[markdroth]

I think you're misunderstanding how the CertificateProvider field is intended to work. That field does not imply that there is only one provider instance; the cardinality is exactly the same in both CertificateProvider and CertificateProviderInstance.

In both cases, the API can specify a different provider instance for each of the identity and root certificates, which means that there can be either one or two provider instances per CommonTlsContext. And each Listener (for incoming connections) and Cluster (for outgoing connections) can specify a different CommonTlsContext, and there is no limit to the number of Listeners or Clusters used by a given client. So there is no limit to the number of provider instances that could be used by a given client (although in practice, I would not expect the number to be large).

In both cases, the client will maintain a global map of currently instantiated provider instances. However, the map is structured a little bit differently between the two cases:

• In the CertificateProviderInstance case, the set of instances in the map is statically configured in the bootstrap file, and the map is keyed by the instance name assigned in the bootstrap file. That instance name is sent by the control plane to tell the client which of its configured instances to use.
• In the CertificateProvider case, the set of instances in the map will change dynamically based on the set of CertificateProvider configurations sent by the control plane. The map is keyed by the provider (implementation) name and config in the CertificateProvider typed_config field. Therefore, if two different CertificateProviders use the same typed_config, they will wind up using the same instance.

The reason we are not replacing the CertificateProvider field is that we think there may be cases in the future where we will want to send the provider config from the control plane. We're not planning to implement support for the CerttificateProvider field in gRPC until/unless we need to (and we currently don't have anyone signed up to implement either of these fields in Envoy), but there's no point in removing it if we think we may want it later.

[jiangtaoli2016]

I meant merge two as follows (which seems cleaner).

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 certificate_name = 1 [(validate.rules).string = {min_bytes: 1}];

    // Provider specific config.
    oneof config {
      option (validate.required) = true;
      // provider config
      config.core.v3.TypedExtensionConfig provider_config = 2;
      // provider name
      string provider_name = 3;
    }
  }

I agree we need to support the control plane to supply which certificate provider instance to use. The difference is the certificate provider config is from local config or supplied by the control plane. Either way, The control plane needs to tell which cert provider to use.

If you already decided, it is fine. I won't hold you back.

[markdroth]

Sanjay actually suggested the same thing. I'm the one who has been advocating for keeping them separate.

I think they are actually sufficiently different intents that they should be expressed separately in the API. For example, a client implementation that supports both mechanisms is likely to have a separate global map for each of the two mechanisms rather than sharing a single map.

That having been said, if the consensus from everyone else is to combine them in the API, I can probably live with that.

[markdroth]

As I said earlier, Sanjay originally suggested the same thing you did.

I'd like feedback from the Envoy folks on this.

[sanjaypujare]

Yes, my idea was to have a single one as described in #12237 (comment) .

[htuch]

@markdroth the question to me is "who will implement these?". If gRPC plans on doing both, then let's keep both, if we're only likely to see the bootstrap indirect in the near term, let's go with that for now.

[sanjaypujare]

Yes the plan is to keep and implement both (although not at the same time necessarily). I don't think we should take out anything. Also it will be good to get input from @arjayara01 .

[markdroth]

@htuch I think it's very likely that we'll still want CertificateProvider in the long term, but we're going to wait for a concrete use-case before implementing it. My inclination is to keep it for now, so that we don't need to re-add it later.

Given that the fields are already here, I assume we can't delete them anyway, right? I mean, in practice, I doubt anyone is using them yet, but technically it seems like we'd be breaking the API guarantee if we did.

[htuch]

This is a bit weird in xDS. Generally, when we name things, we put them in some kind of reverse DNS namespace, e.g. envoy.foo_provider or com.acme.bar_provider.

I think what you're trying to do here is create some rendezvous with the bootstrap, so this is pretty different. If that's the case, why imply any policy here at all?

[markdroth]

Yeah, this is different from those other cases. I think reverse-DNS might make sense for the plugin names themselves, but not for the plugin instances; those are just deployment-specific logical names in the bootstrap file. The distinction I'm trying to draw here is between those two things, since there was some confusion in our meeting between the plugin implementation name and the plugin instance name.

[htuch]

Can you provide a [#not-implemented-hide:] bootstrap implementation to demonstrate this use in Envoy? We probably need to have a tracking issue and idea of how this will eventually land. LGTM otherwise as discussed at the meeting.

[markdroth]

@htuch I've added the global map to the bootstrap file, as requested.

[htuch]

LGTM, will defer to @envoyproxy/api-shepherds for non-Google review and merge.