provider/clc: Fix clc_server password argument as Optional.

[fatmcgav]

The clc_server resource doc's list password as optional [1], however the resource provider has it marked as a Required [2].

This PR switches the password field to an Optional field.

I did look at adding additional tests for this, however the password value isn't returned by the resource, therefore I couldn't see an easy way to validate. Happy to revisit if I've missed something.

cc @ack

[1] https://github.com/hashicorp/terraform/blob/master/website/source/docs/providers/clc/r/server.html.markdown
[2] https://github.com/hashicorp/terraform/blob/master/builtin/providers/clc/resource_clc_server.go#L47-L49

[stack72]

Why is the Default "" removed?

[fatmcgav]

See comment on line 50...

[tonycapone]

I think you should add Computed: true as well, since it can be generated by the platform. Similar to what is done for "private_ip_address."

[tonycapone]

If you let CLC provide the password, you will need to retrieve the credentials from CLC to populate the tfstate file. Otherwise the password field will be blanked out in the tfstate. This is requires an additional call to server/credentials.

[ack]

I remember looking into this when you first filed the ticket. I've got no gripes with making it optional, just would like the case where passwords are provided to be handled properly.

for clarification, are you concerned about leaving passwords in local state files or do you not want to generate and use passwords explicitly?

• when optional (not required), there's no way to get at the generated password. that might be a bug or a feature depending on your bias. consider for instance logging in on a provisioner. special case handling will be required to deal w/ the "" password in the local state file.
• when computed, you can't provide one (should you want to). again, there's no way to access the generated password.

more testing/tests are needed here, but conceptually this should be fine.

[tonycapone]

@ack Why can't you make an additional call to server/credentials to get the generated password? The clc sdk already has hooks for that.

[ack]

we could easily pull credentials via the api endpoint. do we store it in local state though?
if there's a concern about having the passwords available in cleartext in a file, then you've got the same issue.

if we decide not to store the password with the state files, then when we want to provision on the resource, we'll need to inject the credentials and special case the attribute such that it isn't stored into the local state files.

a further issue with making password computed is that it precludes being able to specify one. eg. users who want to explicitly manage passwords.

don't think there's a happy solution for all use cases since we are, after all, dealing with passwords here.

I am leaning towards making it computed though and recommending/making it easy to use private keys for login access (for linux hosts).

[fatmcgav]

@ack / @tonycapone Cheers both for the responses...

I guess it does get more complicated if want to support both scenarios...
From my POV, I'd prefer not to manage root passwords in tf files, and also not have to worry about generating secure random passwords for hosts when CLC already does that for me...

Though I can see a use case where it would be beneficial to retrieve the computed password for display purposes... However then you're back into root password in tfstate conundrum... :)

What's the general preference for Terraform? Store root password or not?

[fatmcgav]

@ack Agree that storing passwords in plain text is probably not the done thing nowadays...
Whether that's in tf config files that are creating the resources, or tfstate files as a result of the resource creation...

The idea of using SSH keys seems to be a common pattern amongst providers - AWS/Google/Openstack... Does the CLC SDK provide a sensible means to manage that though?

[tonycapone]

@ack Well I think we're talking about two distinct things here. One is allowing CLC to compute the password. The other issue is whether to store the password (user provided or computed) in the tfstate file. The pull request is really about the former, as far as I can tell.

I don't see any reason why the password field can't be optional and computed. There are several fields where you're doing this already, such as IP address. If one is provided, pass it along in the create server request, if not, let platform generate it.

As far as the password in tfstate goes, well, that might be a bad idea, but it's already there. I would be careful about removing it now, since it might break an existing automation folks may have that is relying on it being there, for example, if they're parsing it into an ansible inventory.

@fatmcgav AFAIK there's no way to inject keys through the platform, so that has to happen after the VM is stood up and SSH is available. That means at a minimum, that you will have to use the root password once to get on the box and lay down the public key.

[fatmcgav]

@tonycapone Ok, so sounds like we need to retain the password client side...

So I guess it comes down to do we want to store it in the tf files, or the tfstate files?

[ack]

just tested this: 2976536
I think it's better than what we currently have, although yes, it's not backwards compatible.

new behavior is computed + optional:

• when absent, a password is generated by the platform. .tfstate has no password in it.
• when absent then provided in config, updates password on server, stores to tfstate.
• when present, used and update-able from source config, stored to tfstate.

the only remaining piece is having the generated password available as an attribute from, say, a provisioner run and/or available to ansible inventory. in the event that either of these use cases is needed, I think it's acceptable to mandate that the password be manually provided and (as a consequence) stored to the state file.

thoughts?

[fatmcgav]

@ack looks sensible...

I have got a use case where I need the password for provisioner [1].
So having it available in tfstate is probably a requirement for that one.

[1] https://github.com/fatmcgav/terraform-provisioner-clc_exec

[stack72]

If this is not backwards compatible shouldn't we get a migration in place for older TF state?

[tonycapone]

@ack, Looks good, what about also querying for creds in resourceCLCServerRead? Then it would persist to the tfstate file. Here's my version of the fix and it works pretty well: tonycapone@5376ba3

[ack]

on second thought, I'm pretty sure it is backwards compatible. formerly it was a required and now it's optional. all existing configurations have the password field. and any new configs can either specify or omit it.

however, what might break is references to a password in the case where platform generates one. eg.

resource "clc_server" "foo" {
  // password not supplied
  ... 
  provisioner "remote-exec" {
    connection {
      ...
      password = "${clc_server.foo.password}"
    }
  }
}

I think it's reasonable to let this be an unhandled edge case for now. for this case, it seems better to have the user handle passwords explicitly.

unless there are objections I'm gonna close this PR and proceed to merge 2976536

[tonycapone]

@ack I guess I'm missing why you wouldn't just always fetch the credentials. What happens if someone changes the password manually through control portal? As it stands now, the module has no way of detecting that. Unless I'm missing something?

[ack]

so after some discussion, turns out there are 3 cases / scenarios here:

1. "I never want to think/know/generate passwords. I don't want them on disk. I don't want them with green ham and eggs"
2. "I want clc to generate a password for me and allow me to extract/reference/use it"
3. "I want to generate and set passwords instead of having clc do it".

this commit: e9b8687 facilitates most of 1 and all of 2 and 3.

unfortunately, not storing passwords in tfstate excludes the possibility of 2 and/or 3, so we're throwing that requirement out.

the new behavior is optional password (will be generated by the platform) and the generated value will be accessible on the server resource under the password attribute for wide-open usage (by output, provisioners, ansible inventory, etc)

[tonycapone]

looks good. thanks @ack

[fatmcgav]

Cheers both, looks good to me too...

[ack]

changes coalesced into: 3a8cc2b

closing this 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.