[
https://issues.apache.org/jira/browse/HADOOP-14445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16028710#comment-16028710
]
Yongjun Zhang commented on HADOOP-14445:
----------------------------------------
HI [~shahrs87],
Thanks much for the patch and [~jojochuang] and [~asuresh] for the discussion
and review.
I did a review and here are my comments.
1. about new field "clientProvidedUriText"
a. typo "clientProvidedUriText" should be "clientProviderUriText"
b. make it private instead of public, and add accessor methods
c. AND provide a comment on the format of its value.
2. I would like to see a description of the solution as a detailed comment
somewhere in the code, maybe right at the new field of item #1.
3. Suggest to change
{code}
public static KeyProvider createKeyProviderFromToken(final Configuration conf,
final String tokenServiceValue) throws IOException {
{code}
to
{code}
public static KeyProvider createKeyProviderFromTokenService(final
Configuration conf,
final String tokenService) throws IOException {
{code}
3.
{code}
/**
* This method is created for testing of @LoadBalancingKMSClientProvider
* since multiple kms servers in unit tests will only differ in port number
* and multiple kms servers in real world application are designed to
* share the same port number.
* @param conf
* @param tokenServiceValue
* @return
* @throws IOException
*/
public static KeyProvider createKeyProviderFromToken(final Configuration conf,
final String tokenServiceValue) throws IOException {
if (tokenServiceValue == null) {
return null;
}
if (!tokenServiceValue.contains(",")) {
return createKeyProviderFromUri(conf, URI.create(tokenServiceValue));
}
// The following code will be executed in just unit tests.
// The syntax for kms servers will be
// kms://localhost:port1/kms,kms://localhost:port2/kms
String[] keyProviderUrisStr = tokenServiceValue.split(",");
KMSClientProvider[] keyProviderArr =
new KMSClientProvider[keyProviderUrisStr.length];
{code}
a. Comment need to be changed to say that "," only appear in tokenServiceValue
of unit tests because different ports need to be used in unit tests.
b. And indicate what format to expect for production code, and what format to
expect for unit test.
c. We can separate the code used for test only to a separate method and call
this method in the above method, to make it more clear.
4. With this solution, if we add a new kms server, the currently running
application will not be able to use the new kms server.
And if we replace an existing kms server with a different one, currently
running application may fail. So the KMS HA may not be really HA.
This may not be a big concern, but I would like to bring it up here.
Thanks.
> Delegation tokens are not shared between KMS instances
> ------------------------------------------------------
>
> Key: HADOOP-14445
> URL: https://issues.apache.org/jira/browse/HADOOP-14445
> Project: Hadoop Common
> Issue Type: Bug
> Components: documentation, kms
> Affects Versions: 2.8.0, 3.0.0-alpha1
> Reporter: Wei-Chiu Chuang
> Assignee: Rushabh S Shah
> Attachments: HADOOP-14445-branch-2.8.patch
>
>
> As discovered in HADOOP-14441, KMS HA using LoadBalancingKMSClientProvider do
> not share delegation tokens. (a client uses KMS address/port as the key for
> delegation token)
> {code:title=DelegationTokenAuthenticatedURL#openConnection}
> if (!creds.getAllTokens().isEmpty()) {
> InetSocketAddress serviceAddr = new InetSocketAddress(url.getHost(),
> url.getPort());
> Text service = SecurityUtil.buildTokenService(serviceAddr);
> dToken = creds.getToken(service);
> {code}
> But KMS doc states:
> {quote}
> Delegation Tokens
> Similar to HTTP authentication, KMS uses Hadoop Authentication for delegation
> tokens too.
> Under HA, A KMS instance must verify the delegation token given by another
> KMS instance, by checking the shared secret used to sign the delegation
> token. To do this, all KMS instances must be able to retrieve the shared
> secret from ZooKeeper.
> {quote}
> We should either update the KMS documentation, or fix this code to share
> delegation tokens.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
