Add Vsock support to KubeVirt #8546

zhuchenwang · 2022-09-29T22:24:20Z

Signed-off-by: Zhuchen Wang zcwang@google.com

What this PR does / why we need it:

We will have our guest agent running in the VM listening to the Vsock channel. We want to expose a REST endpoint on virt-handler so that our controller can just call the REST API on virt-handler and the virt-handler will forward the stream to the guest agent, similar to vnc and serial.

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 #

Special notes for your reviewer:
This PR implements the same feature as #8531. The major difference is that virt-handler is responsible for allocating the CID in this PR. We had a lot of internal discussion about the attack surface for this feature and that's why we hold the change to upstream. It's glad to see other people has the same requirement. I hope this PR can help the reviewers to understand a slightly different use case and decide which way is good for both cases. Also credit to PiotrProkop, because I took the webhook code and integrate from #8531.

Release note:

Provides the Vsock feature for KubeVirt VMs.

kubevirt-bot · 2022-09-29T22:24:29Z

Hi @zhuchenwang. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rmohr · 2022-09-29T22:55:43Z

/ok-to-test

pkg/virt-handler/cgroup/util.go

PiotrProkop · 2022-09-30T08:32:46Z

one small nit, otherwise LGTM

rmohr · 2022-09-30T09:54:50Z

tests/vmi_vsock_test.go

+			tests.RunMigrationAndExpectCompletion(virtClient, migration, tests.MigrationWaitTime)
+
+			By("Ensuring the Vsock is still present")
+			Expect(console.SafeExpectBatch(vmi, []expect.Batcher{


After considering the comment from @vladikr I am not sure if libvirt would pick up the new vosck CID on the new server. There is a good chance that it kept the one from the old CID (which in this case accidentally matches the new assigned CID).

From my understanding after reading vsock man page libvirt will try to retain CID and if it is unavailable on the host after live-migration it will be assigned with a new CID and it will notify the guest about it.

I think this only means that live migration is supported and the hypervisor may have to give it a different cid. It does not mean that you get from the kernel another cid without involving the hypervisor (at least pretty sure).

Libvirt does retain the CID if available, which is demonstrated in the test case.

Also the email thread confirms the behavior https://www.spinics.net/lists/netdev/msg631998.html.

Also the email thread confirms the behavior https://www.spinics.net/lists/netdev/msg631998.html.

Pefect. That is how it should be. What I did not think about is, that the source libvirt is defining the destionation domain xml during migration. This is a little bit cumbersome for us, since we have to transfer additional info (like here the new CID) back to the source handler, so that we can set it on the target ...

We do that for some info already and the final domain modification is happening at https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-launcher/virtwrap/live-migration-source.go#L257 (you can find it when following the MigrateVMI grcp call in virt-launcher.

I think our best option is to add the CID to the VMI status and let' virt-controller hand out the CIDs. We don't have a back-flow channel between target-handler and source-handler. It would be similar to the target CPU set.

@rmohr yeah, we will just connect from VM to the host(CID of value 2).

It may just be much simpler for you to do it in virt-controller. The status would be your persistent storage and you have an initial sync "guarantee" after the cache sync where you can warm up your in-memory-view of the CIDs allocated. But what you propose will work too. Going with virt-controller just feels more robust and means no possible file-leftovers or filing cleanup steps. You also don't have to change the CID between miagrations and don't have to send that info back-and-forth.

Thanks for the suggestion. This makes sense to me and I also want to simplify things. I will make the corresponding change.

Updated to have virt-controller allocate the CIDs.

Could we use the network namespace support in vsock to simplify the cid allocation? From the example, you can see there the 2 VMs are using the same cid.

No, it got never merged in the kernel. We had initially the same thought 💭

fabiand · 2022-09-30T14:02:05Z

Hey. Would it be possible to then get rid of todays guest agent channel, and use vsock there as well?

IOW: Can't we avoid to introduce a second channel type, and instead replace the one existing?

rmohr · 2022-09-30T17:28:07Z

Hey. Would it be possible to then get rid of todays guest agent channel, and use vsock there as well?

IOW: Can't we avoid to introduce a second channel type, and instead replace the one existing?

Hm, vsock has security implications, not sure if you read the other pr discussion, so it is at this moment not for everyone. Probably at some point. I would not touch qemu guest agent at this stage. Nothing would speak against extending vsock later on, the implementation is neutral regarding to that. Also we do not automatically attach vsock devices to not break the API. What do you think?

fabiand · 2022-10-05T08:10:13Z

Thanks for the summary Roman.

I can follow your reasoning, yet it is important to find points of alignment/deduplication imo.

rmohr · 2022-10-10T12:12:37Z

/retest

rmohr · 2022-10-10T12:19:58Z

Thanks for the summary Roman.

I can follow your reasoning, yet it is important to find points of alignment/deduplication imo.

@fabiand could you explain what you mean? I am not sure I understand your concern or thought.

staging/src/kubevirt.io/api/core/v1/types.go

rmohr · 2022-10-11T07:23:38Z

/retest

pkg/virt-controller/watch/vsock_test.go

PiotrProkop

LGTM

kubevirt-bot · 2022-10-14T07:40:52Z

@PiotrProkop: changing LGTM is restricted to collaborators

In response to this:

LGTM

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.

rmohr · 2022-10-14T07:45:44Z

/retest

rmohr

One last tiny request. Then I think we are done.

pkg/virt-controller/watch/vsock_test.go

virt-controller is responsible for allocating the CID for VMIs in the cluster. The CIDs are unique cluster wide, so that the migrated VMs will have the same CIDs. Signed-off-by: Zhuchen Wang <zcwang@google.com>

rmohr · 2022-10-14T17:26:20Z

/lgtm
/approve

/hold

Let's wait for the proposal to be approved and merged. Then we can unhold.

kubevirt-bot · 2022-10-14T17:26:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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

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

rmohr · 2022-10-18T10:00:42Z

/unhold

proposal is merged: kubevirt/community#191 (comment)

rmohr · 2022-10-18T10:00:54Z

/test all

rmohr · 2022-10-18T10:50:15Z

/retest

PiotrProkop · 2022-10-18T12:40:19Z

/retest

rmohr · 2022-10-18T13:36:59Z

/retest

kubevirt-bot · 2022-10-18T13:47:06Z

@zhuchenwang: 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-check-tests-for-flakes	`f63babd`	link	false	`/test pull-kubevirt-check-tests-for-flakes`

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.

This syncs the seccomp-profile with the latest changes in containerd's profile, applying the same changes as containerd/containerd@17a9324 Some background from the associated ticket: > We want to use vsock for guest-host communication on KubeVirt > (https://github.com/kubevirt/kubevirt). In KubeVirt we run VMs in pods. > > However since anyone can just connect from any pod to any VM with the > default seccomp settings, we cannot limit connection attempts to our > privileged node-agent. > > ### Describe the solution you'd like > We want to deny the `socket` syscall for the `AF_VSOCK` family by default. > > I see in [1] and [2] that AF_VSOCK was actually already blocked for some > time, but that got reverted since some architectures support the `socketcall` > syscall which can't be restricted properly. However we are mostly interested > in `arm64` and `amd64` where limiting `socket` would probably be enough. > > ### Additional context > I know that in theory we could use our own seccomp profiles, but we would want > to provide security for as many users as possible which use KubeVirt, and there > it would be very helpful if this protection could be added by being part of the > DefaultRuntime profile to easily ensure that it is active for all pods [3]. > > Impact on existing workloads: It is unlikely that this will disturb any existing > workload, becuase VSOCK is almost exclusively used for host-guest commmunication. > However if someone would still use it: Privileged pods would still be able to > use `socket` for `AF_VSOCK`, custom seccomp policies could be applied too. > Further it was already blocked for quite some time and the blockade got lifted > due to reasons not related to AF_VSOCK. > > The PR in KubeVirt which adds VSOCK support for additional context: [4] > > [1]: moby#29076 (comment) > [2]: moby@dcf2632 > [3]: https://kubernetes.io/docs/tutorials/security/seccomp/#enable-the-use-of-runtimedefault-as-the-default-seccomp-profile-for-all-workloads > [4]: kubevirt/kubevirt#8546 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This syncs the seccomp-profile with the latest changes in containerd's profile, applying the same changes as containerd/containerd@17a9324 Some background from the associated ticket: > We want to use vsock for guest-host communication on KubeVirt > (https://github.com/kubevirt/kubevirt). In KubeVirt we run VMs in pods. > > However since anyone can just connect from any pod to any VM with the > default seccomp settings, we cannot limit connection attempts to our > privileged node-agent. > > ### Describe the solution you'd like > We want to deny the `socket` syscall for the `AF_VSOCK` family by default. > > I see in [1] and [2] that AF_VSOCK was actually already blocked for some > time, but that got reverted since some architectures support the `socketcall` > syscall which can't be restricted properly. However we are mostly interested > in `arm64` and `amd64` where limiting `socket` would probably be enough. > > ### Additional context > I know that in theory we could use our own seccomp profiles, but we would want > to provide security for as many users as possible which use KubeVirt, and there > it would be very helpful if this protection could be added by being part of the > DefaultRuntime profile to easily ensure that it is active for all pods [3]. > > Impact on existing workloads: It is unlikely that this will disturb any existing > workload, becuase VSOCK is almost exclusively used for host-guest commmunication. > However if someone would still use it: Privileged pods would still be able to > use `socket` for `AF_VSOCK`, custom seccomp policies could be applied too. > Further it was already blocked for quite some time and the blockade got lifted > due to reasons not related to AF_VSOCK. > > The PR in KubeVirt which adds VSOCK support for additional context: [4] > > [1]: moby#29076 (comment) > [2]: moby@dcf2632 > [3]: https://kubernetes.io/docs/tutorials/security/seccomp/#enable-the-use-of-runtimedefault-as-the-default-seccomp-profile-for-all-workloads > [4]: kubevirt/kubevirt#8546 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 57b2290) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

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. labels Sep 29, 2022

kubevirt-bot requested review from kbidarkar and rmohr September 29, 2022 22:24

kubevirt-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL labels Sep 29, 2022

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2022

PiotrProkop reviewed Sep 30, 2022

View reviewed changes

pkg/virt-handler/cgroup/util.go Outdated Show resolved Hide resolved

rmohr reviewed Sep 30, 2022

View reviewed changes

kubevirt-bot added size/XXL and removed size/XL labels Sep 30, 2022

zhuchenwang force-pushed the main branch from d783418 to 1463529 Compare October 3, 2022 23:27

zhuchenwang force-pushed the main branch from 1463529 to b474ce0 Compare October 5, 2022 16:40

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2022

zhuchenwang force-pushed the main branch from b474ce0 to 43684f0 Compare October 7, 2022 20:22

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2022

zhuchenwang force-pushed the main branch from 43684f0 to 2e681bf Compare October 11, 2022 04:17

kubevirt-bot added size/XL and removed size/XXL labels Oct 11, 2022

PiotrProkop reviewed Oct 11, 2022

View reviewed changes

staging/src/kubevirt.io/api/core/v1/types.go Outdated Show resolved Hide resolved

rmohr reviewed Oct 13, 2022

View reviewed changes

pkg/virt-controller/watch/vsock_test.go Show resolved Hide resolved

zhuchenwang force-pushed the main branch 2 times, most recently from 99a7126 to f31d29f Compare October 13, 2022 18:49

zhuchenwang requested a review from rmohr October 13, 2022 19:24

PiotrProkop approved these changes Oct 14, 2022

View reviewed changes

rmohr reviewed Oct 14, 2022

View reviewed changes

pkg/virt-controller/watch/vsock_test.go Show resolved Hide resolved

PiotrProkop mentioned this pull request Oct 14, 2022

Vsock support in Kubevirt VMs. #8531

Closed

Add Vsock support to KubeVirt

f63babd

virt-controller is responsible for allocating the CID for VMIs in the cluster. The CIDs are unique cluster wide, so that the migrated VMs will have the same CIDs. Signed-off-by: Zhuchen Wang <zcwang@google.com>

zhuchenwang force-pushed the main branch from f31d29f to f63babd Compare October 14, 2022 17:15

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 14, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2022

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022

kubevirt-bot merged commit cc2f93d into kubevirt:main Oct 18, 2022

thaJeztah mentioned this pull request Dec 1, 2022

seccomp: block socket calls to AF_VSOCK in default profile moby/moby#44562

Merged

thaJeztah mentioned this pull request Dec 1, 2022

[23.0 backport] seccomp: block socket calls to AF_VSOCK in default profile moby/moby#44563

Merged

thaJeztah mentioned this pull request Dec 1, 2022

[20.10 backport] seccomp: block socket calls to AF_VSOCK in default profile moby/moby#44564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Vsock support to KubeVirt #8546

Add Vsock support to KubeVirt #8546

zhuchenwang commented Sep 29, 2022

kubevirt-bot commented Sep 29, 2022

rmohr commented Sep 29, 2022

PiotrProkop commented Sep 30, 2022

rmohr Sep 30, 2022

PiotrProkop Sep 30, 2022 •

edited

rmohr Sep 30, 2022

zhuchenwang Oct 3, 2022 •

edited

rmohr Oct 4, 2022

PiotrProkop Oct 10, 2022

zhuchenwang Oct 10, 2022

zhuchenwang Oct 11, 2022

alicefr Oct 12, 2022 •

edited

rmohr Oct 12, 2022

fabiand commented Sep 30, 2022

rmohr commented Sep 30, 2022

fabiand commented Oct 5, 2022

rmohr commented Oct 10, 2022

rmohr commented Oct 10, 2022

rmohr commented Oct 11, 2022

PiotrProkop left a comment

kubevirt-bot commented Oct 14, 2022

rmohr commented Oct 14, 2022

rmohr left a comment

rmohr commented Oct 14, 2022

kubevirt-bot commented Oct 14, 2022

rmohr commented Oct 18, 2022

rmohr commented Oct 18, 2022

rmohr commented Oct 18, 2022

PiotrProkop commented Oct 18, 2022

rmohr commented Oct 18, 2022

kubevirt-bot commented Oct 18, 2022 •

edited

Add Vsock support to KubeVirt #8546

Add Vsock support to KubeVirt #8546

Conversation

zhuchenwang commented Sep 29, 2022

kubevirt-bot commented Sep 29, 2022

rmohr commented Sep 29, 2022

PiotrProkop commented Sep 30, 2022

rmohr Sep 30, 2022

Choose a reason for hiding this comment

PiotrProkop Sep 30, 2022 • edited

Choose a reason for hiding this comment

rmohr Sep 30, 2022

Choose a reason for hiding this comment

zhuchenwang Oct 3, 2022 • edited

Choose a reason for hiding this comment

rmohr Oct 4, 2022

Choose a reason for hiding this comment

PiotrProkop Oct 10, 2022

Choose a reason for hiding this comment

zhuchenwang Oct 10, 2022

Choose a reason for hiding this comment

zhuchenwang Oct 11, 2022

Choose a reason for hiding this comment

alicefr Oct 12, 2022 • edited

Choose a reason for hiding this comment

rmohr Oct 12, 2022

Choose a reason for hiding this comment

fabiand commented Sep 30, 2022

rmohr commented Sep 30, 2022

fabiand commented Oct 5, 2022

rmohr commented Oct 10, 2022

rmohr commented Oct 10, 2022

rmohr commented Oct 11, 2022

PiotrProkop left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 14, 2022

rmohr commented Oct 14, 2022

rmohr left a comment

Choose a reason for hiding this comment

rmohr commented Oct 14, 2022

kubevirt-bot commented Oct 14, 2022

rmohr commented Oct 18, 2022

rmohr commented Oct 18, 2022

rmohr commented Oct 18, 2022

PiotrProkop commented Oct 18, 2022

rmohr commented Oct 18, 2022

kubevirt-bot commented Oct 18, 2022 • edited

PiotrProkop Sep 30, 2022 •

edited

zhuchenwang Oct 3, 2022 •

edited

alicefr Oct 12, 2022 •

edited

kubevirt-bot commented Oct 18, 2022 •

edited