HADOOP-18956. Zookeeper SSL/TLS support in ZKDelegationTokenSecretManager and ZKSignerSecretProvider #6263
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
…to avoid FinalClass error in checkstyle, as the class is inherited by a Mockito spy, and final classes cannot be spied on, and because actually instantiating the class via the constructor is not a problem at all, configure() method is a convenience for readability anyway.)
|
Adjusted the code to cope with warnings. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
I ran all the failed tests locally, for me these are passing in my local environment, also none of them seems to be related to the changeset. |
|
Change looks OK to me, but it would be good to get a pass from someone else too, as I have never looked at this area before. |
jojochuang
reviewed
Nov 14, 2023
| DFSConfigKeys.DEFAULT_ZK_CLIENT_SSL_ENABLED); | ||
| return conf.getBoolean(CommonConfigurationKeys.ZK_CLIENT_SSL_ENABLED, | ||
| conf.getBoolean(DFSConfigKeys.ZK_CLIENT_SSL_ENABLED, | ||
| DFSConfigKeys.DEFAULT_ZK_CLIENT_SSL_ENABLED)); |
There was a problem hiding this comment.
This would imply dfs.ha.zkfc.client.ssl.enabled becomes defunct. And perhaps we should update hdfs-default.xml too.
Why is it different from YARN? In YARN, yarn.resourcemanager.zk-client-ssl.enabled takes effect if hadoop.zk.ssl.enabled is not set.
There was a problem hiding this comment.
The idea is to use the hadoop.zk.ssl.enabled as a central configuration option, if that is set to false or true, then that effectively forces all Hadoop services to use SSL, while if hadoop.zk.ssl.enabled is not set, then different components can still set it on their own without affecting other components.
I am not sure I understand what do you mean, as I see Yarn, HDFS and Common components does this the same way in the patch. Do I miss something? Also if the idea is bad, or goes against other similar things, I am open to adjust the behaviour of these configs it just feels straightforward this way.
For the keystore and truststore data I think we need it the other way around, and we need to use the component specific setting first, and fall back to the service specific setting.
There was a problem hiding this comment.
Ah my bad. Misread this. That said, we should update the description of dfs.ha.zkfc.client.ssl.enabled in hdfs-default.xml.
jojochuang
reviewed
Nov 14, 2023
…iguration resolution for ZK ssl enablement.
|
Thank you for the review @sodonnel and @jojochuang, much appreciated. I have added extra information about the precedence order of ssl.enabled configuration options. |
|
+1 thanks @fapifta |
|
💔 -1 overall
This message was automatically generated. |
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…ager and ZKSignerSecretProvider (apache#6263)
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.
Description of PR
Zookeeper based delegation token and Zookeeper based signer secret provider is not able to use an SSL/TLS based communication even if Zookeeper is able to handle such connections.
The pull request standardizes how ZKDelegationTokenSecretManager, and ZKSignerSecretProvider creates their respective CuratorFramework instance (as they are anyway interconnected).
In the new code, they both utilize o.a.h.security.authentication.util.ZookeeperClient class to configure the client based on their respective configuration values.
The change also introduces a new property in CommonConfigurationKeys, that affects YARN as well.
The new key is
hadoop.zk.ssl.enabled. With this new configuration the following dynamics are true:hadoop.zk.ssl.enabledis set then YARN will respect that and won't use the value inyarn.resourcemanager.zk-client-ssl.enabledto decide if SSL is enabled.hadoop.zk.ssl.enabledis set then the ZKDelegationTokenSecretManager will respect that and won't use the value inzk-dt-secret-manager.ssl.enabledto decide if SSL is enabled.signer.secret.provider.zookeeper.ssl.enabledproperty wich defaults to false.hadoop.zk.ssl.enabledprior to evaluatingdfs.ha.zkfc.client.ssl.enabled.hadoop.zk.ssl.enableddoes not have a default value set.The intent is to make it possible to enable SSL/TLS towards Zookeeper at once, or for all 4 places separately if one wish to do so.
ZkDelegationTokenSecretManager, and ZKSignerSecretProvider has their own Truststore and Keystore overrides, those take precedence over the related hadoop.zk.* properties, so custom keystores and truststores can be configured even if the common setup is already set. (DFSZKFailoverController and YARN uses the hadoop.zk.* properties and does not have custom properties to set the truststore and keystore as it was implemented earlier).
How was this patch tested?
Added a JUnit test that checks how the new class introduced to create the CuratorFramework instance sets up the builder. From that point on it is Curator's responsibility to use the configuration as expected.
Additionally some other tests should cover the functionality that should be provided exactly the same way as before.