Uploaded image for project: 'Libcloud'
  1. Libcloud
  2. LIBCLOUD-388

deploy_node not supported in cloudstack driver

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.12.3
    • Fix Version/s: 0.14.0-beta3
    • Component/s: Compute
    • Labels:
      None

      Description

      deploy_node not available due to lack of auth

      1. libcloud388.patch
        13 kB
        sebastien goasguen

        Activity

        Hide
        kami Tomaz Muraus added a comment -

        Closing because this one has been merged.

        Show
        kami Tomaz Muraus added a comment - Closing because this one has been merged.
        Hide
        jc2k John Carr added a comment -

        I think so, thanks for working on this

        Show
        jc2k John Carr added a comment - I think so, thanks for working on this
        Hide
        sebgoa sebastien goasguen added a comment -

        Ok I think I get it. So it looks like you removed the ssh_key metadata.
        We are good then ?

        I will work on adding the find_or_import and supporting the two 'auth' mechanisms.

        thanks a lot for the review

        Show
        sebgoa sebastien goasguen added a comment - Ok I think I get it. So it looks like you removed the ssh_key metadata. We are good then ? I will work on adding the find_or_import and supporting the two 'auth' mechanisms. thanks a lot for the review
        Hide
        jc2k John Carr added a comment -

        sebastien goasguen,

        In trunk deploy_node is totally decoupled from self.features['create_node']. That metadata is only used right now to indicate what kinds of values you can pass to the auth argument. Note that this doesn't mean you have to pass them to the auth argument, auth can be None. So you are correct - it is possible to provide NO authentication related information to create_node, but deploy_node would still work. This is by design. Some services don't actually let you specify a password, ssh key or keyring name at all, and deploy_node will still work for them. So it is possible for deploy_node to work even if the features dict is entirely empty!

        I do agree that for services that have a keyring and people know what their key is called already then it makes sense to continue to support the ex_keyname API. So definitely don't remove the ex_keyname API. But there are hosting services that take the pubkey and don't have a keyring. The find_or_import approach means we can have a single API that works for services with or without a keyring, as well as the ex_keyname API for the services with a keyring.

        Show
        jc2k John Carr added a comment - sebastien goasguen , In trunk deploy_node is totally decoupled from self.features ['create_node'] . That metadata is only used right now to indicate what kinds of values you can pass to the auth argument. Note that this doesn't mean you have to pass them to the auth argument, auth can be None. So you are correct - it is possible to provide NO authentication related information to create_node, but deploy_node would still work. This is by design. Some services don't actually let you specify a password, ssh key or keyring name at all, and deploy_node will still work for them. So it is possible for deploy_node to work even if the features dict is entirely empty! I do agree that for services that have a keyring and people know what their key is called already then it makes sense to continue to support the ex_keyname API. So definitely don't remove the ex_keyname API. But there are hosting services that take the pubkey and don't have a keyring. The find_or_import approach means we can have a single API that works for services with or without a keyring, as well as the ex_keyname API for the services with a keyring.
        Hide
        sebgoa sebastien goasguen added a comment -

        @tomaz:
        Ok I did not have 3.2 and 3.3 installed, I did install them now and run tox and I see the errors.
        I was only running the 2.7 tests manually.
        I will make sure to run tox next time, sorry about that.
        About the docs that's my bad, I wanted to fix the security group weird string passing and did not properly re-read everythign for compliance. I would have fixed it in a next commit.

        @john:
        thanks for the review. I will look more closely into it. This is merely a first step to get deploy_node to work. Right now It only works by passing a ssh_key parameter and expecting a ex_keyname in the kwargs of create_node. I did not implement tested the auth object. base.py is actually a bit confusing there because ssh_key in the features is used to say that you need a auth as NodeauthSSHKey, yet you can pass ssh_key and it will go forward with the deploy. If deploy only works with ssh_key can we set features to an empty dict ?

        Indeed we also have ex_import_keypair calls in the driver, I will add the import_by_key_material, but I don't think it's very intuitive. A User that has already import or generated a keypair will know it by name and can specify by name instead of passing the actual pubkey, but that's just my opinion

        Thanks guys

        Show
        sebgoa sebastien goasguen added a comment - @tomaz: Ok I did not have 3.2 and 3.3 installed, I did install them now and run tox and I see the errors. I was only running the 2.7 tests manually. I will make sure to run tox next time, sorry about that. About the docs that's my bad, I wanted to fix the security group weird string passing and did not properly re-read everythign for compliance. I would have fixed it in a next commit. @john: thanks for the review. I will look more closely into it. This is merely a first step to get deploy_node to work. Right now It only works by passing a ssh_key parameter and expecting a ex_keyname in the kwargs of create_node. I did not implement tested the auth object. base.py is actually a bit confusing there because ssh_key in the features is used to say that you need a auth as NodeauthSSHKey, yet you can pass ssh_key and it will go forward with the deploy. If deploy only works with ssh_key can we set features to an empty dict ? Indeed we also have ex_import_keypair calls in the driver, I will add the import_by_key_material, but I don't think it's very intuitive. A User that has already import or generated a keypair will know it by name and can specify by name instead of passing the actual pubkey, but that's just my opinion Thanks guys
        Hide
        sebgoa sebastien goasguen added a comment -

        Hum, this is strange, I did run the tests...
        let me check

        Show
        sebgoa sebastien goasguen added a comment - Hum, this is strange, I did run the tests... let me check
        Hide
        jc2k John Carr added a comment -

        Hello,

        Sorry for not commenting on this sooner but i was only receiving notifications about GitHub pull requests - that won't happen again!

        The behaviour of create_node/deploy_node has changed in trunk and the "features = {}" line is now used to indicate one of three things:

        • If it contains "password", it should take a NodeAuthPassword via the "auth" argument and pass that password to the Node that is created
        • If it contains "ssh_key" it should take a NodeAuthSSHKey object via the "auth" argument. That will contain a public key that should be used in teh authorized_keys file in the created node
        • If it contains "generates_password" then the VM creates a password and returns it via an extra parameter.

        I noticed your patch adds the "ssh_key" feature to CloudStack, but it does not take a NodeAuthSSHKey parameter. That breaks the API and tools that rely on the metadata will wrongly try and pass a NodeAuthSSHKey to your driver. So I am going to revert that part of your patch.

        deploy_node does not rely on the features metadata any more so this should not impact your deploy_node fixes, but it would be good if you could do a follow up pull request that properly supports NodeAuthSSHKey. It looks that CloudStack is similar to EC2 here in that there is a keyring service of some kind. If you look at the EC2 driver you will see it has a method called ex_find_or_import_keypair_by_key_material which imports a key into the keyring automatically if it doesnt exist already and gives you the key name.

        Show
        jc2k John Carr added a comment - Hello, Sorry for not commenting on this sooner but i was only receiving notifications about GitHub pull requests - that won't happen again! The behaviour of create_node/deploy_node has changed in trunk and the "features = {}" line is now used to indicate one of three things: If it contains "password", it should take a NodeAuthPassword via the "auth" argument and pass that password to the Node that is created If it contains "ssh_key" it should take a NodeAuthSSHKey object via the "auth" argument. That will contain a public key that should be used in teh authorized_keys file in the created node If it contains "generates_password" then the VM creates a password and returns it via an extra parameter. I noticed your patch adds the "ssh_key" feature to CloudStack, but it does not take a NodeAuthSSHKey parameter. That breaks the API and tools that rely on the metadata will wrongly try and pass a NodeAuthSSHKey to your driver. So I am going to revert that part of your patch. deploy_node does not rely on the features metadata any more so this should not impact your deploy_node fixes, but it would be good if you could do a follow up pull request that properly supports NodeAuthSSHKey. It looks that CloudStack is similar to EC2 here in that there is a keyring service of some kind. If you look at the EC2 driver you will see it has a method called ex_find_or_import_keypair_by_key_material which imports a key into the keyring automatically if it doesnt exist already and gives you the key name.
        Hide
        kami Tomaz Muraus added a comment - - edited

        Just FYI, tests failed under 3.2 and 3.3.

        ======================================================================
        ERROR: test_list_nodes_response (libcloud.test.compute.test_cloudstack.CloudStackNodeDriverTest)
        ----------------------------------------------------------------------
        Traceback (most recent call last):
        File "/w/lc/libcloud/libcloud/test/compute/_init_.py", line 26, in test_list_nodes_response
        nodes = self.driver.list_nodes()
        File "/w/lc/libcloud/libcloud/compute/drivers/cloudstack.py", line 259, in list_nodes
        public_ips.extend([ip for ip in private_ips
        AttributeError: 'dict_keys' object has no attribute 'extend'

        ...

        This time I've fixed it (https://git-wip-us.apache.org/repos/asf?p=libcloud.git;a=commitdiff;h=260e305d2f7516a65423f31b3795d777c3af87ba), but next time please run "tox" before submitting a final patch and make sure all the tests pass.

        There were also a bunch of issues with the documentation which I have also fixed (https://git-wip-us.apache.org/repos/asf?p=libcloud.git;a=commitdiff;h=b6fa496d7010938519e6fdecdbbf60674e7d9321 ):

        • some text was out of date
        • Issues with example code (undefined IMAGE_ID, extra_args was used in a wrong way, unused TEMPLATE_ID, ..).
        Show
        kami Tomaz Muraus added a comment - - edited Just FYI, tests failed under 3.2 and 3.3. ====================================================================== ERROR: test_list_nodes_response (libcloud.test.compute.test_cloudstack.CloudStackNodeDriverTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/w/lc/libcloud/libcloud/test/compute/_ init _.py", line 26, in test_list_nodes_response nodes = self.driver.list_nodes() File "/w/lc/libcloud/libcloud/compute/drivers/cloudstack.py", line 259, in list_nodes public_ips.extend([ip for ip in private_ips AttributeError: 'dict_keys' object has no attribute 'extend' ... This time I've fixed it ( https://git-wip-us.apache.org/repos/asf?p=libcloud.git;a=commitdiff;h=260e305d2f7516a65423f31b3795d777c3af87ba ), but next time please run "tox" before submitting a final patch and make sure all the tests pass. There were also a bunch of issues with the documentation which I have also fixed ( https://git-wip-us.apache.org/repos/asf?p=libcloud.git;a=commitdiff;h=b6fa496d7010938519e6fdecdbbf60674e7d9321 ): some text was out of date Issues with example code (undefined IMAGE_ID, extra_args was used in a wrong way, unused TEMPLATE_ID, ..).
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 54b778f264a1f85bb9e82763e2fea4f3b961bfab in branch refs/heads/trunk from sebastien goasguen
        [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=54b778f ]

        LIBCLOUD-388: Added deploy_node for basic zone, improved consistency of params in create_node

        Signed-off-by: Tomaz Muraus <tomaz@apache.org>

        Show
        jira-bot ASF subversion and git services added a comment - Commit 54b778f264a1f85bb9e82763e2fea4f3b961bfab in branch refs/heads/trunk from sebastien goasguen [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=54b778f ] LIBCLOUD-388 : Added deploy_node for basic zone, improved consistency of params in create_node Signed-off-by: Tomaz Muraus <tomaz@apache.org>
        Hide
        sebgoa sebastien goasguen added a comment -

        ok I think I found the test you are thinking about test_create_node_with_ex_keyname...etc
        But I am not seeing any test or try/except on the import of base64 or b, I looked at openstack and ec2 driver.

        Sorry about those though, it's true that I did not test the userdata stuff, hence the missing imports.

        Show
        sebgoa sebastien goasguen added a comment - ok I think I found the test you are thinking about test_create_node_with_ex_keyname...etc But I am not seeing any test or try/except on the import of base64 or b, I looked at openstack and ec2 driver. Sorry about those though, it's true that I did not test the userdata stuff, hence the missing imports.
        Hide
        sebgoa sebastien goasguen added a comment -

        This is a squash commit that address 1-5

        About 6, I am not sure what you mean. How can we test deploy_node ?
        I made sure that the existing tests passed. I did look at the other drivers but did not see a test for the import...

        Getting this to you before heading to bed

        Show
        sebgoa sebastien goasguen added a comment - This is a squash commit that address 1-5 About 6, I am not sure what you mean. How can we test deploy_node ? I made sure that the existing tests passed. I did look at the other drivers but did not see a test for the import... Getting this to you before heading to bed
        Hide
        kami Tomaz Muraus added a comment -

        Thanks.

        There are a couple of issues in this patch which need to be addressed before it can be merged:

        1. Missing "base64" and "b" import
        2. pep8 / style issues
        3. [ip for ip in private_ips if is_private_subnet(ip) == False] - should use "if not is_private_subnet(..) here"
        4. I assume "if keypair in vm.keys():" should be "if 'keypair' in vm.keys()", because keypair is initialize to None. Same goes for the line bellow.
        5. Need to add a note to documentation example which says that this functionality is only available in trunk
        6. Also need to add some test cases which exercises the new functionality and catch the previously mentioned missing import issue.

        Show
        kami Tomaz Muraus added a comment - Thanks. There are a couple of issues in this patch which need to be addressed before it can be merged: 1. Missing "base64" and "b" import 2. pep8 / style issues 3. [ip for ip in private_ips if is_private_subnet(ip) == False] - should use "if not is_private_subnet(..) here" 4. I assume "if keypair in vm.keys():" should be "if 'keypair' in vm.keys()", because keypair is initialize to None. Same goes for the line bellow. 5. Need to add a note to documentation example which says that this functionality is only available in trunk 6. Also need to add some test cases which exercises the new functionality and catch the previously mentioned missing import issue.
        Hide
        sebgoa sebastien goasguen added a comment -

        This provides deploy_node for basic zones. Advanced zones will require more work.
        I also fixed some consistency issues in create_node and it looks a lot more like the openstack create_node.
        Fixed tests and documentation

        Show
        sebgoa sebastien goasguen added a comment - This provides deploy_node for basic zones. Advanced zones will require more work. I also fixed some consistency issues in create_node and it looks a lot more like the openstack create_node. Fixed tests and documentation

          People

          • Assignee:
            Unassigned
            Reporter:
            sebgoa sebastien goasguen
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development