xds: bootstrapper fixes: remove extra readBootstrap & avoid parseConfig #7436
Conversation
| @@ -34,8 +34,8 @@ | |||
| private final CertProviderClientSslContextProvider.Factory | |||
| certProviderClientSslContextProviderFactory; | |||
|
|
|||
| ClientSslContextProviderFactory() { | |||
| this(Bootstrapper.getInstance(), CertProviderClientSslContextProvider.Factory.getInstance()); | |||
| ClientSslContextProviderFactory(Bootstrapper bootstrapper) { | |||
There was a problem hiding this comment.
Is this change necessary? For tests you are using the other constructor, but for real instantiation it just calls Bootstrapper.getInstance() inside the constructor to get the default implementation. That doesn't sound wrong. It doesn't need to leave this burden to the caller of this class.
There was a problem hiding this comment.
This is highly desirable. The Bootstrapper.getInstance() is now called in only one place: io.grpc.xds.internal.sds.TlsContextManagerImpl.getInstance() and that instance is then propagated to all the members. Similar to how the abstract Factories are used wherever possible.
There was a problem hiding this comment.
It doesn't matter as Bootstrap.getInstance returns the singleton.
There was a problem hiding this comment.
Yes, but it returns an implementation that is fixed and that reads from disk.
| ServerSslContextProviderFactory() { | ||
| this(Bootstrapper.getInstance(), CertProviderServerSslContextProvider.Factory.getInstance()); | ||
| ServerSslContextProviderFactory(Bootstrapper bootstrapper) { | ||
| this(bootstrapper, CertProviderServerSslContextProvider.Factory.getInstance()); |
There was a problem hiding this comment.
Same comment as for ClientSslContextProviderFactory.
There was a problem hiding this comment.
Same answer as above.
| CertificateProvider.DistributorWatcher distWatcher = | ||
| new CertificateProvider.DistributorWatcher(); | ||
| Map<String, ?> map = buildMinimalConfig(); | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, ?> map = (Map<String, ?>) JsonParser.parse(MINIMAL_MESHCA_CONFIG); |
There was a problem hiding this comment.
nit: map is a bad variable name. Same for other places.
No description provided.