Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.205.0
    • Component/s: None
    • Labels:
      None

      Description

      After the patches on Friday, org.apache.hadoop.hdfs.security.TestDelegationToken is failing.

      1. h-7625.patch
        7 kB
        Owen O'Malley
      2. h-7625.patch
        7 kB
        Owen O'Malley

        Issue Links

          Activity

          Hide
          Owen O'Malley added a comment -

          The fix was two fold:

          1. Update the build.xml to use the core jar instead of the classes, so that the service lookup works correctly.

          2. Fix the deprecated DistributedFileSystem.getDelegationToken and DFSClient.getDelegationToken to correctly set the service.

          Show
          Owen O'Malley added a comment - The fix was two fold: 1. Update the build.xml to use the core jar instead of the classes, so that the service lookup works correctly. 2. Fix the deprecated DistributedFileSystem.getDelegationToken and DFSClient.getDelegationToken to correctly set the service.
          Hide
          Daryn Sharp added a comment -

          I think you need to also handle the case in DFSClient where the ctor is passed a null nameNodeAddr.

          Show
          Daryn Sharp added a comment - I think you need to also handle the case in DFSClient where the ctor is passed a null nameNodeAddr .
          Hide
          Jitendra Nath Pandey added a comment -

          What is the reason for moving setting of service from DistributedFileSystem (DFS) to DFSClient? One flip side I see is that DFSClient now needs to store the nnAddress, but DFS can get it from URI.

          Token#setKind
          Please add a comment why renewer is being set to null in setKind.

          Show
          Jitendra Nath Pandey added a comment - What is the reason for moving setting of service from DistributedFileSystem (DFS) to DFSClient? One flip side I see is that DFSClient now needs to store the nnAddress, but DFS can get it from URI. Token#setKind Please add a comment why renewer is being set to null in setKind.
          Hide
          Owen O'Malley added a comment -

          I believe the null nameNodeAddr is only done in some of the unit tests that aren't creating delegation tokens. I haven't checked extensively.

          Show
          Owen O'Malley added a comment - I believe the null nameNodeAddr is only done in some of the unit tests that aren't creating delegation tokens. I haven't checked extensively.
          Hide
          Owen O'Malley added a comment -

          Yeah, I just went through the 4 callers of DFSClient that pass in non-null Client protocols and they are all testing append or retry in the client. There is no service to set in those cases and we are better off getting a runtime error in that case.

          Show
          Owen O'Malley added a comment - Yeah, I just went through the 4 callers of DFSClient that pass in non-null Client protocols and they are all testing append or retry in the client. There is no service to set in those cases and we are better off getting a runtime error in that case.
          Hide
          Owen O'Malley added a comment -

          Jitendra,
          The previous code would not set the service if the user called DFSClient.getDelegationToken, which is broken. It was also only setting it for the non-deprecated DistributedFileSystem.getDelegationToken.

          Show
          Owen O'Malley added a comment - Jitendra, The previous code would not set the service if the user called DFSClient.getDelegationToken, which is broken. It was also only setting it for the non-deprecated DistributedFileSystem.getDelegationToken.
          Hide
          Owen O'Malley added a comment -

          Added the requested comment about clearing the renewer cache in Token.setKind.

          Show
          Owen O'Malley added a comment - Added the requested comment about clearing the renewer cache in Token.setKind.
          Hide
          Daryn Sharp added a comment -

          The comment hasn't clarified the reasoning (for me) behind blanking out the renewer when the kind is changed. Twiddling the kind is only used to trick hftp into renewing hdfs tokens, so it doesn't seem like the renewer field should be changed? The comment alludes to the renewer being looked up again. How does this happen?

          Show
          Daryn Sharp added a comment - The comment hasn't clarified the reasoning (for me) behind blanking out the renewer when the kind is changed. Twiddling the kind is only used to trick hftp into renewing hdfs tokens, so it doesn't seem like the renewer field should be changed? The comment alludes to the renewer being looked up again. How does this happen?
          Hide
          Owen O'Malley added a comment -

          I don't think there is currently a case where a token will have a non-null renewer when setKind is called, but it could.

          When isManaged, renew or cancel are called on a token, it first calls getRenewer. getRenewer returns the renewer field, if it is set or otherwise it finds the right kind of renewer based on the kind. When you change the kind, generally the discovered renewer will be different. Therefore, to avoid accidentally using the wrong renewer, it is safer to clear it when we set the kind.

          Show
          Owen O'Malley added a comment - I don't think there is currently a case where a token will have a non-null renewer when setKind is called, but it could. When isManaged, renew or cancel are called on a token, it first calls getRenewer. getRenewer returns the renewer field, if it is set or otherwise it finds the right kind of renewer based on the kind. When you change the kind, generally the discovered renewer will be different. Therefore, to avoid accidentally using the wrong renewer, it is safer to clear it when we set the kind.
          Hide
          Jitendra Nath Pandey added a comment -

          +1. Looks good to me.

          Show
          Jitendra Nath Pandey added a comment - +1. Looks good to me.
          Hide
          Daryn Sharp added a comment -

          If the renewer field is defined, it appears to mean that a renew or cancel has already been performed on the token. Will it get dicey to change the kind on a token that it already being manipulated by something else?

          Show
          Daryn Sharp added a comment - If the renewer field is defined, it appears to mean that a renew or cancel has already been performed on the token. Will it get dicey to change the kind on a token that it already being manipulated by something else?
          Hide
          Daryn Sharp added a comment -

          +1 if what I mentioned before is not a problem

          Show
          Daryn Sharp added a comment - +1 if what I mentioned before is not a problem
          Hide
          Owen O'Malley added a comment -

          I've committed this to 205 and 20-s. It will be committed to trunk with MAPREDUCE-2764.

          Show
          Owen O'Malley added a comment - I've committed this to 205 and 20-s. It will be committed to trunk with MAPREDUCE-2764 .
          Hide
          Kihwal Lee added a comment -

          After commit of this jira HADOOP-7649 has been failing.

          Show
          Kihwal Lee added a comment - After commit of this jira HADOOP-7649 has been failing.
          Hide
          Kihwal Lee added a comment -

          After the commit of this jira, two tests have been failing. See HADOOP-7649.

          Show
          Kihwal Lee added a comment - After the commit of this jira, two tests have been failing. See HADOOP-7649 .
          Hide
          Matt Foley added a comment -

          Closed upon release of 0.20.205.0

          Show
          Matt Foley added a comment - Closed upon release of 0.20.205.0
          Hide
          Eli Collins added a comment -

          Hey Owen,

          How does using the core jar (instead of the test classes dir) cause the ServiceLoader work? Wouldn't it be better to specify the renewer classes explicitly in build.xml for the tests using the <service>/<provider> tags?

          Thanks,
          Eli

          Show
          Eli Collins added a comment - Hey Owen, How does using the core jar (instead of the test classes dir) cause the ServiceLoader work? Wouldn't it be better to specify the renewer classes explicitly in build.xml for the tests using the <service>/<provider> tags? Thanks, Eli
          Hide
          Eli Collins added a comment -

          Ah, looks like from HADOOP-7644 it doesn't, a services file is still needed.

          Show
          Eli Collins added a comment - Ah, looks like from HADOOP-7644 it doesn't, a services file is still needed.

            People

            • Assignee:
              Owen O'Malley
              Reporter:
              Owen O'Malley
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development