HBase
  1. HBase
  2. HBASE-10847

0.94: drop non-secure builds, make security the default

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.19
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I would like to only create a single 0.94 tarball/release that contains the security code - and drop the non-secure tarballs and releases.

      Let's discuss...

      1. 10847-v4.txt
        6 kB
        Lars Hofhansl
      2. 10847-v3.txt
        5 kB
        Lars Hofhansl
      3. 10847-v2.txt
        9 kB
        Lars Hofhansl
      4. 10847.txt
        4 kB
        Lars Hofhansl

        Activity

        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.94-JDK7 #97 (See https://builds.apache.org/job/HBase-0.94-JDK7/97/)
        HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480)

        • /hbase/branches/0.94/pom.xml
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.94-JDK7 #97 (See https://builds.apache.org/job/HBase-0.94-JDK7/97/ ) HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480) /hbase/branches/0.94/pom.xml /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.94-on-Hadoop-2 #62 (See https://builds.apache.org/job/HBase-0.94-on-Hadoop-2/62/)
        HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480)

        • /hbase/branches/0.94/pom.xml
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.94-on-Hadoop-2 #62 (See https://builds.apache.org/job/HBase-0.94-on-Hadoop-2/62/ ) HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480) /hbase/branches/0.94/pom.xml /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Hide
        Lars Hofhansl added a comment -

        The -security build failed with a known flaky test (need to look into that).

        Show
        Lars Hofhansl added a comment - The -security build failed with a known flaky test (need to look into that).
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-0.94 #1335 (See https://builds.apache.org/job/HBase-0.94/1335/)
        HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480)

        • /hbase/branches/0.94/pom.xml
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-0.94 #1335 (See https://builds.apache.org/job/HBase-0.94/1335/ ) HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480) /hbase/branches/0.94/pom.xml /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.94-security #452 (See https://builds.apache.org/job/HBase-0.94-security/452/)
        HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480)

        • /hbase/branches/0.94/pom.xml
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.94-security #452 (See https://builds.apache.org/job/HBase-0.94-security/452/ ) HBASE-10847 0.94: drop non-secure builds, make security the default. (larsh: rev 1583480) /hbase/branches/0.94/pom.xml /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithSecureRpcEngine.java
        Hide
        Lars Hofhansl added a comment -

        Committed to 0.94. I also changed the -security build to use the -security-test profile.
        Watching the builds now.

        Show
        Lars Hofhansl added a comment - Committed to 0.94. I also changed the -security build to use the -security-test profile. Watching the builds now.
        Hide
        Lars Hofhansl added a comment -

        What I am going to commit.

        Just fixes some typos and forces TestFromClientSide to run with WritableRpcEngine (which is not public, hence the string path to the class), since we have TestFromClientSideWithSecureRpcEngine, that will run with SecureRpcEngine unconditionally.

        Show
        Lars Hofhansl added a comment - What I am going to commit. Just fixes some typos and forces TestFromClientSide to run with WritableRpcEngine (which is not public, hence the string path to the class), since we have TestFromClientSideWithSecureRpcEngine, that will run with SecureRpcEngine unconditionally.
        Hide
        Jesse Yates added a comment -

        +1 lgtm

        Show
        Jesse Yates added a comment - +1 lgtm
        Hide
        Gary Helmling added a comment -

        +1

        This seems like a good approach. TestFromClientSideWithSecureRpcEngine should give us most of the coverage we need, but we still have the security-test profile to back that up.

        Thanks for taking this up, Lars! Should simplify the packaging (and security) story for 0.94 quite a bit.

        Show
        Gary Helmling added a comment - +1 This seems like a good approach. TestFromClientSideWithSecureRpcEngine should give us most of the coverage we need, but we still have the security-test profile to back that up. Thanks for taking this up, Lars! Should simplify the packaging (and security) story for 0.94 quite a bit.
        Hide
        Lars Hofhansl added a comment -

        Thanks Stack. Unless there are further comments I will commit this in a bit. (I might force TestFromClientSide to use the WritableRpcEngine - that we get a one test run with that rpc engine even when the secure-test profile is selected).

        Show
        Lars Hofhansl added a comment - Thanks Stack. Unless there are further comments I will commit this in a bit. (I might force TestFromClientSide to use the WritableRpcEngine - that we get a one test run with that rpc engine even when the secure-test profile is selected).
        Hide
        stack added a comment -

        lgtm

        Show
        stack added a comment - lgtm
        Hide
        Lars Hofhansl added a comment -

        Patch that does this.

        Show
        Lars Hofhansl added a comment - Patch that does this.
        Hide
        Lars Hofhansl added a comment -

        OK... Here's my "final" proposal.

        • Pull security code into the normal build
        • Run tests with normal WritableRpcEngine
        • All the security tests already configure themselves to run with the SecureRpcEngine
        • Add a TestFromClientSideWithSecureRpcEngine
        • Rename the security profile to security-test and keep the secure test resources
        • Keep the -security jenkins build and run it with the secure-test profile

        That way we get some test coverage for SecureRpcEngine with just the normal build, and full coverage for both WritableRpcEngine and SecureRpcEngine among the two jenkins runs. The default HBase build will ship with all the security code.

        Show
        Lars Hofhansl added a comment - OK... Here's my "final" proposal. Pull security code into the normal build Run tests with normal WritableRpcEngine All the security tests already configure themselves to run with the SecureRpcEngine Add a TestFromClientSideWithSecureRpcEngine Rename the security profile to security-test and keep the secure test resources Keep the -security jenkins build and run it with the secure-test profile That way we get some test coverage for SecureRpcEngine with just the normal build, and full coverage for both WritableRpcEngine and SecureRpcEngine among the two jenkins runs. The default HBase build will ship with all the security code.
        Hide
        Lars Hofhansl added a comment -

        It turns out that asynchbase does not work out of the box with the SecureRpcEngine. So we cannot just change it (and maybe that would be a little radical at this point in 0.94, anyway).

        So new plan: Default tests back to WritableRpcEngine, but all run all secure tests with the SecureRpcEngine and also run TestFromClientSide twice, once with each engine.

        Show
        Lars Hofhansl added a comment - It turns out that asynchbase does not work out of the box with the SecureRpcEngine. So we cannot just change it (and maybe that would be a little radical at this point in 0.94, anyway). So new plan: Default tests back to WritableRpcEngine, but all run all secure tests with the SecureRpcEngine and also run TestFromClientSide twice, once with each engine.
        Hide
        Lars Hofhansl added a comment -

        I think that'd be for another ticket with more discussion. Now sure - now thinking about it more - if folks would rely on anything specific from WritableRcpEngine.

        For this ticket I'd like to:

        1. Default to the secure build, but with WritableRpcEngine still by default
        2. Get rid of the secure jenkins build, run all tests including the secure ones as part of one run
        3. Allow the secure artifacts in maven

        The sticky part is the testing as mentioned above. If we do not want to run two full builds anymore we'll have to accept decreased coverage for one of the Rpc Egnines. Many tests do not use RPC anyway. So I think it might be best to switch to SecureRpcEngine for tests now and run TestFromClientSide and TestTableInputFormat (and others?) with both Rpc Engines.

        Show
        Lars Hofhansl added a comment - I think that'd be for another ticket with more discussion. Now sure - now thinking about it more - if folks would rely on anything specific from WritableRcpEngine. For this ticket I'd like to: Default to the secure build, but with WritableRpcEngine still by default Get rid of the secure jenkins build, run all tests including the secure ones as part of one run Allow the secure artifacts in maven The sticky part is the testing as mentioned above. If we do not want to run two full builds anymore we'll have to accept decreased coverage for one of the Rpc Egnines. Many tests do not use RPC anyway. So I think it might be best to switch to SecureRpcEngine for tests now and run TestFromClientSide and TestTableInputFormat (and others?) with both Rpc Engines.
        Hide
        stack added a comment -

        After reading above, if asynchbase works against secure rpc and no (or little perf degradation), just go secure all the time (as you suggest).

        Show
        stack added a comment - After reading above, if asynchbase works against secure rpc and no (or little perf degradation), just go secure all the time (as you suggest).
        Hide
        Lars Hofhansl added a comment -

        Not sure that would do. We'd then only run the secure tests with the SecureRpcEngine.
        The way I see it: The SecureRpcEngine is more general (by definition). It can run a secure and non-secure setting. It's the one we should switch to and the one that gets the testing.

        Maybe another option is to now have a non-secure profile, just for testing. It would exclude the the secure tests and the secure resources (with some maven trickery). That way we could still two jenkins build.

        Yet another option is run a select set of with the both RpcEngines. TestFromClientSide comes to mind, as well as some of the bulkload and M/R tests. So almost all tests would run with the SecureRpcEngine, and some would also run with the WritableRpcEngine.
        I think I'd favor this latter option.

        Show
        Lars Hofhansl added a comment - Not sure that would do. We'd then only run the secure tests with the SecureRpcEngine. The way I see it: The SecureRpcEngine is more general (by definition). It can run a secure and non-secure setting. It's the one we should switch to and the one that gets the testing. Maybe another option is to now have a non-secure profile, just for testing. It would exclude the the secure tests and the secure resources (with some maven trickery). That way we could still two jenkins build. Yet another option is run a select set of with the both RpcEngines. TestFromClientSide comes to mind, as well as some of the bulkload and M/R tests. So almost all tests would run with the SecureRpcEngine, and some would also run with the WritableRpcEngine. I think I'd favor this latter option.
        Hide
        Jesse Yates added a comment -

        You always keep the secure configs around and add them (switch out the
        standard one?) for something like a -Dsecure-tests flag

        A little more work, but means not changing every test

        Show
        Jesse Yates added a comment - You always keep the secure configs around and add them (switch out the standard one?) for something like a -Dsecure-tests flag A little more work, but means not changing every test
        Hide
        Lars Hofhansl added a comment -

        I guess now we cover WritableRcpEngine in the non-secure builds and SecureRpcEngine in the secure builds. Alas, no free lunch

        Show
        Lars Hofhansl added a comment - I guess now we cover WritableRcpEngine in the non-secure builds and SecureRpcEngine in the secure builds. Alas, no free lunch
        Hide
        Lars Hofhansl added a comment -

        I could default to the insecure engine (WritableRpcEngine) and tweak all the security tests to use SecureRpcEngine. You think that'd be better so we keep coverage for the WritableRpcEngine?

        Show
        Lars Hofhansl added a comment - I could default to the insecure engine (WritableRpcEngine) and tweak all the security tests to use SecureRpcEngine. You think that'd be better so we keep coverage for the WritableRpcEngine?
        Hide
        stack added a comment -

        Thats fine. Anyway to pass args and run tests on insecure? No biggie if not.

        Show
        stack added a comment - Thats fine. Anyway to pass args and run tests on insecure? No biggie if not.
        Hide
        Lars Hofhansl added a comment -

        How about this?

        • added comment as requested by Jesse
        • removes security test resources
        • adds the SecureRpcEngine to the testing hbase-site.xml
        Show
        Lars Hofhansl added a comment - How about this? added comment as requested by Jesse removes security test resources adds the SecureRpcEngine to the testing hbase-site.xml
        Hide
        stack added a comment -

        +1 Minimal disturbance.

        Show
        stack added a comment - +1 Minimal disturbance.
        Hide
        Gary Helmling added a comment -

        There is a lot of duplication between security/src/test/resources/hbase-site.xml and src/test/resources/hbase-site.xml, so my only concern would be changes made to one but not the other if they both stay in place. So +1 to removing the security/src/test/resources version and moving the "hbase.rpc.engine" setting to src/test/resources.

        I also think moving to SecureRpcEngine as the default would make sense, but also agree it merits a separate issue for discussion. There may be impact to other project like AsyncHBase?

        The rest of the patch looks good to me.

        Show
        Gary Helmling added a comment - There is a lot of duplication between security/src/test/resources/hbase-site.xml and src/test/resources/hbase-site.xml, so my only concern would be changes made to one but not the other if they both stay in place. So +1 to removing the security/src/test/resources version and moving the "hbase.rpc.engine" setting to src/test/resources. I also think moving to SecureRpcEngine as the default would make sense, but also agree it merits a separate issue for discussion. There may be impact to other project like AsyncHBase? The rest of the patch looks good to me.
        Hide
        Lars Hofhansl added a comment -

        It would be using the secure engine for all tests now (I think... Since we're overriding the rpc enging config with the hbase-site.xml in security/...), and they all still pass. So I could change src/test/resources/hbase-site.xml instead.

        I plan a followup jira to remove the WritableRpcEngine (or at least make the SecureRpcEngine the default).

        Do you think this is an issue?

        Show
        Lars Hofhansl added a comment - It would be using the secure engine for all tests now (I think... Since we're overriding the rpc enging config with the hbase-site.xml in security/...), and they all still pass. So I could change src/test/resources/hbase-site.xml instead. I plan a followup jira to remove the WritableRpcEngine (or at least make the SecureRpcEngine the default). Do you think this is an issue?
        Hide
        Gary Helmling added a comment -

        I don't think we actually need security/src/test/resources/hbase-site.xml without the security profile. The only thing it was doing was setting SecureRpcEngine to be used for tests. Which raises the question: should SecureRpcEngine or WritableRpcEngine be used for running the tests if security is integrated into the main build?

        Show
        Gary Helmling added a comment - I don't think we actually need security/src/test/resources/hbase-site.xml without the security profile. The only thing it was doing was setting SecureRpcEngine to be used for tests. Which raises the question: should SecureRpcEngine or WritableRpcEngine be used for running the tests if security is integrated into the main build?
        Hide
        Jesse Yates added a comment -

        Nm, I was wrong. thanks Lars Hofhansl] for calling me on it.

        +1 maybe add a comment, on commit, for why they are separate

        Show
        Jesse Yates added a comment - Nm, I was wrong. thanks Lars Hofhansl ] for calling me on it. +1 maybe add a comment, on commit, for why they are separate
        Show
        Jesse Yates added a comment - Wouldn't using this: http://maven.apache.org/plugins/maven-resources-plugin/examples/include-exclude.html be cleaner?
        Hide
        Lars Hofhansl added a comment -

        Here's the simple patch. (as described above I opted for the maven trickery rather than moving all the files into place under the src directory).

        stack, Andrew Purtell, Gary Helmling, Jesse Yates, please have a quick look. Thanks.

        Show
        Lars Hofhansl added a comment - Here's the simple patch. (as described above I opted for the maven trickery rather than moving all the files into place under the src directory). stack , Andrew Purtell , Gary Helmling , Jesse Yates , please have a quick look. Thanks.
        Hide
        Lars Hofhansl added a comment -

        Ran all tests:

        Results :
        Tests run: 728, Failures: 0, Errors: 0, Skipped: 0
        ...
        Results :
        Tests run: 1482, Failures: 0, Errors: 0, Skipped: 13
        
        [INFO] ------------------------------------------------------------------------
        [INFO] BUILD SUCCESS
        [INFO] -----------------------------------------------------
        

        Notice that we now run 1482 (instead of 1422). Those are all the security related ones.

        Show
        Lars Hofhansl added a comment - Ran all tests: Results : Tests run: 728, Failures: 0, Errors: 0, Skipped: 0 ... Results : Tests run: 1482, Failures: 0, Errors: 0, Skipped: 13 [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ----------------------------------------------------- Notice that we now run 1482 (instead of 1422). Those are all the security related ones.
        Hide
        Lars Hofhansl added a comment -

        Talked to Mr. Jesse Yates. I will just move the add-source portion from build under the secure profile into the global build. Running all tests now. So far so good.
        Will post patch once I confirmed the test suite passing.

        Show
        Lars Hofhansl added a comment - Talked to Mr. Jesse Yates . I will just move the add-source portion from build under the secure profile into the global build. Running all tests now. So far so good. Will post patch once I confirmed the test suite passing.
        Hide
        Lars Hofhansl added a comment -

        Now the question is. Do I just change pom.xml to always include the security files like this:

        +              <execution>
        +                <id>add-source</id>
        +                <goals>
        +                  <goal>add-source</goal>
        +                </goals>
        +                <configuration>
        +                  <sources>
        +                    <source>${project.basedir}/security/src/main/java</source>
        +                  </sources>
        +                </configuration>
        +              </execution>
        

        or do I move the security files into place under src? The former is less disruptive, the latter is cleaner.

        Show
        Lars Hofhansl added a comment - Now the question is. Do I just change pom.xml to always include the security files like this: + <execution> + <id>add-source</id> + <goals> + <goal>add-source</goal> + </goals> + <configuration> + <sources> + <source>${project.basedir}/security/src/main/java</source> + </sources> + </configuration> + </execution> or do I move the security files into place under src? The former is less disruptive, the latter is cleaner.

          People

          • Assignee:
            Lars Hofhansl
            Reporter:
            Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development