flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] ChengbingLiu commented on a change in pull request #15810: [FLINK-22534] Set delegation token's service name as credential alias
Date Wed, 12 May 2021 02:45:50 GMT

ChengbingLiu commented on a change in pull request #15810:
URL: https://github.com/apache/flink/pull/15810#discussion_r630686815

File path: flink-runtime/src/main/java/org/apache/flink/runtime/security/modules/HadoopModule.java
@@ -97,8 +97,7 @@ public void install() throws SecurityInstallException {
                     // If UGI use keytab for login, do not load HDFS delegation token.
                     for (Token<? extends TokenIdentifier> token : usrTok) {
                         if (!token.getKind().equals(hdfsDelegationTokenKind)) {
-                            final Text id = new Text(token.getIdentifier());
-                            credentialsToBeAdded.addToken(id, token);
+                            credentialsToBeAdded.addToken(token.getService(), token);

Review comment:
       > E.g. when `token.service` is changed, do we want it to be reflected in the `Credentials::tokenMap`?
   No, we don't want that. But that should not happen with this change, explained below.
   > However, a similar change is made in `Utils::setTokensFor` where `usrTok` is retrieved
from current UGI and therefore is not local to the method. Will that be an issue for us?
   I don't think so. In `Utils::setTokensFor`, all tokens retrieved from the UGI is used to
construct the local variable `credentials`, which is then used to be serialized into `ByteBuffer
securityTokens`, which will not reflect the change in the UGI.
   > It seems both `Text(byte[])` and `Text(Text)` copy the byte array. So previously to
this change, we do make a deep copy, no?
   Sorry for not making myself clear. I think the purpose of the previous code `new Text(token.getIdentifier())`
was type conversion instead of deep copy, otherwise it should look like `addToken(new Text(...),
new Token(token))`.

This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:

View raw message