Hadoop YARN
  1. Hadoop YARN
  2. YARN-47 [Umbrella] Security issues in YARN
  3. YARN-503

DelegationTokens will be renewed forever if multiple jobs share tokens and the first one sets JOB_CANCEL_DELEGATION_TOKEN to false

    Details

    • Type: Sub-task Sub-task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.23.3, 3.0.0, 2.0.0-alpha
    • Fix Version/s: None
    • Component/s: resourcemanager
    • Labels:
      None

      Description

      The first Job/App to register a token is the one which DelegationTokenRenewer associates with a a specific Token. An attempt to remove/cancel these shared tokens by subsequent jobs doesn't work - since the JobId will not match.
      As a result, Even if subsequent jobs have MRJobConfig.JOB_CANCEL_DELEGATION_TOKEN set to true - tokens will not be cancelled when those jobs complete.
      Tokens will eventually be removed from the RM / JT when the service that issued them considers them to have expired or via an explicit cancelDelegationTokens call (not implemented yet in 23).
      A side affect of this is that the same delegation token will end up being renewed multiple times (a separate TimerTask for each job which uses the token).

      DelegationTokenRenewer could maintain a reference count/list of jobIds for shared tokens.

      1. YARN-503.patch
        34 kB
        Daryn Sharp
      2. YARN-503.patch
        34 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Siddharth Seth added a comment -

          I don't like sleeps either. 1s is an eternity in this case because the initial renew and cancel timer tasks fire immediately on mocked objects, so it should run in a few ms. I assume you are suggesting using notify in a mock'ed answer method? Multiple timers are expected to fire in some cases, so it would probably require something like a CountdownLatch, which will get tricky to keep swapping in a new one by re-adding mocked responses with the new latch. Let me know if you feel it's worth it to change it.

          Was actually suggesting doing the post-sleep verify in a check-sleep loop, instead of just sleeping. Passing this step indicates the required execution has completed. Would prefer keeping a sleep out of the tests if we can. Otherwise a longer sleep for sure.

          It's ok if another task is executing, because it's just trying to abort any pending task. Since there's only one possible pending task per token at any given time, the wrong task can't be cancelled. Did I miss an edge case?

          I think there's an edge case. Sequence
          1. [t1] timerTask is a RenewalTask
          2. [t1] timer kicks in and starts executing
          3. [t2] scheduleCancelled gets called in a parallel thread [via AppRemovalTask]
          4. [t2] scheduleCancelled.abortScheduled called - synchronized but does nothing useful since the current task is already running.
          5. [t2] scheduleCancelled runs to completion and creates a cancelTask
          6. [t1] completes execution - and calls scheduleTask(new TokenRenewTask(), renewIn) - which effectively destorys the scheduled cancelTask

          Are you suggesting to move managedToken.add(appId) into the loop in addApplication? I was trying to encapsulate the implementation details of adding/removing the appId within ManagedApp. Is it ok to leave it as-is?

          I thought it'd be cleaner leaving this outside of ManagedApp - ManagedApp should not be managing ManagedTokens. IAC, don't feel strongly about this; whatever you decide.

          Show
          Siddharth Seth added a comment - I don't like sleeps either. 1s is an eternity in this case because the initial renew and cancel timer tasks fire immediately on mocked objects, so it should run in a few ms. I assume you are suggesting using notify in a mock'ed answer method? Multiple timers are expected to fire in some cases, so it would probably require something like a CountdownLatch, which will get tricky to keep swapping in a new one by re-adding mocked responses with the new latch. Let me know if you feel it's worth it to change it. Was actually suggesting doing the post-sleep verify in a check-sleep loop, instead of just sleeping. Passing this step indicates the required execution has completed. Would prefer keeping a sleep out of the tests if we can. Otherwise a longer sleep for sure. It's ok if another task is executing, because it's just trying to abort any pending task. Since there's only one possible pending task per token at any given time, the wrong task can't be cancelled. Did I miss an edge case? I think there's an edge case. Sequence 1. [t1] timerTask is a RenewalTask 2. [t1] timer kicks in and starts executing 3. [t2] scheduleCancelled gets called in a parallel thread [via AppRemovalTask] 4. [t2] scheduleCancelled.abortScheduled called - synchronized but does nothing useful since the current task is already running. 5. [t2] scheduleCancelled runs to completion and creates a cancelTask 6. [t1] completes execution - and calls scheduleTask(new TokenRenewTask(), renewIn) - which effectively destorys the scheduled cancelTask Are you suggesting to move managedToken.add(appId) into the loop in addApplication? I was trying to encapsulate the implementation details of adding/removing the appId within ManagedApp. Is it ok to leave it as-is? I thought it'd be cleaner leaving this outside of ManagedApp - ManagedApp should not be managing ManagedTokens. IAC, don't feel strongly about this; whatever you decide.
          Hide
          Daryn Sharp added a comment -

          There's likely a race between the RenewalTask and AbortTask. [...] I think it's possible for a schedulesTask to be executing - in which case the abortScheduleTask() may have no affect and can result in the wrong task being cancelled / scheduled.

          It's ok if another task is executing, because it's just trying to abort any pending task. Since there's only one possible pending task per token at any given time, the wrong task can't be cancelled. Did I miss an edge case?

          ManagedApp.add - instead of adding to the app and the token here, this composite add can be kept outside of ManagedApp/ManagedToken

          Not sure I follow. Are you suggesting to move managedToken.add(appId) into the loop in addApplication? I was trying to encapsulate the implementation details of adding/removing the appId within ManagedApp. Is it ok to leave it as-is?

          ManagedApp.expunge() - is synchronization on 'appTokens' required ?

          Strictly speaking, probably not. It's a throwback to earlier implementation that was doing trickier stuff. It was to avoid concurrent modification exceptions while iterating, but appTokens isn't mutating in multiple threads. And the remove is essentially guarding it too. For that matter, I don't think appTokens needs to be a synch-ed set. I'll change it.

          MangedToken.expunge() - tokenApps.clear() required ?

          Probably not. Seemed like good housekeeping, but I'll remove it.

          In the unit test - the 1 second sleep seems rather low. Instead of the sleep, this can be changed to a timed wait on one of the fields being verified.

          I don't like sleeps either. 1s is an eternity in this case because the initial renew and cancel timer tasks fire immediately on mocked objects, so it should run in a few ms. I assume you are suggesting using notify in a mock'ed answer method? Multiple timers are expected to fire in some cases, so it would probably require something like a CountdownLatch, which will get tricky to keep swapping in a new one by re-adding mocked responses with the new latch. Let me know if you feel it's worth it to change it.

          Show
          Daryn Sharp added a comment - There's likely a race between the RenewalTask and AbortTask. [...] I think it's possible for a schedulesTask to be executing - in which case the abortScheduleTask() may have no affect and can result in the wrong task being cancelled / scheduled. It's ok if another task is executing, because it's just trying to abort any pending task. Since there's only one possible pending task per token at any given time, the wrong task can't be cancelled. Did I miss an edge case? ManagedApp.add - instead of adding to the app and the token here, this composite add can be kept outside of ManagedApp/ManagedToken Not sure I follow. Are you suggesting to move managedToken.add(appId) into the loop in addApplication ? I was trying to encapsulate the implementation details of adding/removing the appId within ManagedApp. Is it ok to leave it as-is? ManagedApp.expunge() - is synchronization on 'appTokens' required ? Strictly speaking, probably not. It's a throwback to earlier implementation that was doing trickier stuff. It was to avoid concurrent modification exceptions while iterating, but appTokens isn't mutating in multiple threads. And the remove is essentially guarding it too. For that matter, I don't think appTokens needs to be a synch-ed set. I'll change it. MangedToken.expunge() - tokenApps.clear() required ? Probably not. Seemed like good housekeeping, but I'll remove it. In the unit test - the 1 second sleep seems rather low. Instead of the sleep, this can be changed to a timed wait on one of the fields being verified. I don't like sleeps either. 1s is an eternity in this case because the initial renew and cancel timer tasks fire immediately on mocked objects, so it should run in a few ms. I assume you are suggesting using notify in a mock'ed answer method? Multiple timers are expected to fire in some cases, so it would probably require something like a CountdownLatch, which will get tricky to keep swapping in a new one by re-adding mocked responses with the new latch. Let me know if you feel it's worth it to change it.
          Hide
          Siddharth Seth added a comment -

          Nice patch, much cleaner than the older code. Couple of comments though

          • There's likely a race between the RenewalTask and AbortTask. The actual tasks aren't synchronized in any way. I think it's possible for a schedulesTask to be executing - in which case the abortScheduleTask() may have no affect and can result in the wrong task being cancelled / scheduled.
          • ManagedApp.add - instead of adding to the app and the token here, this composite add can be kept outside of ManagedApp/ManagedToken
          • ManagedApp.expunge() - is synchronization on 'appTokens' required ?
          • MangedToken.expunge() - tokenApps.clear() required ?
          • In the unit test - the 1 second sleep seems rather low. Instead of the sleep, this can be changed to a timed wait on one of the fields being verified.
          Show
          Siddharth Seth added a comment - Nice patch, much cleaner than the older code. Couple of comments though There's likely a race between the RenewalTask and AbortTask. The actual tasks aren't synchronized in any way. I think it's possible for a schedulesTask to be executing - in which case the abortScheduleTask() may have no affect and can result in the wrong task being cancelled / scheduled. ManagedApp.add - instead of adding to the app and the token here, this composite add can be kept outside of ManagedApp/ManagedToken ManagedApp.expunge() - is synchronization on 'appTokens' required ? MangedToken.expunge() - tokenApps.clear() required ? In the unit test - the 1 second sleep seems rather low. Instead of the sleep, this can be changed to a timed wait on one of the fields being verified.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12577915/YARN-503.patch
          against trunk revision .

          +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. The javadoc tool did not generate any warning messages.

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

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

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

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          +1 contrib tests. The patch passed contrib unit tests.

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

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12577915/YARN-503.patch against trunk revision . +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 . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/698//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/698//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Fixed findbugs issues. Upon review of those issues, I realized I was trying too hard to prevent tight & unlikely race conditions.

          I simplified the design further which allowed for the removal of virtually all the synchronization which could lead to tricky deadlocks.

          Updated tests to ensure apps and/or tokens don't leak.

          Show
          Daryn Sharp added a comment - Fixed findbugs issues. Upon review of those issues, I realized I was trying too hard to prevent tight & unlikely race conditions. I simplified the design further which allowed for the removal of virtually all the synchronization which could lead to tricky deadlocks. Updated tests to ensure apps and/or tokens don't leak.
          Hide
          Daryn Sharp added a comment -

          Will the case of MR actions be an issue? that the launcher goes away?

          No, the central focus of this patch is to keep tokens alive as long as at least one job is using the tokens. Upon job submission, the new app is immediately linked against the tokens. So for an oozie action, it's ok for the launcher to exit after submitting an action. The tokens will stay alive until the action, and any sub-jobs it may have launched, have completed. After no app is running with the tokens, and the keepalive expires, the tokens are cancelled.

          Note that by default I maintained 100% backwards compat in that tokens for oozie jobs setting the " mapreduce.job.complete.cancel.delegation.tokens=false" will never be cancelled. The RM will stop renewing them and won't issue duplicate renews. Until we deprecate/remove the setting, we may internally try make the conf setting a final to see what happens.

          Will address findbugs after some webhdfs firefighting.

          Show
          Daryn Sharp added a comment - Will the case of MR actions be an issue? that the launcher goes away? No, the central focus of this patch is to keep tokens alive as long as at least one job is using the tokens. Upon job submission, the new app is immediately linked against the tokens. So for an oozie action, it's ok for the launcher to exit after submitting an action. The tokens will stay alive until the action, and any sub-jobs it may have launched, have completed. After no app is running with the tokens, and the keepalive expires, the tokens are cancelled. Note that by default I maintained 100% backwards compat in that tokens for oozie jobs setting the " mapreduce.job.complete.cancel.delegation.tokens=false" will never be cancelled. The RM will stop renewing them and won't issue duplicate renews. Until we deprecate/remove the setting, we may internally try make the conf setting a final to see what happens. Will address findbugs after some webhdfs firefighting.
          Hide
          Alejandro Abdelnur added a comment -

          Oozie should be unaffected because as long as the launcher job or any child job is running the tokens will remain valid. Alejandro Abdelnur, can you confirm?

          In the case of Oozie MR actions, the launcher job ends as soon as the MR job is started, then Oozie tracks the real MR job.

          For all other actions (pig, hive, sqoop, distcp, java, shell) the launcher job blocks (because of the corresponding Main class) until the child/ren job/s end.

          Will the case of MR actions be an issue? that the launcher goes away?

          Show
          Alejandro Abdelnur added a comment - Oozie should be unaffected because as long as the launcher job or any child job is running the tokens will remain valid. Alejandro Abdelnur, can you confirm? In the case of Oozie MR actions, the launcher job ends as soon as the MR job is started, then Oozie tracks the real MR job. For all other actions (pig, hive, sqoop, distcp, java, shell) the launcher job blocks (because of the corresponding Main class) until the child/ren job/s end. Will the case of MR actions be an issue? that the launcher goes away?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12575120/YARN-503.patch
          against trunk revision .

          +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 tests included appear to have a timeout.

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

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

          -1 findbugs. The patch appears to introduce 4 new Findbugs (version 1.3.9) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/581//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/581//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/581//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12575120/YARN-503.patch against trunk revision . +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 tests included appear to have a timeout. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 4 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/581//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/581//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Console output: https://builds.apache.org/job/PreCommit-YARN-Build/581//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Overhaul and simplify the RM's token renewer. Apps now track their tokens, and tokens track their apps. The 3 background threads - renewer, canceler, delayed canceler - are consolidated into a single timer thread.

          Tokens are cancelled after all their active apps have completed, unless an app requested the token not be cancelled (compatibility), in which case the renewal is simply stopped. This solves the multiple timers per token, and persistent renewal issues.

          This change should allow the deprecation and removal of support for not canceling tokens.

          Show
          Daryn Sharp added a comment - Overhaul and simplify the RM's token renewer. Apps now track their tokens, and tokens track their apps. The 3 background threads - renewer, canceler, delayed canceler - are consolidated into a single timer thread. Tokens are cancelled after all their active apps have completed, unless an app requested the token not be cancelled (compatibility), in which case the renewal is simply stopped. This solves the multiple timers per token, and persistent renewal issues. This change should allow the deprecation and removal of support for not canceling tokens.
          Hide
          Daryn Sharp added a comment -

          Siddharth Seth I almost have a patch ready that tracks tokens against apps and cancels when the last app completes (honoring the keepalive of course). I am contemplating whether this patch negates the need for MRJobConfig.JOB_CANCEL_DELEGATION_TOKEN? It appears to be a "hack" for oozie & pig? If so, I think we can deprecate it, and possibly even noop it.

          Oozie should be unaffected because as long as the launcher job or any child job is running the tokens will remain valid. Alejandro Abdelnur, can you confirm? Likewise for pig run under oozie, and pig run directly should already be getting new tokens during top-level job submissions. The only case where I can think of where it would potentially cause problems is if something manually gets its own tokens, and then explicitly stuffs them into multiple job submissions.

          Show
          Daryn Sharp added a comment - Siddharth Seth I almost have a patch ready that tracks tokens against apps and cancels when the last app completes (honoring the keepalive of course). I am contemplating whether this patch negates the need for MRJobConfig.JOB_CANCEL_DELEGATION_TOKEN ? It appears to be a "hack" for oozie & pig? If so, I think we can deprecate it, and possibly even noop it. Oozie should be unaffected because as long as the launcher job or any child job is running the tokens will remain valid. Alejandro Abdelnur , can you confirm? Likewise for pig run under oozie, and pig run directly should already be getting new tokens during top-level job submissions. The only case where I can think of where it would potentially cause problems is if something manually gets its own tokens, and then explicitly stuffs them into multiple job submissions.

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Siddharth Seth
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:

                Development