provider/cloudstack: add SSH keypair resource

[benjvi]

This is a Cloudstack resource for SSH keypairs so we can register existing keys, get Cloudstack to generate keys for us and also so we can assign those keypairs to virtual machines. I added a couple of test cases and also added fields for keypairs into the VM tests

[svanharmelen]

Wow, that sounds like a nasty bug! Let me check this one with some CloudStack guys I work with...

[benjvi]

Thanks for that! I didn't have time to chase it down, but I guessed it would be to do with the fact I was testing with multiple keypairs registered at Cloudstack that in fact were pointing to the same underlying key values

[svanharmelen]

So we chased it and indeed it seems to be buggy and/or questionable at the very least.

Still I think it's such a corner case, that I would prefer to read back the associated value. At the same time we will work on fixing this (or making it more robust) in CS. For that it would be great if you can log an issue: http://issues.apache.org/jira

So what do you think about reading it back? Should be safe for normal operations right?

[benjvi]

OK cool I will raise that then.

About reading it back, I'm not really sure I'd say it is safe for the way I'm using this at the moment (non-converging keypair values causes a load of unneccessary reboots on every update), but I agree it seems like the best option in general.

So, I must test if I can avoid this reboot-on-update issue by de-duplicating my SSH keys. In the meantime I will make the change and update this PR

[svanharmelen]

Hmm... Thinking about it again, let's just keep it as is. It's a simple thing to uodate later when it's behaviour is changed in CS. So will pull this one is shortly (not at a desk rest of this day).

[svanharmelen]

@benjvi just had a quick look and overall it looks good. Will have a closer and do some testing tomorrow.

Thanks!

[svanharmelen]

One question about reading back the keypair value, other than that LGTM!

[svanharmelen]

Wouldn't it be better to join the changing of the keypair together with any possible changes of the service offering? As both these options need the VM to be stopped and started and if you change both it would currently do this twice for one apply...

So I guess something like this would be more effecient:

    // Check if the service offering is changed and if so, update the offering
    if d.HasChange("service_offering") || d.HasChange("keypair"){
        // Before we can actually make these changes, the virtual machine must be stopped
        _, err := cs.VirtualMachine.StopVirtualMachine(cs.VirtualMachine.NewStopVirtualMachineParams(d.Id()))
        if err != nil {
            return fmt.Errorf(
                "Error stopping instance %s before making changes: %s", name, err)
        }

        if d.HasChange("service_offering") {
                log.Printf("[DEBUG] Service offering changed for %s, starting update", name)

                // Retrieve the service_offering UUID
                serviceofferingid, e := retrieveUUID(cs, "service_offering", d.Get("service_offering").(string))
                if e != nil {
                    return e.Error()
                }

                // Create a new parameter struct
                p := cs.VirtualMachine.NewChangeServiceForVirtualMachineParams(d.Id(), serviceofferingid)


                // Change the service offering
                _, err = cs.VirtualMachine.ChangeServiceForVirtualMachine(p)
                if err != nil {
                    return fmt.Errorf(
                        "Error changing the service offering for instance %s: %s", name, err)
                }

                d.SetPartial("service_offering")
        }

        if d.HasChange("keypair") {
                log.Printf("[DEBUG] SSH keypair changed for %s, starting update", name)

                // Create a new parameter struct
                p := cs.SSH.NewResetSSHKeyForVirtualMachineParams(d.Id(), d.Get("keypair").(string))

                // Change the ssh keypair
                _, err = cs.SSH.ResetSSHKeyForVirtualMachine(p)
                if err != nil {
                    return fmt.Errorf(
                        "Error changing the SSH keypair for instance %s: %s", name, err)
                }

                d.SetPartial("keypair")
        }

        // Start the virtual machine again
        _, err = cs.VirtualMachine.StartVirtualMachine(cs.VirtualMachine.NewStartVirtualMachineParams(d.Id()))
        if err != nil {
            return fmt.Errorf(
                "Error starting instance %s after making changes: %s", name, err)
        }
    }

[svanharmelen]

While having a last (little closer) look I noticed a few small things, so commented on them and triggered Travis to check stuff again as the Makefile was updated to be more strict on go vet errors...

All small things, but would be good to have this fixed before we merge this one.

[benjvi]

Hi @svanharmelen Thanks for the detailed review! I can't dispute any of these points so i'll get started on the updates ;)

[benjvi]

Updated as per comments and rebased

[svanharmelen]

Great work @benjvi! LGTM!

[svanharmelen]

@benjvi so now the code is in... Are you also up for the docs that need to go with it? If not I will make sure they are updated... Let me know!

[benjvi]

Sure np I'll update that .. its just the "website/" stuff that needs to be added to right?

[svanharmelen]

Yep... You can just copy and paste an existing resource as a starting point.

Next to that you'll also need to add the page to the index page. Should be good otherwise...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.