-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
YARN-11738 Modernize SecretManager config #7144
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@K0K0V0K , thanks for working on this! If we change the HMAC and key length, then the keys generated before this change and after this change won't be the same. Then, this become an incompatible change. We probably need to add confs to set the algorithm and key length for backward compatibility. |
1352cfd
to
9854cee
Compare
Hi @szetszwo and @brumi1024 ! First of all thanks for the review. I modified the PR, now it is configurable. I know to create a configuration object at class static method is a bit risky, but that was the quick win what i found. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
...mmon-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
Make hash algorithm at SecretManager configurable. - hadoop.security.hmac-algorithm: The name of the hashing algorithm. Default: HmacSHA1 - hadoop.security.hmac-length: The length of the random keys to use. Default: 64 Change-Id: I735573c1d7b9f256e05722c98cd550cd8dd4acf0
🎊 +1 overall
This message was automatically generated. |
Hi @brumi1024 @szetszwo! May i ask a review again? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@K0K0V0K , thanks for the update! Please see the comments inlined.
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
Outdated
Show resolved
Hide resolved
...mmon-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java
Outdated
Show resolved
Hide resolved
...mmon-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
Outdated
Show resolved
Hide resolved
Change-Id: I8feab0406d46892daa954a1e41eb193f031ee77a
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@K0K0V0K , thanks for the update. There is a typo in a conf key. The change looks good other than that.
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
Outdated
Show resolved
Hide resolved
Change-Id: I8feab0406d46892daa954a1e41eb193f031ee77a
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
Outdated
Show resolved
Hide resolved
Change-Id: I8feab0406d46892daa954a1e41eb193f031ee77a
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 the change looks good.
Description of PR
YARN-11738
Make hash algorithm at SecretManager configurable.
How was this patch tested?