openshift / appliance

OpenShift-based Appliance Builder
Apache License 2.0
21 stars 24 forks source link

MGMT-17097: Fix SSHAuthorizedKeys in installation and post-installation phases #207

Closed mlorenzofr closed 3 months ago

mlorenzofr commented 3 months ago

This PR fixes the creation of SSH authorized keys files during the installation and post-installation phases. Currently, when the appliance installation completes, it is not possible log into the nodes using SSH. After installation, the MachineConfig files 99-master-set-core-pass and 99-worker-set-core-pass overwrite the user "core" settings and remove authorized_keys. With this PR, the sshKeys value set in the appliance-config.yaml file will also be added to MachineConfig objects.

openshift-ci-robot commented 3 months ago

@mlorenzofr: This pull request references MGMT-17097 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/openshift/appliance/pull/207): >This PR fixes the creation of SSH authorized keys files during the installation and post-installation phases. >Currently, when the appliance installation completes, it is not possible log into the nodes using SSH. >After installation, the _MachineConfig_ files `99-master-set-core-pass` and `99-worker-set-core-pass` overwrite the user "_core_" settings and remove `authorized_keys`. >With this PR, the `sshKeys` value set in the `appliance-config.yaml` file will also be added to MachineConfig objects. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fappliance). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mlorenzofr Once this PR has been reviewed and has the lgtm label, please assign jhernand for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openshift/appliance/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
danielerez commented 3 months ago

So the current behaviour is actually by design. I.e. post-installation, only the ssh key specified by the user should be available - it can be specified in the install-config.yaml. The sshKey specified in appliance-config.yaml is intended only for the factory (the debug the machine during bootstrap). After installation, the ssh key provided in the install-config.yaml (within the config-image) should be available for the user to log into the machine. If it doesn't work, then it's indeed a bug:)

openshift-ci[bot] commented 3 months ago

@mlorenzofr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-sno-dualstack-dhcp ab78a9266e7bdff84310b9736b364fc82e4ca131 link false /test e2e-sno-dualstack-dhcp

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
jhernand commented 3 months ago

Looks good to me. I think that @danielerez will want to take a look.

mlorenzofr commented 3 months ago

Considering @danielerez comment, this PR needs to be modified because the modification uses the sshKey value of appliance-config.yaml (appliance configuration) instead of install-config.yaml (agent configuration)

/hold

danielerez commented 3 months ago

Considering @danielerez comment, this PR needs to be modified because the modification uses the sshKey value of appliance-config.yaml (appliance configuration) instead of install-config.yaml (agent configuration)

Actually, the agent installer should already handle the usage of sshKey from install-config.yaml. But worth verifying it again:)

/hold

mlorenzofr commented 3 months ago

I agree, and testing it this morning I can verify it works as expected :) Thank you for your help @danielerez I'll close the PR, it's not necessary.