provider/cloudstack: Improve ssh keypair handling

[serbaut]

• adds support for projects
• adds support for public_key strings as well as filenames

[serbaut]

label: s/openstack/cloudstack/

[svanharmelen]

@serbaut thanks for the PR!

I just had a look at it, but I fail to understand why you added the public_key_openssh parameter as functionally the existing public_key parameter does exactly the same thing. Could you explain what you are trying to accomplish?

As for the project ID stuff, I like the approach but think we should make it more generic by placing the setProjectID func and the SSHParams type in the resources.go file and renaming SSHParams to projectIDSetter as that feels like it's a much more generic interface name.

After we done that, we can use the setProjectID func in all locations that have the option to set a project ID, like in the cloudstack_network resource.

[serbaut]

I added public_key_openssh since there is tls_private_key.public_key_openssh (public_key looks like a resource and not a file name) and having providers read files directly is to be avoided in terraform since the file() interpolation fills that function. Also it makes the following possible

resource "tls_private_key" "test" {
    algorithm = "RSA"
}

resource "cloudstack_ssh_keypair" "test" {
    name = "mykey"
    public_key_openssh = "${tls_private_key.test.public_key_openssh}"
    project = "myproj"
}

I will regorg the code as per your suggestion and make a new pr.

[svanharmelen]

Ok, that's clear 😀 In that case I would suggest updating the logic behind the public_key parameter, instead of adding a new parameter. As in the end they both make the exact same API call to CloudStack, so the only difference is how the param can be used.

There is a way to do this which enables the param to take both content or a file path, so it will not break backwards compatibility and will allow both use cases to work with that single parameter. Have a look at this PR which implements this as well (the rename of the param is not needed in this case as we don't have path in the param name). The magic is added by using the pathorcontents package found here.

Let me know if you need any help with it, but I guess it's pretty straight forward 😉

[serbaut]

Having an interface that can take a string or a filename doesn't sound like a good practice. What if the file doesn't exists? You would get a strange error message instead of foo.key: no such file or directory. What if the string is "E:foo", you might get an I/O error on windows?

Why not make it compatible with tls_private_key and call it public_key_openssh and let users pass a string or a filename (using file()). A cleaner design in my opinion.

[svanharmelen]

This approach (having a single param that accepts both a string and a filename) is done in more places within TF (hence there is a dedicated helper package made for it) and gives the user maximum flexibility about how they want to build their TF config.

Especially here were you only want to extend the way how you can input a value and not the logic behind the scenes (no other API calls or special treatment is needed in the resource provider code) or the purpose of the param (it still takes in a key), it makes a lot of sense to me to just add that option to the existing param by extending the inputs it allows.

Next to that this option will be fully compatible with tls_private_key and/or file(), so I it will definitely solve your use case(s) as well.

So still I'm not in favour of adding a separate param and the additional if else code that needs to go into the resource provider, only to have two dedicated way's of assigning a value to a param which is the end both need to execute the same code...

Last but not least using openssh in the param name seems a little strange to me as OpenSSH is just an implementation of SSH and CloudStack is not restricted to only work with OpenSSH...

[serbaut]

Cloudstack only accepts public keys in the OpenSSH format afaict.

I fail to find any module that mentions a "string or filename" duality in the docs?

[svanharmelen]

LGTM! Thanks again for the PR!

[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.