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:
[email protected]
