Ensure ssh_authorized_keys is a list in cloud-init #164
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cjeanner The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/db56a202b8c9457ab62766e8d85486c8 ❌ openstack-baremetal-operator-content-provider FAILURE in 8m 24s |
cjeanner
force-pushed
the
cloud-init/ssh_authorized_keys-list
branch
2 times, most recently
from
6665e78
to
658efb5
Compare
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/fc4d0078022045e0ac719c9eb8a5f89c ❌ openstack-baremetal-operator-content-provider FAILURE in 7m 55s |
cjeanner
force-pushed
the
cloud-init/ssh_authorized_keys-list
branch
2 times, most recently
from
432faf0
to
992e29d
Compare
| splitKeys := strings.Split(strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n"), "\n") | ||
| sshKeys := make([]string, len(splitKeys)) | ||
| sshKeys = append(sshKeys, splitKeys...) | ||
| templateParameters["AuthorizedKeys"] = sshKeys |
There was a problem hiding this comment.
I think this can be cleaned up and simplified. Right now it duplicates the total number of keys by injecting a blank one for every actual key. The cloud-init ends up looking like this:
ssh_authorized_keys:
-
- ssh-rsa somekeyyyyyyyyyyyyyyyyyyyyyyyy
I think it might be as simple as:
templateParameters["AuthorizedKeys"] = strings.Split(strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n"), "\n")
There was a problem hiding this comment.
@abays What happens if the content the user provided have line breaks with more than one consecutive \n. I'd ensure that the keys passed to the template are not empty string to avoid such cases.
There was a problem hiding this comment.
Yeah, validation of some sort might be a good idea
There was a problem hiding this comment.
It depends on how the keys are in the secret data, whether the way it's expected by cloud-init (as a list) or how it's in authorized_keys file (multiple lines with newline separator).
There was a problem hiding this comment.
maybe we should be clear about the format then?
until we pushed a workaround (inject only one key), the framework was:
- loading the
~/.ssh/authorized_keys - generate the nodeset ConfigMap by injecting the file content as-is, in b64 format
We can push a proper list instead - so that the operator would just need to check if it's a string or a list; if string, it probably should enforce some format (either "only one key", or "use that separator only"?); if list, just nudge it in the cloud-init config file.
Any thoughts? Checking the input is more than probably mandatory at some point. And getting some proper testing around the generated cloud-init config file is also something we really should consider, in order to avoid future regressions/issues.
Any thoughts?
There was a problem hiding this comment.
This approach [1] works for OSPdO, where we assume that the authorized_keys in the Secret is a newline-separated string of keys. i.e. we expect the format you'd see in a ~/.ssh/authorized_keys.
There was a problem hiding this comment.
where we assume that the authorized_keys in the Secret is a newline-separated string of keys
Should we just assume or document that and throw a proper error if it's not a string separated by newline char.
There was a problem hiding this comment.
@abays just updated this current patch, adding the check for non-empty string. Up to you if you want to import the patch from OSPdO instead of getting this one. I can also import the comment as-is to match your own commit - just lemme know :).
cjeanner
force-pushed
the
cloud-init/ssh_authorized_keys-list
branch
from
992e29d
to
e085dba
Compare
| templateParameters["AuthorizedKeys"] = strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n") | ||
| // Prepare ssh_authorized_keys list for template | ||
| splitKeys := strings.Split(strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n"), "\n") | ||
| sshKeys := make([]string, len(splitKeys)) |
cjeanner
force-pushed
the
cloud-init/ssh_authorized_keys-list
branch
from
e085dba
to
e50810e
Compare
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/89876a28c20a431195c6e26e9f8de0f7 ❌ openstack-baremetal-operator-content-provider FAILURE in 7m 16s |
According to the official documentation[1], `ssh_authorized_keys` is a list, not a string. This patch should hopefully correct the issue we faced while trying to inject multiple authorized keys: the cloud-init configuration file was broken, preventing to apply any credential related data, leading to failures when RHOSO deploy actually started. [1] https://cloudinit.readthedocs.io/en/latest/reference/examples.html#configure-instance-s-ssh-keys Co-Authored-By: @pablintino <[email protected]>
cjeanner
force-pushed
the
cloud-init/ssh_authorized_keys-list
branch
from
e50810e
to
b4ddb1a
Compare
According to the official documentation[1],
ssh_authorized_keysis alist, not a string.
This patch should hopefully correct the issue we faced while trying to
inject multiple authorized keys: the cloud-init configuration file was
broken, preventing to apply any credential related data, leading to
failures when RHOSO deploy actually started.
[1] https://cloudinit.readthedocs.io/en/latest/reference/examples.html#configure-instance-s-ssh-keys
Co-Authored-By: @pablintino [email protected]