It's supposed to be acceptable to pass a null renewer. Since the renewer is a Text, the typical problem is code leaving off a null check before calling renewer.toString().

If someone doesn't take this, I'll get to it in the next few days since I'm already touching tokens.

Hey Daryn, any update here? I just bumped into this myself.

I'll look into it. It's probably a 1-line fix.

It's potentially more than a 1-liner. The NN used to generate (apparently by design) a non-renewable token if the renewer is null. The PB stuff broke the ability to pass a null renewer. OTOH, the hftp servlet defaults the renewer to the client's UGI when passed a null renewer.

The quick and dirty answer is for the token fetcher to default the renewer to UGI's login user unless the renewer flag is explicitly provided. This breaks the ability to request a non-renewable hdfs token, but as mentioned earlier, PB broke that anyway.

Or should the PB code be fixed to allow a null renewer once again, and perhaps for consistency change hftp to not default a null renewer to the client's UGI?

Seems to me (without a ton of expertise in this area of the code) that we should treat it as a bug in the PB-ification and restore the previous behavior of non-renewable tokens.

I am leaning towards fixing it to be like it was before, but wanted input before making the change. Does anyone else disagree?

I am leaning towards fixing it to be like it was befor

+1

Here we are defining the protocol message as

message GetDelegationTokenRequestProto {
  required string renewer = 1;
}

based on some of the above comments I feel the renewer should be optional (since null can be passed I mean we are not providing renewer).

Even with optional we will have the generated class with null check for renewer, so I guess we can have null check for renewer like

if(renewer != null) {
    GetDelegationTokenRequestProto req = GetDelegationTokenRequestProto
        .newBuilder()
        .setRenewer(renewer.toString())
        .build();
    } else {
      GetDelegationTokenRequestProto req = GetDelegationTokenRequestProto
      .newBuilder()
      .build();
    }    

This should be possible since we are declaring the renewer optional, similarly we can parse the message back at serverside translator.

Please correct me if I am wrong

Hey Daryn, any update on this?

Unfortunately I've been too consumed with other token work to double back to this jira... I can spare time to review if you want to take a stab at a patch.

Thanks for the update, Daryn. Unassigning from you.

Attaching the patch, Please review

Made the renewer optional as per above comments.

I'm not a protobuf expert, but I think downgrading a required field to optional is ok for compatibility but someone else should definitively comment.

Only nit is the condition renewer.toString() != null) is unnecessary because it will never return null.

Hi Aaron T. Myers, could you please take a look at the patch as you already aware of the issue..?
Thanks

You actually can't safely downgrade a PB field from required to optional in a compatible fashion. If you did so, the side trying to deserialize a message that still has the field marked "required" will consider a message missing this field to be a malformed message. See the section on this page entitled "Required is Forever."

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12600339/HDFS-2956.patch
against trunk revision 47f7f18.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA
org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager
org.apache.hadoop.hdfs.TestAppendSnapshotTruncate

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9811//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9811//console

This message is automatically generated.

Attaching a patch.
Passing a empty renewer string if renewer not passed.

-1 overall

This message was automatically generated.

Test failures are unrelated to current patch. Those are related to HADOOP-12009.

Hi Vinayakumar B, thank you for taking this issue. My concern is, when passing empty string renewer to a request, the request will fail under any condition. If the concern is true, it's better to fail fast by checking if --renewer argument exists in DelegationTokenFetcher#main.

IMO, Without renewer, non-renewable delegationToken should be able to fetch, But renew should fail.

Attached a test also to verify the same.

-1 overall

This message was automatically generated.

Last patch was generated from branch-2.

Now attaching the patch for trunk.

Thanks Vinayakumar B for the clarification and creating the regression test. +1 pending Jenkins.

Attaching the branch-2 patch.
Since DelegationTokenFetcher differs b/w trunk and branch-2, test also needs update in branch-2.
cherry-pick will not work.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Findbugs is not actually there.
trunk test failures are not related.
branch-2 checkstyles are also not related.

Akira Ajisaka, Thanks for the reviews.

Could you also take a look at the branch-2 patch? This is different in test.

Findbugs is not actually there.

Not related to the patch. Filed HDFS-8749.

Thanks Vinayakumar B for creating the patch for branch-2. Mostly looks good to me. I noticed there is a difference in the test between trunk and branch-2.

      // make sure we got back exactly the 1 token we expected
      assertTrue(itr.hasNext());
      assertNotNull("Token without renewer shouldn't be null", itr.next());
      assertTrue(!itr.hasNext());

     assertTrue("token not exist error", itr.hasNext());
     assertNotNull("Token should be there without renewer", itr.next());

I'm thinking we can add assertTrue(!itr.hasNext()) to the trunk as well.

Updated the trunk patch as per comments

Attached the patch with correct name now.

-1 overall

This message was automatically generated.

test failure unrelated

+1

Thanks for the reviews Akira Ajisaka, I will commit the patch soon.

-1 overall

This message was automatically generated.

Committed to trunk and branch-2.
Thanks Akira Ajisaka for the final reviews.
Thanks Aaron T. Myers, Daryn Sharp, Todd Lipcon for earlier discussions and reviews.

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #252 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/252/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Yarn-trunk #982 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/982/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java

FAILURE: Integrated in Hadoop-trunk-Commit #8147 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8147/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #2179 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2179/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2198 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2198/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #240 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/240/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java

SUCCESS: Integrated in Hadoop-Mapreduce-trunk-Java8 #250 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/250/)
HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B)HDFS-2956. calling fetchdt without a --renewer argument throws NPE (Contributed by Vinayakumar B) (vinayakumarb: rev b48908033fcac7a4bd4313c1fd1457999fba08e1)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java