[
https://issues.apache.org/jira/browse/HADOOP-14445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16305093#comment-16305093
]
Xiao Chen commented on HADOOP-14445:
------------------------------------
Thanks a lot [~shahrs87] for the latest patch, looks good overall.
Some comments from code review:
KMSCP:
- I think we can improve the comments with {{dtService}} to be clearer. Suggest
something along the lines of:
{quote}
/* The value is read from
* CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH from
* the conf when creating the KeyProvider, and used as the token's service.
* With this value, later the KeyProviderFactory will be able to instantiate
* KeyProvider from the token to renew / cancel, without the need to read
* configs.
{quote}
- Now that we handle the port stuff nicely in unit tests,
{{fallbackDefaultPortForTesting}} and related logic can be removed.
- On {{renew}} and {{cancel}}, suggest to add a debug log when {{keyProvider ==
null}} for supportability.
- Let's use {{HADOOP_SECURITY_KEY_PROVIDER_PATH}} instead of
{{KeyProviderFactory.KEY_PROVIDER_PATH}}.
KMSUtil:
- When {{createKeyProviderForTests}} returns non-null value (before {{return
kp}}), add a info log, since this should only happen in tests
TestKMS:
- It's great to see extra assertions were done in {{getTokenService}}.
- {{doKMSWithZKWithDelegationToken}}, do we need to loop through the tokens and
verify? After this fix, there would only be 1 kms-dt mapping to the entire url
right? IMO we should verify there's just 1 kms-dt.
- {{doKMSWithZKWithDelegationToken}}, besides verifying renewal, we should also
verify some key operations.
- Happy to see the compat test, thanks! We should also verify some key
operations too here.
- Trivial, {{createKeyProviderForTests}} should have a line break after.
HdfsKMSUtil:
- Looks like we can remove the not-used {{createKeyProvider}} method.
There is also 1 thing that I think missed in the recent compat discussions:
For the clients to 'work', besides creating the kp and the token renewer, there
is also the complication of {{DelegationTokenAuthenticatedURL}} recognizing the
token. How would we handle the case there the job submitter's
{{DelegationTokenAuthenticatedURL#getDelegationTokenService}} did not match the
cluster's mapper's version, to prevent issues similar to HADOOP-14441? In other
words, if I submit a job using the new jar and get a new format token, but when
it's distributed to the cluster, the mappers are still on the old version which
tries to get the ip, it won't recognize it and will fall back to kerberos.
IMHO, since {{token.setService}} is done in KMSCP, what matters here would be
to require the job submitter (whoever gets the token) to be on the same version
of the mappers (or whatever is actually authenticating using the token).
Otherwise, handling this at code level bi-directional would require 1)
duplicating the same token with ip (for new submitter, old mapper) and 2)
falling back to check old format in DTAuthURL (for old submitter, new mapper),
which sounds tedious and ugly. Curious to learn the thoughts from you and
[~daryn].
> 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: Rushabh S Shah
> Attachments: HADOOP-14445-branch-2.8.002.patch,
> HADOOP-14445-branch-2.8.patch, HADOOP-14445.002.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.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
