Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.205.0
    • Component/s: None
    • Labels:
      None

      Description

      The token's service field must (currently) be set to "ip:port". All the producers of a token are independently building the service string. This should be done via a common method to reduce the chance of error, and to facilitate the field value being easily changed in the (near) future.

      1. MAPREDUCE-2780.patch
        15 kB
        Daryn Sharp
      2. MAPREDUCE-2780-2.patch
        15 kB
        Daryn Sharp
      3. MAPREDUCE-2780-3.patch
        23 kB
        Daryn Sharp
      4. MAPREDUCE-2780-4.patch
        21 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Daryn Sharp added a comment -

          The changes were integrated to trunk/2.0 via HADOOP-7808 and its related jiras.

          Show
          Daryn Sharp added a comment - The changes were integrated to trunk/2.0 via HADOOP-7808 and its related jiras.
          Hide
          Jitendra Nath Pandey added a comment -

          Hi Daryn, Can you please provide a trunk patch too?

          Show
          Jitendra Nath Pandey added a comment - Hi Daryn, Can you please provide a trunk patch too?
          Hide
          Jitendra Nath Pandey added a comment -

          I have committed this to 20-security. Thanks to Daryn!

          Show
          Jitendra Nath Pandey added a comment - I have committed this to 20-security. Thanks to Daryn!
          Hide
          Daryn Sharp added a comment -

          Ran "ant test". Only tests that failed where marked with @Ignore.

          Show
          Daryn Sharp added a comment - Ran "ant test". Only tests that failed where marked with @Ignore.
          Hide
          Daryn Sharp added a comment -

          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 15 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
          [exec]

          Show
          Daryn Sharp added a comment - [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 15 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. [exec]
          Hide
          Jitendra Nath Pandey added a comment -

          +1 for the patch.

          Show
          Jitendra Nath Pandey added a comment - +1 for the patch.
          Hide
          Daryn Sharp added a comment -

          Revert unintended (but harmless) changes to Token. Reduced visibility of the buildDTAuthority methods per Jitendra's advice.

          Show
          Daryn Sharp added a comment - Revert unintended (but harmless) changes to Token. Reduced visibility of the buildDTAuthority methods per Jitendra's advice.
          Hide
          Daryn Sharp added a comment -

          Changes to use the compromise of a static SecurityUtil. Thanks Jitendra!

          Show
          Daryn Sharp added a comment - Changes to use the compromise of a static SecurityUtil. Thanks Jitendra!
          Hide
          Jitendra Nath Pandey added a comment -

          Daryn and I discussed it at length, the conclusions were:
          It is better to treat service as opaque, as in current Token API.
          A convenience method to set service using InetSocketAddress is useful.
          Therefore we add a method in SecurityUtil that takes a token and an InetSocketAddress, constructs a service and sets it in the token. And the Token API remains unchanged.

          Show
          Jitendra Nath Pandey added a comment - Daryn and I discussed it at length, the conclusions were: It is better to treat service as opaque, as in current Token API. A convenience method to set service using InetSocketAddress is useful. Therefore we add a method in SecurityUtil that takes a token and an InetSocketAddress, constructs a service and sets it in the token. And the Token API remains unchanged.
          Hide
          Daryn Sharp added a comment -

          It forces to set ip/host/port in the service.

          Per the title of this jira, that is the goal. Every token producer is required to use the same service field format to comply with the format needed by the token's selector. Currently every token producer has a copy-n-paste chunk selector's code to construct the service format. If the selector and the producer get out of sync, there's a big problem.

          If we later decide to store something else in the service for example, uri with scheme, we will have to change it again.

          Please elaborate? This appears as a non-sequitur since token producers that choose to use a URI (someday), for instance, will require a change in either case.

          Here is the evolution from the original code of:

          token.setService(new Text(addr.getAddress().getHostAddress() + ":" + addr.getPort()));
          selector.selectToken(new Text(addr.getAddress().getHostAddress() + ":" + addr.getPort()), tokens);
          

          I think is your suggestion? It's incrementally better, but continues to require a copy-n-paste in each token producer, and every token producer continues to have intimate knowledge of the service format.

          token.setService(SecurityUtil.buildDTAuthority(addr));
          selector.selectToken(SecurityUtil.buildDTAuthority(addr), tokens);
          

          The patch applies another layer of abstraction. The format is privatized to the token, instead of publicly diffused over all the tokens in hadoop.

          token.setService(addr);
          selector.selectToken(Token.createService(addr), tokens);
          // I removed this due to your earlier concerns in the parent jira
          // selector.selectToken(addr, tokens);
          

          Do you believe this a persuasive case for the patch?

          Show
          Daryn Sharp added a comment - It forces to set ip/host/port in the service. Per the title of this jira, that is the goal. Every token producer is required to use the same service field format to comply with the format needed by the token's selector. Currently every token producer has a copy-n-paste chunk selector's code to construct the service format. If the selector and the producer get out of sync, there's a big problem. If we later decide to store something else in the service for example, uri with scheme, we will have to change it again. Please elaborate? This appears as a non-sequitur since token producers that choose to use a URI (someday), for instance, will require a change in either case. Here is the evolution from the original code of: token.setService( new Text(addr.getAddress().getHostAddress() + ":" + addr.getPort())); selector.selectToken( new Text(addr.getAddress().getHostAddress() + ":" + addr.getPort()), tokens); I think is your suggestion? It's incrementally better, but continues to require a copy-n-paste in each token producer, and every token producer continues to have intimate knowledge of the service format. token.setService(SecurityUtil.buildDTAuthority(addr)); selector.selectToken(SecurityUtil.buildDTAuthority(addr), tokens); The patch applies another layer of abstraction. The format is privatized to the token, instead of publicly diffused over all the tokens in hadoop. token.setService(addr); selector.selectToken(Token.createService(addr), tokens); // I removed this due to your earlier concerns in the parent jira // selector.selectToken(addr, tokens); Do you believe this a persuasive case for the patch?
          Hide
          Jitendra Nath Pandey added a comment -

          I see only one issue with changing the setService API to take InetSocketAddress. It forces to set ip/host/port in the service. If we later decide to store something else in the service for example, uri with scheme, we will have to change it again.
          I would recommend to keep setService API unchanged, however a utility method in SecurityUtil to construct DT service from a socket address is good.

          Show
          Jitendra Nath Pandey added a comment - I see only one issue with changing the setService API to take InetSocketAddress. It forces to set ip/host/port in the service. If we later decide to store something else in the service for example, uri with scheme, we will have to change it again. I would recommend to keep setService API unchanged, however a utility method in SecurityUtil to construct DT service from a socket address is good.
          Hide
          Daryn Sharp added a comment -

          Oops, posted earlier rev of this patch. Small change to keep the service field encoding completely encapsulated within the Token class.

          Show
          Daryn Sharp added a comment - Oops, posted earlier rev of this patch. Small change to keep the service field encoding completely encapsulated within the Token class.
          Hide
          Daryn Sharp added a comment -

          Add Token.setService(InetSocketAddress) and change all token producers to call the method instead of constructing the same string.

          Show
          Daryn Sharp added a comment - Add Token.setService(InetSocketAddress) and change all token producers to call the method instead of constructing the same string.

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Daryn Sharp
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development