Skip to content

change xDS certificate provider to contain a map by cluster#25120

Merged
markdroth merged 1 commit intogrpc:masterfrom
markdroth:xds_security_aggregate_cluster_fix
Jan 12, 2021
Merged

change xDS certificate provider to contain a map by cluster#25120
markdroth merged 1 commit intogrpc:masterfrom
markdroth:xds_security_aggregate_cluster_fix

Conversation

@markdroth
Copy link
Copy Markdown
Member

@markdroth markdroth commented Jan 8, 2021

Currently, the CDS policy creates a single XdsCertificateProvider object 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 the XdsCertificateProvider to 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 XdsCertificateProvider object 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 XdsCertificateProvider when 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.

@markdroth markdroth added the release notes: no Indicates if PR should not be in release notes label Jan 8, 2021
@markdroth markdroth force-pushed the xds_security_aggregate_cluster_fix branch 4 times, most recently from 3a81f99 to bb85730 Compare January 11, 2021 20:17
@markdroth markdroth requested a review from yashykt January 11, 2021 21:17
@markdroth markdroth marked this pull request as ready for review January 11, 2021 21:17
Copy link
Copy Markdown
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to xds_certificate_provider_test.cc to verify that using and updating configs for different clusters has the correct effect.

@markdroth
Copy link
Copy Markdown
Member Author

Test added. PTAL.

@markdroth markdroth force-pushed the xds_security_aggregate_cluster_fix branch from abd60c7 to 8ddbc50 Compare January 12, 2021 18:52
@markdroth markdroth merged commit 11f53c3 into grpc:master Jan 12, 2021
@markdroth markdroth deleted the xds_security_aggregate_cluster_fix branch January 12, 2021 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants