[
https://issues.apache.org/jira/browse/HADOOP-14445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16412915#comment-16412915
]
Rushabh S Shah commented on HADOOP-14445:
-----------------------------------------
Thanks [~xiaochen] for the revision.
[~daryn] shared his concerns offline that this patch is dependent on config
key and we will live with the baggage of 2 tokens forever.
I will request him to review asap.
But below are my review comments Mostly all are minor.
+KMSClientProvider.java+
1. {{KMSCP#addTokenToCredentials}}
I don't agree with the method name. The method name suggests that we are just
adding token to credentials object.
But we are doing much more in this method.
I would add {{credentials.addToken}} line to calling method
{{addDelegationTokens}} and rename method {{addTokenToCredentials}} to
something like {{createLegacyKmsToken}}
and add some javadoc to it saying that creating this token is dependent on
conf key {{KMS_CLIENT_COPY_LEGACY_TOKEN_KEY}}.
+DelegationTokenAuthenticatedURL.java+
1. Lets change the {{selectDelegationToken}} to {{getDelegationToken}}.
In the base implementation, we are not implementing any Selector.
We are just getting the token based on the service field.
In {{KMSCP}} we can change the method name {{getKMSToken}} to
{{selectKMSToken}} because there we are implementing {{TokenSelector}}.
+core-default.xml+
{quote}With the default value set to true, the client will locally duplicate
the KMS_DELEGATION_TOKEN token and create a kms-dt token, all other parts of
the token remain the same.
{quote}
Technically this is not true. The service is also changed.
I am sorry I _should have_ mentioned all the above comments in the previous
review.
+TestKMS.java+
1. This is unrelated to patch.
Do we really want to stop kdc after every test ?
2. {{providersCreated}}: Should this be a list or just {{KeyProvider}} ?
It will always create {{LoadBalancingKeyProivder}} which internally is a set
of {{KeyProvider}}.
{{LoadBalancingKMSCP}} never throws IOException back. It just swallows all the
{{IOException}} and just logs it.
Maybe we might want to return MultipleIOException from
{{LoadBalancingKMSCP#close}}. Totally fine if we want to do it as a separate
jira.
But as far as this jira is concerned, we can get rid of
{{MultipleIOException}} related changes and can just change it to
{{IOException}}.
3. {{testKMSHAZKDelegationTokenRenewCancel(final Text tokenKind)}}
Unable to understand why were are calling {{setupConfForToken}} multiple times.
If we filter out all tokens other than passed {{tokenKind}}, then we can just
call {{setupConfForToken}} once at the start.
That way the code will be more clean and _will only_ work on {{token ==
tokenKind}}.
In short just one for loop, filter out the token at the start and test renew,
cancel and again renew(which should fail) in sequence.
4. {{testTokenCompatibilityOldRenewer}}
This test ran for {{34.887}} on my local machine.
I am sure majority of time was spent in sleeping for token to expire.
Can we decrease the {{renewInterval}} period to less than 30 seconds (maybe 5
seconds or so).
Also the test renewed the token 30 times. Is that expected ?
Did you mean to renew after while the code came out of while loop ?
{quote}LOG.info("Rolling key {} via provider {} with tokenUGi.",keyName);
kp.createKey("newkey", new KeyProvider.Options(conf));
{quote}
The log line should be {{creating a new key}} instead of {{Rolling key}}.
Let me know if any part is unclear.
> 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: kms
> Affects Versions: 2.8.0, 3.0.0-alpha1
> Environment: CDH5.7.4, Kerberized, SSL, KMS-HA, at rest encryption
> Reporter: Wei-Chiu Chuang
> Assignee: Xiao Chen
> Priority: Major
> Attachments: HADOOP-14445-branch-2.8.002.patch,
> HADOOP-14445-branch-2.8.patch, HADOOP-14445.002.patch,
> HADOOP-14445.003.patch, HADOOP-14445.004.patch, HADOOP-14445.05.patch,
> HADOOP-14445.06.patch, HADOOP-14445.07.patch, HADOOP-14445.08.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
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
