Propogate public-keys to cloud-init NoCloud meta-data #10231

kvaps · 2023-08-03T11:23:17Z

What this PR does / why we need it:

Cloud-init's NoCloud also supports specifying public-keys in meta-data.
This patch enables propogation of ssh-keys into instance metadata.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #10249

Special notes for your reviewer:

ExampleUsage

accessCredentials:
- sshPublicKey:
    source:
      secret:
        secretName: my-pub-key
    propagationMethod:
      noCloud: {}
volumes:
- cloudInitNoCloud: {}
  name: cloudinitdisk

Release note:

Propogate public-keys to cloud-init NoCloud meta-data

maiqueb · 2023-08-03T14:02:21Z

Thanks for evolving the nocloud cloud-init datasource.

I think this makes sense - and addresses an important gap: we couldn't use secrets when defining the ssh keys and define a netplanv2 network configuration in the same datasource.

We would just need you to fix the unit tests (you need to adjust to the new resolveNoCloudSecrets function's signature), and provide an e2e test assuring the feature works as intended.

kvaps · 2023-08-05T11:47:08Z

/retest

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

kvaps · 2023-08-06T17:00:14Z

Job is done @maiqueb 🙂

maiqueb

I have some very minor comments into how I think the e2e test should look like.

Plus some suggestions on what should be checked in the unit tests, and finally an opinionated comment into how I think the code should look like.

But overall really good, this is a nice feature to have around.

Thanks for the contribution !

pkg/cloud-init/cloud-init.go

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go

tests/credentials_test.go

alaypatel07 · 2023-08-11T15:31:16Z

/cc

0xFelix · 2023-09-04T12:01:25Z

@kvaps Can you please update it to match @lyarwood's suggestion? #10231 (comment)

The function name and logic should match.

kvaps · 2023-09-04T13:32:36Z

@0xFelix @lyarwood sure, done

0xFelix · 2023-09-04T13:44:54Z

Excuse me if I'm wrong, but now the names do not match the logic again? Please change just the names to isNoCloudAccessCredential and isConfigDriveAccessCredential.

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

kvaps · 2023-09-04T13:56:10Z

yeah, you're right, fixed that, sorry

0xFelix

Apart from one comment it looks good to me.

Thanks!

/lgtm

0xFelix · 2023-09-04T14:20:16Z

tests/credentials_test.go


+		It("[test_id:TODO]should have ssh-key under authorized keys added by NoCloud", func() {


You could write these tests as a table, but it can be done in a follow up because there is more to clean up here (e.g. swapping NewRandomVMIWithEphemeralDiskHighMemory with libvmi).

0xFelix · 2023-09-05T09:23:59Z

/retest-required

lyarwood · 2023-09-05T10:27:15Z

/lgtm

kvaps · 2023-09-05T12:10:22Z

/retest-required

aburdenthehand · 2023-09-07T10:23:01Z

/cc @iholder101

0xFelix · 2023-09-21T09:01:24Z

Ping @iholder101 @alicefr

iholder101

Thank you @kvaps! This looks great.
Sorry to chime in late on this.

/approve
/hold

Holding to let @alicefr respond if she wants to. If she doesn't respond soon, feel free to unhold 👍

iholder101 · 2023-09-21T12:12:57Z

tests/utils.go

+func NewRandomVMIWithEphemeralDiskAndNoCloudUserdataHighMemory(containerImage string, userData string) *v1.VirtualMachineInstance {
+	vmi := NewRandomVMIWithEphemeralDiskAndNoCloudUserdata(containerImage, userData)
+
+	vmi.Spec.Domain.Resources.Requests[k8sv1.ResourceMemory] = resource.MustParse("512M")
+	return vmi
+}
+


For the record, we sometime need to create similar functions in libvmi

iholder101 · 2023-09-21T13:56:33Z

pkg/cloud-init/cloud-init.go

@@ -162,15 +163,73 @@ func ReadCloudInitVolumeDataSource(vmi *v1.VirtualMachineInstance, secretSourceD
 	return nil, nil
 }

+func resolveSSHPublicKeys(accessCredentials []v1.AccessCredential, secretSourceDir string, methodNoCloud, methodConfigDrive bool) (map[string]string, error) {


nit: I would loose the bool arguments and go with something like

type AccessCredentialPropagationMethod string const ( methodNoCloud = "methodNoCloud" methodConfigDrive = "methodConfigDrive" ) // ... func resolveSSHPublicKeys(accessCredentials []v1.AccessCredential, propagationMethod AccessCredentialPropagationMethod) (map[string]string, error) { // ... }

If we'd support more methods in the future it's better not to end up with many bool arguments that are all false but one.

kubevirt-bot · 2023-09-21T14:04:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iholder101

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [iholder101]

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

maiqueb

Really happy to see this feature accepted.

0xFelix · 2023-09-25T12:32:50Z

Feeling free to unhold ;)

/hold cancel

0xFelix · 2023-09-25T12:35:47Z

/retest-required

kubevirt-bot · 2023-09-25T16:56:51Z

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

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.26-sig-compute	`6c22622`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.26-sig-compute`
pull-kubevirt-e2e-k8s-1.26-sig-storage	`6c22622`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.26-sig-storage`

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/test-infra repository. I understand the commands that are listed here.

maiqueb · 2023-09-26T08:39:02Z

@0xFelix do we want to back-port this into other releases ?

xpivarc · 2023-09-26T08:40:58Z

@0xFelix do we want to back-port this into other releases ?

Please don't backport features to stable releases, it goes against the meaning of stable releases.

0xFelix · 2023-09-26T08:41:38Z

@maiqueb I'd consider this a new feature, not a bug. So I would not backport it.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Aug 3, 2023

kubevirt-bot requested review from maiqueb and rmohr August 3, 2023 11:23

kubevirt-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L and removed size/S labels Aug 3, 2023

kvaps force-pushed the no-cloud-public-keys branch from cca113d to 7727d5b Compare August 3, 2023 12:02

kvaps force-pushed the no-cloud-public-keys branch from 737289b to ac7ad07 Compare August 4, 2023 08:21

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 4, 2023

kvaps force-pushed the no-cloud-public-keys branch from 8de7d91 to db250c6 Compare August 4, 2023 10:34

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Aug 4, 2023

kvaps force-pushed the no-cloud-public-keys branch from db250c6 to 9054d33 Compare August 4, 2023 11:18

Propogate public-keys to cloud-init NoCloud meta-data

5e3be24

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

kvaps force-pushed the no-cloud-public-keys branch from 3abc0b6 to 5e3be24 Compare August 6, 2023 12:58

maiqueb suggested changes Aug 7, 2023

View reviewed changes

kubevirt-bot assigned maiqueb Aug 7, 2023

kubevirt-bot requested a review from alaypatel07 August 11, 2023 15:31

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 17, 2023

kvaps force-pushed the no-cloud-public-keys branch from 18e8dc0 to ae9d7d7 Compare August 17, 2023 22:13

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Aug 17, 2023

kvaps force-pushed the no-cloud-public-keys branch from 624a457 to 28c4106 Compare September 4, 2023 13:32

apply review suggestions

6c22622

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

kvaps force-pushed the no-cloud-public-keys branch from 28c4106 to 6c22622 Compare September 4, 2023 13:55

0xFelix approved these changes Sep 4, 2023

View reviewed changes

kubevirt-bot assigned 0xFelix Sep 4, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2023

kubevirt-bot assigned lyarwood Sep 5, 2023

kubevirt-bot requested a review from iholder101 September 7, 2023 10:23

iholder101 reviewed Sep 21, 2023

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023

maiqueb approved these changes Sep 21, 2023

View reviewed changes

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2023

kubevirt-bot merged commit 9a94b8c into kubevirt:main Sep 25, 2023
37 of 39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propogate public-keys to cloud-init NoCloud meta-data #10231

Propogate public-keys to cloud-init NoCloud meta-data #10231

kvaps commented Aug 3, 2023 •

edited

maiqueb commented Aug 3, 2023

kvaps commented Aug 5, 2023

kvaps commented Aug 6, 2023

maiqueb left a comment

alaypatel07 commented Aug 11, 2023

0xFelix commented Sep 4, 2023

kvaps commented Sep 4, 2023

0xFelix commented Sep 4, 2023

kvaps commented Sep 4, 2023

0xFelix left a comment

0xFelix Sep 4, 2023

0xFelix commented Sep 5, 2023

lyarwood commented Sep 5, 2023

kvaps commented Sep 5, 2023

aburdenthehand commented Sep 7, 2023

0xFelix commented Sep 21, 2023

iholder101 left a comment

iholder101 Sep 21, 2023

iholder101 Sep 21, 2023

kubevirt-bot commented Sep 21, 2023

maiqueb left a comment

0xFelix commented Sep 25, 2023

0xFelix commented Sep 25, 2023

kubevirt-bot commented Sep 25, 2023

maiqueb commented Sep 26, 2023

xpivarc commented Sep 26, 2023

0xFelix commented Sep 26, 2023


		It("[test_id:TODO]should have ssh-key under authorized keys added by NoCloud", func() {

Propogate public-keys to cloud-init NoCloud meta-data #10231

Propogate public-keys to cloud-init NoCloud meta-data #10231

Conversation

kvaps commented Aug 3, 2023 • edited

maiqueb commented Aug 3, 2023

kvaps commented Aug 5, 2023

kvaps commented Aug 6, 2023

maiqueb left a comment

Choose a reason for hiding this comment

alaypatel07 commented Aug 11, 2023

0xFelix commented Sep 4, 2023

kvaps commented Sep 4, 2023

0xFelix commented Sep 4, 2023

kvaps commented Sep 4, 2023

0xFelix left a comment

Choose a reason for hiding this comment

0xFelix Sep 4, 2023

Choose a reason for hiding this comment

0xFelix commented Sep 5, 2023

lyarwood commented Sep 5, 2023

kvaps commented Sep 5, 2023

aburdenthehand commented Sep 7, 2023

0xFelix commented Sep 21, 2023

iholder101 left a comment

Choose a reason for hiding this comment

iholder101 Sep 21, 2023

Choose a reason for hiding this comment

iholder101 Sep 21, 2023

Choose a reason for hiding this comment

kubevirt-bot commented Sep 21, 2023

maiqueb left a comment

Choose a reason for hiding this comment

0xFelix commented Sep 25, 2023

0xFelix commented Sep 25, 2023

kubevirt-bot commented Sep 25, 2023

maiqueb commented Sep 26, 2023

xpivarc commented Sep 26, 2023

0xFelix commented Sep 26, 2023

kvaps commented Aug 3, 2023 •

edited