hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rushabh S Shah (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-14445) Delegation tokens are not shared between KMS instances
Date Sun, 25 Mar 2018 06:44:00 GMT

    [ 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: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message