Whirr
  1. Whirr
  2. WHIRR-164

Tests fail if there is no ~/.ssh/id_rsa keypair

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.3.0
    • Component/s: core
    • Labels:
      None

      Description

      The unit and integration tests depend on there being a ~/.ssh/id_rsa keypair.

      1. WHIRR-164.patch
        28 kB
        Andrei Savu
      2. WHIRR-164.patch
        25 kB
        Andrei Savu
      3. WHIRR-164.patch
        16 kB
        Andrei Savu
      4. WHIRR-164-after-161.patch
        26 kB
        Andrei Savu

        Activity

        Hide
        Tom White added a comment -

        We should add a ClusterSpec(boolean loadDefaults) for testing, so unit tests can pass in false when they don't care about having a keypair.

        For integration tests, it's probably OK to fall back to the default since the user can override by setting the config system property. However, we can remove the following from each service test, since it never happens:

        if (clusterSpec.getPrivateKey() == null) {
          Map<String, String> pair = KeyPair.generate();
          clusterSpec.setPublicKey(pair.get("public"));
          clusterSpec.setPrivateKey(pair.get("private"));
        }
        

        HadoopProxy has some conditional code to get the identity file. The first branch is never taken now (since WHIRR-160), since files are never stored as FilePayloads (we should fix ClusterSpec to remedy this). The second branch fails because the permissions on the generated key are too open. The following code needs adding:

        // Set to 600
        identity.setReadable(false, false);
        identity.setReadable(true, true);
        identity.setWritable(false, false);
        identity.setWritable(true, true);
        identity.setExecutable(false);
        
        Show
        Tom White added a comment - We should add a ClusterSpec(boolean loadDefaults) for testing, so unit tests can pass in false when they don't care about having a keypair. For integration tests, it's probably OK to fall back to the default since the user can override by setting the config system property. However, we can remove the following from each service test, since it never happens: if (clusterSpec.getPrivateKey() == null ) { Map< String , String > pair = KeyPair.generate(); clusterSpec.setPublicKey(pair.get( " public " )); clusterSpec.setPrivateKey(pair.get( " private " )); } HadoopProxy has some conditional code to get the identity file. The first branch is never taken now (since WHIRR-160 ), since files are never stored as FilePayloads (we should fix ClusterSpec to remedy this). The second branch fails because the permissions on the generated key are too open. The following code needs adding: // Set to 600 identity.setReadable( false , false ); identity.setReadable( true , true ); identity.setWritable( false , false ); identity.setWritable( true , true ); identity.setExecutable( false );
        Hide
        Andrei Savu added a comment -

        hint: useful method for generating temporary key pairs org.apache.whirr.ssh.KeyPair.generateTemporaryFiles added in WHIRR-160

        Show
        Andrei Savu added a comment - hint: useful method for generating temporary key pairs org.apache.whirr.ssh.KeyPair.generateTemporaryFiles added in WHIRR-160
        Hide
        Tom White added a comment -

        Thanks Andrei. Are you interested in fixing this one?

        Show
        Tom White added a comment - Thanks Andrei. Are you interested in fixing this one?
        Hide
        Andrei Savu added a comment -

        Yes. I've assigned this issue to myself.

        Show
        Andrei Savu added a comment - Yes. I've assigned this issue to myself.
        Hide
        Andrei Savu added a comment -

        Let me know what you think.

        Show
        Andrei Savu added a comment - Let me know what you think.
        Hide
        Andrei Savu added a comment -

        I have fixed some of the remaining issues. With this patch all the tests (unit + integration) are passing without needing a key in ~/.ssh/id_rsa

        Show
        Andrei Savu added a comment - I have fixed some of the remaining issues. With this patch all the tests (unit + integration) are passing without needing a key in ~/.ssh/id_rsa
        Hide
        Andrei Savu added a comment -

        I have attached a new patch generated starting from trunk + WHIRR-161. I suggest that you should start by first reviewing WHIRR-161.

        Show
        Andrei Savu added a comment - I have attached a new patch generated starting from trunk + WHIRR-161 . I suggest that you should start by first reviewing WHIRR-161 .
        Hide
        Tom White added a comment -

        This looks good. Just a couple of comments:

        • Mark the static factory methods in ClusterSpec as @VisibleForTesting, or move them to the test where they are used.
        • Can we call KeyPair.setTo600() from HadoopProxy rather than duplicating the code?

        Also, the patch does not apply cleanly to trunk any more.

        Show
        Tom White added a comment - This looks good. Just a couple of comments: Mark the static factory methods in ClusterSpec as @VisibleForTesting, or move them to the test where they are used. Can we call KeyPair.setTo600() from HadoopProxy rather than duplicating the code? Also, the patch does not apply cleanly to trunk any more.
        Hide
        Andrei Savu added a comment -

        Fixed and updated the patch.

        Show
        Andrei Savu added a comment - Fixed and updated the patch.
        Hide
        Tom White added a comment -

        +1

        I've just committed this. Thanks Andrei!

        Show
        Tom White added a comment - +1 I've just committed this. Thanks Andrei!
        Hide
        Andrei Savu added a comment -

        Thanks Tom for all the support!

        Show
        Andrei Savu added a comment - Thanks Tom for all the support!

          People

          • Assignee:
            Andrei Savu
            Reporter:
            Tom White
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development