Skip to content

Update documentation about dynamic SSH key injection #870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akrejcir
Copy link

What this PR does / why we need it:

  • Added deprecation notice, that in a future release dynamic SSH key injection will stop working for older guest agents
  • Changed wording, so that there are not 4 Note:... blocks right next to each other.

Special notes for your reviewer:
This should be merged after the deprecation PR is merged: kubevirt/kubevirt#13881

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

None

- Added deprecation notice, that in a future release
  dynamic SSH key injection will stop working for older guest agents

- Changed wording, so that there are not 4 "Note:" blocks
  right next to each other.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@kubevirt-bot kubevirt-bot requested a review from dhiller February 11, 2025 13:03
@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Feb 11, 2025
@akrejcir
Copy link
Author

/cc @0xFelix @jcanocan

Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2025
> **Requirement:** The qemu-guest-agent must be installed within the guest.

> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
> and will stop working in a future release.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> and will stop working in a future release.
> and it will stop working in a future release.

> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
> and will stop working in a future release.

> **Important:** When using `qemuGuestAgent` propagation,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> **Important:** When using `qemuGuestAgent` propagation,
> **Important:** When using `qemuGuestAgent` propagation method,

Comment on lines +166 to +167
> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
> and will stop working in a future release.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
> and will stop working in a future release.
> **Deprecation Notice:** Support for qemu-guest-agent versions older than 5.2 is deprecated
> and will be removed in a future release.

@akrejcir
Copy link
Author

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2025
Copy link
Member

@aburdenthehand aburdenthehand left a comment

Choose a reason for hiding this comment

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

I think we can help to make this easier for the reader by breaking it up with headings, and then using the admonition markdown available through the Material theme.
I've tried to incorporate the other feedback as well.
Hopefully I understood the content and the suggestions make sense :)

>
> Note: When using qemuGuestAgent propagation,
> the `/home/$USER/.ssh/authorized_keys` file will be owned by the guest agent.
> **Requirement:** The qemu-guest-agent must be installed within the guest.
Copy link
Member

Choose a reason for hiding this comment

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

These 4 different admonition types are unfortunately rendered indistinguishable grey here.
I suggest we break this into two headings and then use the admonition markdown. So I'd make this:

Suggested change
> **Requirement:** The qemu-guest-agent must be installed within the guest.
#### Requirements
The `qemu-guest-agent` must be installed within the guest.

> the `/home/$USER/.ssh/authorized_keys` file will be owned by the guest agent.
> **Requirement:** The qemu-guest-agent must be installed within the guest.

> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
!!! note "Deprecation notice"
Support for `qemu-guest-agent` versions older than 5.2 is deprecated and will be removed in a future release.

https://squidfunk.github.io/mkdocs-material/reference/admonitions/#changing-the-title

> **Deprecation Notice:** The implementation supporting qemu-guest-agent versions older than 5.2 is deprecated
> and will stop working in a future release.

> **Important:** When using `qemuGuestAgent` propagation,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> **Important:** When using `qemuGuestAgent` propagation,
!!! Important
When using `qemuGuestAgent` propagation method,

You'd need to replace the subsequent '>' characters and add the 4 spaces (as here) to keep it in the same admonition.
I think this also belongs above the 'requirements' heading in the opening text.

Comment on lines +173 to 176
> **Further Reading:** More information about the motivation behind the access credentials API
> can be found in the
> [pull request description](https://github.com/kubevirt/kubevirt/pull/4195)
> that introduced the API.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> **Further Reading:** More information about the motivation behind the access credentials API
> can be found in the
> [pull request description](https://github.com/kubevirt/kubevirt/pull/4195)
> that introduced the API.
For more information about the motivation behind the access credentials API,
see the [pull request description](https://github.com/kubevirt/kubevirt/pull/4195)
that introduced the API.

I would also move this above the requirements heading, and in to the opening text. (iiuc)

> can be found in the
> [pull request description](https://github.com/kubevirt/kubevirt/pull/4195)
> that introduced the API.

In the example below the `Secret` containing the SSH public key is
In the example below, the `Secret` containing the SSH public key is
Copy link
Member

Choose a reason for hiding this comment

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

And then we can add an example heading here to distinguish it from the preceding section.

Suggested change
In the example below, the `Secret` containing the SSH public key is
#### Example: Dynamic SSH key injection in a Secret
In this example, the `Secret` containing the SSH public key is

@kubevirt-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@kubevirt-bot kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2025
@akrejcir
Copy link
Author

akrejcir commented Jun 5, 2025

/remove-lifecycle stale

@kubevirt-bot kubevirt-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants