change xDS certificate provider to contain a map by cluster#25120
Merged
markdroth merged 1 commit intogrpc:masterfrom Jan 12, 2021
Merged
change xDS certificate provider to contain a map by cluster#25120markdroth merged 1 commit intogrpc:masterfrom
markdroth merged 1 commit intogrpc:masterfrom
Conversation
3a81f99 to
bb85730
Compare
yashykt
requested changes
Jan 11, 2021
Member
yashykt
left a comment
There was a problem hiding this comment.
Please add a test to xds_certificate_provider_test.cc to verify that using and updating configs for different clusters has the correct effect.
Member
Author
|
Test added. PTAL. |
yashykt
approved these changes
Jan 12, 2021
abd60c7 to
8ddbc50
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, the CDS policy creates a single
XdsCertificateProviderobject containing the certificate configuration for the entire cluster. That provider gets passed down via channel args to all of the underlying subchannels. This approach poses a problem for the aggregate cluster functionality being implemented by @donnadionne in #25078. For an aggregate cluster, the cluster will be composed of more than one underlying cluster, each of which may have different certificate configuration, so we have to pass down a different configuration for different sets of subchannels. This PR solves that problem by changing theXdsCertificateProviderto contain a map, so that it can contain a different certificate configuration for each cluster.Previously, whenever the xDS certificate configuration changed, the CDS policy would create a new
XdsCertificateProviderobject containing the new config. The new object would be passed down via channel args, and the new channel args would cause all of the subchannels to be recreated. However, this approach will no longer work, because in the aggregate cluster case, if the certificate config changes for just one of the underlying clusters, we don't want to cause the subchannels to be recreated for all clusters. So instead, we will need to change the XdsCreds code to notice when the config for the cluster has changed and create a new security connector when that happens.The latter change has not been fully implemented as part of this PR, because it turns out that there's a pre-existing bug: The XdsCreds code is creating a new TlsCreds object every time it creates a security connector, but the security connector's pointer to its channel creds object is part of its identity, so this causes the channel args to be different, which means that we're actually recreating the subchannels every single time there's an xDS update of any kind. This clearly needs to be fixed anyway, so I've left a TODO for @yashykt to change the XdsCreds code to more intelligently decide when to change the security connector, which should be done only when the config for the cluster has changed.
For now, though, I have removed the code in the CDS policy that recreated the
XdsCertificateProviderwhen a config changes. Instead, it creates a single object and simply updates the map within that object as the configs for the underlying cluster change.