virtctl: add commands to dynamically set SSH keys and passwords on a VM #9818

akrejcir · 2023-05-29T14:01:23Z

What this PR does / why we need it:
Added 3 commands to virtctl:

virtctl credentials add-ssh-key - Adds an SSH public key to one of the secrets that are specified in AccessCredentials field of a VM or VMI.
virtctl credentials remove-ssh-key - Removes an SSH key from one of the secretes.
virtctl credentials set-password - Sets a user's password in one of the secretes.

Release note:

Added "virtctl credentials" commands to dynamically change SSH keys in a VM, and to set user's password.

akrejcir · 2023-05-29T14:02:39Z

/cc @0xFelix @ksimon1

akrejcir · 2023-05-30T07:22:25Z

/retest

alicefr · 2023-05-30T07:44:50Z

@akrejcir does the secret need already to exist before adding the ssh key? If so, shouldn't we create the secret together with the first key we want to add?

akrejcir · 2023-05-30T07:52:42Z

Good point. Yes the secret needs to exist. We cannot change running VM, but we can create the secret for a stopped VM.

/hold

pkg/virtctl/credentials/credentials_test.go

alicefr · 2023-05-30T08:00:02Z

pkg/virtctl/credentials/credentials.go

+
+func secretContainsKey(secretData map[string][]byte, key string) bool {
+	for _, data := range secretData {
+		lines := strings.Split(string(data), "\n")


Is it correct checking the ssh key line by line? I mean we could have new lines or not but the key is the same? Shouldn't we trim the data and eliminate then \n and then compare?

One data entry in the Secret contains a file, that can contain multiple keys, each on it's own line.
That's why this loop checks each line separately.

pkg/virtctl/credentials/credentials.go

tests/virtctl/credentials.go

alicefr · 2023-05-30T08:23:15Z

What happens if the propagation method for the ssh key injection is cloud-init? Is this command not supported?

0xFelix

This is quite a large PR. Tbh. I'd like to see it split up into smaller an easier to review PRs.

Added some comments.

pkg/virtctl/credentials/credentials.go

pkg/virtctl/credentials/credentials_test.go

alicefr · 2023-06-01T06:47:10Z

This is quite a large PR. Tbh. I'd like to see it split up into smaller an easier to review PRs.

Added some comments.

IMO, if we try to reduce the code duplication into functions, the size of the pr should reasonably decrease

akrejcir · 2023-06-02T15:33:41Z

Added new functionality.

Can you take another look?

akrejcir · 2023-06-05T08:29:42Z

/hold cancel

pkg/virtctl/credentials/common/common.go

pkg/virtctl/credentials/set-password/set-password.go

0xFelix · 2023-06-05T09:25:42Z

pkg/virtctl/credentials/add-key/add-key.go

+	if err != nil {
+		// TODO: Check error type and only execute second patch for some errors
+		// Try adding the array
+		patchData := patchToAddAccessCredentialArray(accessCredential)


When is this necessary?

If the VM has no credentials, the slice is nil, and patch will fail, because path /spec/template/spec/accessCredentials/- does not exist. So this second patch is used to add the array.

pkg/virtctl/credentials/add-key/add-key_test.go

pkg/virtctl/credentials/remove-key/remove-key_test.go

0xFelix · 2023-06-05T09:37:57Z

pkg/virtctl/credentials/set-password/set-password.go

What about clearing the password secret?

There is no QEMU GA command to clear a password that has been set for a user. We could clear the secret, but the VM would still have the password.

ACK, might be worth adding a --clear flag in a follow up.

pkg/virtctl/credentials/set-password/set-password_test.go

akrejcir · 2023-06-06T20:40:19Z

/retest

0xFelix

Can you add examples to the cmds and give some context when they are working (using qemu guest propagation) or not working (using cloud init propagation)?

Also, do you think the init/updating/helper code in the test files could be deduplicated?

pkg/virtctl/credentials/add-key/add-key_test.go

akrejcir · 2023-06-12T10:40:04Z

I've added usage examples.

akrejcir · 2023-06-12T11:09:50Z

@0xFelix , @alicefr Can you take another look?

The command adds a SSH public key to the secret specified in the "AccessCredentials" field of a VM or VMI. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

The command removes specified key from all the secrets that are specified in the "AccessCredentials" list. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

The command modifies a secret to set the user's password. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

0xFelix

The descriptions of the commands could still be a little improved and IMO the unit test code could still be deduplicated but let's do it in a follow up. For now this looks good to me.

/lgtm

akrejcir · 2023-06-12T21:08:11Z

/retest

akrejcir · 2023-06-13T06:48:42Z

/retest

alicefr · 2023-06-13T13:18:35Z

tests/virtctl/credentials.go

+
+		keySecret, err = cli.CoreV1().Secrets(util.NamespaceTestDefault).Create(context.Background(), keySecret, metav1.CreateOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		DeferCleanup(func() {


Is this necessary? Aren't the secrets deleted by default on the namespace test cleanup?

alicefr · 2023-06-13T13:20:55Z

tests/virtctl/credentials.go

+			Expect(secret.Data).To(ContainElement([]byte(testKey2)))
+		})
+
+		It("[test_id:TODO] should add ssh key from a file", func() {


test_id:TODO should be updated

alicefr · 2023-06-13T13:24:07Z

pkg/virtctl/credentials/add-key/add-key.go

+
+	// First, Try to add the new access credential to the existing array.
+	_, err = cli.VirtualMachine(vm.Namespace).Patch(cmd.Context(), vm.Name, types.JSONPatchType, common.MustMarshalPatch(accessCredentialPatch), &metav1.PatchOptions{})
+	if err != nil {


nit:any chances to check the actual error? Maybe with Contains?

alicefr · 2023-06-13T13:28:57Z

/approve
Left a couple of questions and nit comments.
I'm also missing the answer to #9818 (comment) . If not addressed, this could be done in a follow PR

kubevirt-bot · 2023-06-13T13:29:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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~~ [alicefr]

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

kubevirt-bot · 2023-06-14T03:29:46Z

@akrejcir: 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
pull-kubevirt-e2e-k3d-1.25-sriov	`d4b4082`	link	true	`/test pull-kubevirt-e2e-k3d-1.25-sriov`

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.

kubevirt-commenter-bot · 2023-06-14T06:57:07Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

akrejcir · 2023-06-23T14:55:41Z

/cherry-pick release-1.0

kubevirt-bot · 2023-06-23T14:56:21Z

@akrejcir: new pull request created: #9969

In response to this:

/cherry-pick release-1.0

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.

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/XXL labels May 29, 2023

kubevirt-bot requested review from alicefr and xpivarc May 29, 2023 14:01

kubevirt-bot requested review from 0xFelix and ksimon1 May 29, 2023 14:02

akrejcir force-pushed the virtctl-credentials branch from c745e24 to 3f244a1 Compare May 29, 2023 14:27

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

alicefr reviewed May 30, 2023

View reviewed changes

pkg/virtctl/credentials/credentials.go Outdated Show resolved Hide resolved

alicefr reviewed May 30, 2023

View reviewed changes

pkg/virtctl/credentials/credentials.go Outdated Show resolved Hide resolved

tests/virtctl/credentials.go Outdated Show resolved Hide resolved

0xFelix reviewed May 30, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2023

akrejcir force-pushed the virtctl-credentials branch 2 times, most recently from 45ea683 to d4b4082 Compare June 2, 2023 15:33

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2023

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

0xFelix reviewed Jun 5, 2023

View reviewed changes

akrejcir force-pushed the virtctl-credentials branch 3 times, most recently from b640227 to f4bbf9b Compare June 5, 2023 20:07

akrejcir force-pushed the virtctl-credentials branch 2 times, most recently from e1581e8 to f4d4cdf Compare June 6, 2023 14:22

0xFelix reviewed Jun 7, 2023

View reviewed changes

pkg/virtctl/credentials/add-key/add-key_test.go Show resolved Hide resolved

akrejcir force-pushed the virtctl-credentials branch from f4d4cdf to f172881 Compare June 12, 2023 10:39

akrejcir added 3 commits June 12, 2023 14:37

virtctl: add command: credentials add-ssh-key

93177a8

The command adds a SSH public key to the secret specified in the "AccessCredentials" field of a VM or VMI. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

virtctl: add command: credentials remove-ssh-key

bdb4509

The command removes specified key from all the secrets that are specified in the "AccessCredentials" list. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

virtctl: add command: credentials set-password

3249e02

The command modifies a secret to set the user's password. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir force-pushed the virtctl-credentials branch from f172881 to 3249e02 Compare June 12, 2023 12:38

0xFelix approved these changes Jun 12, 2023

View reviewed changes

kubevirt-bot assigned 0xFelix Jun 12, 2023

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

alicefr reviewed Jun 13, 2023

View reviewed changes

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

kubevirt-bot merged commit 7a4a4e3 into kubevirt:main Jun 14, 2023
34 checks passed

akrejcir deleted the virtctl-credentials branch June 21, 2023 06:59

kubevirt-bot mentioned this pull request Jun 23, 2023

[release-1.0] virtctl: add commands to dynamically set SSH keys and passwords on a VM #9969

Closed

0xFelix mentioned this pull request Jul 4, 2023

Feature Request: Ansible Connection Plugin for Kubevirt VMs #10021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virtctl: add commands to dynamically set SSH keys and passwords on a VM #9818

virtctl: add commands to dynamically set SSH keys and passwords on a VM #9818

akrejcir commented May 29, 2023 •

edited

akrejcir commented May 29, 2023

akrejcir commented May 30, 2023

alicefr commented May 30, 2023

akrejcir commented May 30, 2023

alicefr May 30, 2023

akrejcir May 30, 2023

alicefr commented May 30, 2023

0xFelix left a comment

alicefr commented Jun 1, 2023

akrejcir commented Jun 2, 2023

akrejcir commented Jun 5, 2023

0xFelix Jun 5, 2023

akrejcir Jun 5, 2023

0xFelix Jun 5, 2023

akrejcir Jun 5, 2023

0xFelix Jun 6, 2023

akrejcir commented Jun 6, 2023

0xFelix left a comment

akrejcir commented Jun 12, 2023

akrejcir commented Jun 12, 2023

0xFelix left a comment

akrejcir commented Jun 12, 2023

akrejcir commented Jun 13, 2023

alicefr Jun 13, 2023

alicefr Jun 13, 2023

alicefr Jun 13, 2023

alicefr commented Jun 13, 2023 •

edited

kubevirt-bot commented Jun 13, 2023

kubevirt-bot commented Jun 14, 2023 •

edited

kubevirt-commenter-bot commented Jun 14, 2023

akrejcir commented Jun 23, 2023

kubevirt-bot commented Jun 23, 2023

virtctl: add commands to dynamically set SSH keys and passwords on a VM #9818

virtctl: add commands to dynamically set SSH keys and passwords on a VM #9818

Conversation

akrejcir commented May 29, 2023 • edited

akrejcir commented May 29, 2023

akrejcir commented May 30, 2023

alicefr commented May 30, 2023

akrejcir commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr commented May 30, 2023

0xFelix left a comment

Choose a reason for hiding this comment

alicefr commented Jun 1, 2023

akrejcir commented Jun 2, 2023

akrejcir commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir commented Jun 6, 2023

0xFelix left a comment

Choose a reason for hiding this comment

akrejcir commented Jun 12, 2023

akrejcir commented Jun 12, 2023

0xFelix left a comment

Choose a reason for hiding this comment

akrejcir commented Jun 12, 2023

akrejcir commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr commented Jun 13, 2023 • edited

kubevirt-bot commented Jun 13, 2023

kubevirt-bot commented Jun 14, 2023 • edited

kubevirt-commenter-bot commented Jun 14, 2023

akrejcir commented Jun 23, 2023

kubevirt-bot commented Jun 23, 2023

akrejcir commented May 29, 2023 •

edited

alicefr commented Jun 13, 2023 •

edited

kubevirt-bot commented Jun 14, 2023 •

edited