add additional checks for instance ssh key conditional
kral2 opened this issue · 4 comments
From discussion Originally posted by @calorbeer in #67 (comment)
There is an issue with ssh_public_key
variables having default value set to null
.
If var.ssh_public_key
value is set by the user, ssh_authorized_keys
argument is set correctly however if it's null
the condition is also evaluated to true
and as a result it is set to null
. The file statements are never reached. To avoid this the variables either need to default to "" or the conditions have to test for != null and "".
To do:
- test should be done uppon
null
and we also initialize relavant variables tonull
- rename
ssh_public_key_path
tossh_public_keys_file
(plural form forkeys
and mentionfile
explicitely)
@calorbeer Would you be volunteer to fix that? :-)
For the implementation details, I suggest that the test should be done uppon null
and we also initialize relavant variables to null
. ""
is an empty string: that would make the connection impossible, but it becomes a deliberate choice from the module user.
Let's keep the conditional as simple as possible.
Yes to check for null should be sufficient.
Based on a review comment that appeared in PR #71, we should also adapt the variable names to be semantically correct: using plural form.
rename ssh_public_key_path
to ssh_public_keys_path
For ssh_public_key
, we need to figure out if there is possibility to pass multiple ssh keys as string or array of strings.
Sometimes, less is more :-)
All the use cases can be handled by only one variable: ssh_public_keys
(Changing to plural form regarding fc66206).
This greatly simplify the code in the module, and will be probably less confusing for the user.
resource "oci_core_compute" "my_instance" {
...
metadata = {
ssh_authorized_keys = var.ssh_public_keys != null ? var.ssh_public_keys : file(var.ssh_authorized_keys)
user_data = var.user_data
}
...
}
The conditional is here only for backward compatibility with var.ssh_authorized_keys
. As soon as we move to the next major release, we can drop the conditional all together and adopt this simpler form:
resource "oci_core_compute" "my_instance" {
...
metadata = {
ssh_authorized_keys = var.ssh_public_keys
user_data = var.user_data
}
...
}
The module user will assign value to this argument like this:
module "my_instance" {
...
ssh_public_keys = var.my_public_ssh_key
...
}
To provide multiple keys at once, just use Heredoc strings:
module "my_instance" {
...
ssh_public_keys = <<EOF
<public ssh key 1>
<public ssh key 2>
<public ssh key n>
EOF
...
}
If the module user prefer to provide keys from a file, that's also possible:
module "my_instance" {
...
ssh_public_keys = file("/path/to/file")
...
}