Details

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

      Description

      This would reduce the number of support issues that trip up users.

      We already check that the private and public keys both exist (but I don't think we have a unit test for that).

      In addition, we should

      1. WHIRR-160.patch
        21 kB
        Andrei Savu
      2. WHIRR-160.patch
        18 kB
        Andrei Savu
      3. WHIRR-160.patch
        14 kB
        Andrei Savu
      4. ASF.LICENSE.NOT.GRANTED--WHIRR-160.patch
        9 kB
        Andrei Savu

        Issue Links

          Activity

          Hide
          Andrei Savu added a comment -

          I've attached a patch that checks the private key for validity using JSch. I'm still working on checking that both keys belong to the same pair.

          Show
          Andrei Savu added a comment - I've attached a patch that checks the private key for validity using JSch . I'm still working on checking that both keys belong to the same pair.
          Hide
          Tom White added a comment -

          This looks good so far. A few comments:

          • You can use KeyPair in the org.apache.whirr.ssh test package to generate keys for DestroyClusterCommandTest and LaunchClusterCommandTest.
          • It would be good to have some tests for file existence, i.e. fail early if either key doesn't exist.
          • I think we default the private key to $ {sys:user.home}

            /.ssh/id_rsa and the public key to $

            {sys:user.home}

            /.ssh/id_rsa.pub (see core/src/main/resources/whirr-default.properties). I think the public key should actually default to the private key + ".pub". Can we make this change (with tests) here too?

          BTW you could do the check that both keys belong to the same pair in another JIRA if it is more involved, since the rest is useful enough to go in by itself I think.

          Show
          Tom White added a comment - This looks good so far. A few comments: You can use KeyPair in the org.apache.whirr.ssh test package to generate keys for DestroyClusterCommandTest and LaunchClusterCommandTest. It would be good to have some tests for file existence, i.e. fail early if either key doesn't exist. I think we default the private key to $ {sys:user.home} /.ssh/id_rsa and the public key to $ {sys:user.home} /.ssh/id_rsa.pub (see core/src/main/resources/whirr-default.properties). I think the public key should actually default to the private key + ".pub". Can we make this change (with tests) here too? BTW you could do the check that both keys belong to the same pair in another JIRA if it is more involved, since the rest is useful enough to go in by itself I think.
          Hide
          Andrei Savu added a comment -

          Tom, I will make the change as you suggested and open another JIRA for checking that both keys belong to the same pair. Should I define multiple types of exceptions derived from ConfigurationException class, one for each error case?

          Show
          Andrei Savu added a comment - Tom, I will make the change as you suggested and open another JIRA for checking that both keys belong to the same pair. Should I define multiple types of exceptions derived from ConfigurationException class, one for each error case?
          Hide
          Tom White added a comment -

          > Should I define multiple types of exceptions derived from ConfigurationException class, one for each error case?

          I wouldn't bother since I'm not sure whether clients can handle different cases differently. We can add that specialization later if it's needed.

          Show
          Tom White added a comment - > Should I define multiple types of exceptions derived from ConfigurationException class, one for each error case? I wouldn't bother since I'm not sure whether clients can handle different cases differently. We can add that specialization later if it's needed.
          Hide
          Andrei Savu added a comment -

          This patch should be complete but unfortunately it does not compile due to a maven related issue. It seems like the org.apache.whirr.ssh.KeyPair class from whirr-core is not available in whirr-cli. Need some help from you on this.

          Show
          Andrei Savu added a comment - This patch should be complete but unfortunately it does not compile due to a maven related issue. It seems like the org.apache.whirr.ssh.KeyPair class from whirr-core is not available in whirr-cli. Need some help from you on this.
          Hide
          Andrei Savu added a comment -

          I have moved the file KeyPair.java from core/test/java/org/apache/whirr/ssh/ to core/main/.... All the tests are now green. I will open another JIRA for checking that both keys are part of the same pair.

          Show
          Andrei Savu added a comment - I have moved the file KeyPair.java from core/test/java/org/apache/whirr/ssh/ to core/main/... . All the tests are now green. I will open another JIRA for checking that both keys are part of the same pair.
          Hide
          Tom White added a comment -

          I've just committed this. Thanks Andrei!

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

          Great! Tom, thanks for reviewing.

          Show
          Andrei Savu added a comment - Great! Tom, thanks for reviewing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development