Skip to content

Fix user_data to be in line with cloud-init - #164

Merged
openshift-merge-bot[bot] merged 1 commit into
openstack-k8s-operators:mainfrom
cjeanner:cloud-init/ssh_authorized_keys-list
Aug 25, 2026
Merged

Fix user_data to be in line with cloud-init#164
openshift-merge-bot[bot] merged 1 commit into
openstack-k8s-operators:mainfrom
cjeanner:cloud-init/ssh_authorized_keys-list

Conversation

@cjeanner

@cjeanner cjeanner commented Apr 30, 2024

Copy link
Copy Markdown
Contributor

According to the official documentation[1], ssh_authorized_keys is a list, not a string.

The list key under chpasswd is also deprecated in cloud-init[2] in favor of the users list. Switch to the supported form to avoid deprecation warnings and future breakage.

[1] https://cloudinit.readthedocs.io/en/latest/reference/examples.html#configure-instance-s-ssh-keys
[2] https://github.com/canonical/cloud-init/blob/main/cloudinit/config/cc_set_passwords.py

jira: OSPRH-35290

@openshift-ci
openshift-ci Bot requested review from hjensas and stuggi April 30, 2024 13:55
@softwarefactory-project-zuul

Copy link
Copy Markdown

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/db56a202b8c9457ab62766e8d85486c8

openstack-baremetal-operator-content-provider FAILURE in 8m 24s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

@cjeanner
cjeanner force-pushed the cloud-init/ssh_authorized_keys-list branch 2 times, most recently from 6665e78 to 658efb5 Compare April 30, 2024 14:53
@softwarefactory-project-zuul

Copy link
Copy Markdown

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/fc4d0078022045e0ac719c9eb8a5f89c

openstack-baremetal-operator-content-provider FAILURE in 7m 55s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

@cjeanner
cjeanner force-pushed the cloud-init/ssh_authorized_keys-list branch 2 times, most recently from 432faf0 to 992e29d Compare April 30, 2024 15:19
Comment on lines +74 to +77
splitKeys := strings.Split(strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n"), "\n")
sshKeys := make([]string, len(splitKeys))
sshKeys = append(sshKeys, splitKeys...)
templateParameters["AuthorizedKeys"] = sshKeys

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, validation of some sort might be a good idea

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[1] openstack-k8s-operators/osp-director-operator#1043

@rabi rabi May 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
cjeanner force-pushed the cloud-init/ssh_authorized_keys-list branch from 992e29d to e085dba Compare May 3, 2024 06:14
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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to do this as in @abays' patch.. strings.Split() returns a slice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've shamelessly copy-pasted code then - with mention of the original PR in the comment. cc @abays - guess we should be good?

@cjeanner
cjeanner force-pushed the cloud-init/ssh_authorized_keys-list branch from e085dba to e50810e Compare May 6, 2024 07:50
@softwarefactory-project-zuul

Copy link
Copy Markdown

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/89876a28c20a431195c6e26e9f8de0f7

openstack-baremetal-operator-content-provider FAILURE in 7m 16s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

Comment thread pkg/openstackbaremetalset/baremetalhost.go Outdated
@cjeanner
cjeanner force-pushed the cloud-init/ssh_authorized_keys-list branch from e50810e to b4ddb1a Compare May 7, 2024 11:51
@openshift-merge-robot

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@rabi
rabi force-pushed the cloud-init/ssh_authorized_keys-list branch from b4ddb1a to 2b48fdb Compare August 20, 2026 07:56
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 605ac5dc-49ff-4d3d-b8fe-80539d942f93


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rabi

rabi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@cjeanner Hey! I had a customer asking for this, so rebased/updated your PR. Hope you don't mind 😄

@centosinfra-prod-github-app

Copy link
Copy Markdown

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/rdoproject.org/buildset/c77a4776d3004ecbba786c89064007ed

✔️ openstack-baremetal-operator-content-provider SUCCESS in 21m 26s
openstack-baremetal-operator-crc-podified-edpm-baremetal NODE_FAILURE Node(set) request 099-0000175914 failed in 0s
openstack-baremetal-operator-edpm-baremetal-minor-update NODE_FAILURE Node(set) request 099-0000175915 failed in 0s

@cjeanner

Copy link
Copy Markdown
Contributor Author

@rabi no problem - it completely went out of my mind, sorry, should have followed it better back then...

@rabi

rabi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

recheck

@centosinfra-prod-github-app

Copy link
Copy Markdown

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/rdoproject.org/buildset/6150239dc7ab45d2bc89c5923fd46a38

✔️ openstack-baremetal-operator-content-provider SUCCESS in 32m 56s
openstack-baremetal-operator-crc-podified-edpm-baremetal NODE_FAILURE Node(set) request 099-0000176428 failed in 0s
openstack-baremetal-operator-edpm-baremetal-minor-update NODE_FAILURE Node(set) request 099-0000176429 failed in 0s

@rabi

rabi commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

recheck

@rabi

rabi commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@abays Hey! Do you have any concerns with this updated one or we can go ahead?

According to the official documentation[1], `ssh_authorized_keys` is a
list, not a string.

The `list` key under `chpasswd` is also deprecated in cloud-init[2] in
favor of the `users` list. Switch to the supported form to avoid
deprecation warnings and future breakage.

[1] https://cloudinit.readthedocs.io/en/latest/reference/examples.html#configure-instance-s-ssh-keys
[2] https://github.com/canonical/cloud-init/blob/main/cloudinit/config/cc_set_passwords.py

jira: OSPRH-35290

Co-Authored-By: @pablintino <pabrodri@redhat.com>
Co-Authored-By: rabi <ramishra@redhat.com>
Signed-off-by: rabi <ramishra@redhat.com>
@rabi
rabi force-pushed the cloud-init/ssh_authorized_keys-list branch from 2b48fdb to 0da24a5 Compare August 25, 2026 04:25
@rabi rabi changed the title Ensure ssh_authorized_keys is a list in cloud-init Fix user_data to be in line with cloud-init Aug 25, 2026

@abays abays left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci

openshift-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abays, cjeanner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot
openshift-merge-bot Bot merged commit 282fe58 into openstack-k8s-operators:main Aug 25, 2026
8 checks passed
@rabi

rabi commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

/cherry-pick 18-stable

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rabi: new pull request created: #455

Details

In response to this:

/cherry-pick 18-stable

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants