virtiofs: Run unprivileged without feature gate #10657

germag · 2023-10-31T13:46:04Z

What this PR does / why we need it:

Currently, virtiofsd can only run unprivileged for sharing ConfigMaps, Secrets, etc., but to be able to share filesystem PVs requires the feature gate ExperimentalVirtiofsSupport.

This PR removes that restriction, making it possible to share filesystem PVs using unprivileged virtiofsd if the feature gate is disabled, and continue running as root if the feature gate is enabled. So this change doesn't affect users that are already running a privileged virtiofsd.

Running virtiofsd without privileges has some limitations, for instance, virtiofsd cannot switch uids/gids so all the new files created inside the PV will be owned by the same uid/gid (the virtiofsd uid/gid) regardless of the uid/gid of the process that created them inside the VM. After the merge, all the limitations and workarounds will be documented (see kubevirt/user-guide#734).

Signed-off-by: German Maglione gmaglione@redhat.com
Tested-by: Javier Cano Cano jcanocan@redhat.com

Release note:

Exposing Filesystem Persistent Volumes (PVs)  to the VM using unprivilege virtiofsd.

germag · 2023-10-31T13:46:42Z

/cc @jcanocan
/cc @alicefr

germag · 2023-10-31T13:47:22Z

/ok-to-test

pkg/virt-controller/services/virtiofs_test.go

pkg/virtiofs/virtiofs.go

tests/virtiofs/datavolume.go

0xFelix · 2023-10-31T17:35:33Z

tests/virtiofs/datavolume.go

-			)
-		})
+		if !libstorage.HasCDI() {
+			Skip("Skip DataVolume tests when CDI is not present")


Are the tests expected to run without CDI at all?

No, we skip the test if CDI is not present (that check was already in the test before my changes)

Yes, my question was more aimed at whether KubeVirt tests run at all without CDI. IOW can we expected CDI always to be present when the tests run (without the need for another check or skip, not present is expected to result in failure)? IIRC it is present by default in kubevirtci.

Ohh, I don't know really, maybe this was copy-pasted from tests/storage/datavolume.go, that was the original location of this test (btw, the first test in that file checks twice per test if the CDI is present, line 91 and 99).

I can remove the check if is unnecessary

Are these tests only intended for this CI or also for someone who does not have/needs CDI?

I can only speak for CNV where CDI is present in downstream tests.

alicefr · 2023-11-01T08:47:10Z

pkg/virt-controller/services/virtiofs_test.go

+	}
+	config, _, kvInformer := testutils.NewFakeClusterConfigUsingKV(kv)
+
+	enableFeatureGate := func(featureGate string) {


Not for this PR, but we should really use a common function for doing this

pkg/virtiofs/virtiofs.go

alicefr · 2023-11-01T09:03:42Z

The change looks good to me
/lgtm
/cc @xpivarc

germag · 2023-11-02T13:55:08Z

/retest-required

jcanocan · 2023-11-03T08:07:25Z

/retest-required

germag · 2023-11-03T17:34:25Z

/retest-required

alicefr · 2023-11-10T07:45:02Z

IMO, the code looks good
/approve
/hold
Wait for another round of reviews, @0xFelix @xpivarc, does this change look sane to you?

kubevirt-bot · 2023-11-10T07:45:11Z

[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

0xFelix

/lgtm

Looks good to me, leaving hold in place to give @xpivarc a chance to look at it.

xpivarc

I am a little bit worried that this will give the wrong impression that you could share any volume. Can we clarify this in the PR description + release note?

pkg/virt-controller/services/virtiofs_test.go

tests/virtiofs/datavolume.go

Currently, virtiofsd can only run unprivileged for sharing ConfigMaps, Secrets, etc., so requiring the feature gate `ExperimentalVirtiofsSupport` enabled, to be able to share PVs. Let's remove that restriction, making possible to share PVs using unprivileged virtiofsd if the feature gate is disabled. This has some limitations, for instance virtiofsd cannot switch the uid/gid. Signed-off-by: German Maglione <gmaglione@redhat.com>

germag · 2023-11-10T13:38:15Z

I am a little bit worried that this will give the wrong impression that you could share any volume. Can we clarify this in the PR description + release note?

Do you mean block PVs?, if so, is it enough calling them "filesystem PVs"?
could you expand on which ones cannot be shared using virtiofs?

xpivarc · 2023-11-10T14:33:37Z

I am a little bit worried that this will give the wrong impression that you could share any volume. Can we clarify this in the PR description + release note?

Do you mean block PVs?, if so, is it enough calling them "filesystem PVs"? could you expand on which ones cannot be shared using virtiofs?

Yes, the filesystem PVs sound good. What I am more worried about is the privileges issue, users need to be aware there is a limitation to it.

germag · 2023-11-10T15:33:42Z

I am a little bit worried that this will give the wrong impression that you could share any volume. Can we clarify this in the PR description + release note?

Do you mean block PVs?, if so, is it enough calling them "filesystem PVs"? could you expand on which ones cannot be shared using virtiofs?

Yes, the filesystem PVs sound good. What I am more worried about is the privileges issue, users need to be aware there is a limitation to it.

We plan to update the documentation, with all the limitations, as soon this PR is merged

xpivarc · 2023-11-10T18:24:04Z

I am a little bit worried that this will give the wrong impression that you could share any volume. Can we clarify this in the PR description + release note?

Do you mean block PVs?, if so, is it enough calling them "filesystem PVs"? could you expand on which ones cannot be shared using virtiofs?

Yes, the filesystem PVs sound good. What I am more worried about is the privileges issue, users need to be aware there is a limitation to it.

We plan to update the documentation, with all the limitations, as soon this PR is merged

Would you mind posting placeholder PR?

germag · 2023-11-14T10:06:51Z

/retest-required

jcanocan · 2023-11-14T15:56:53Z

I am a little bit worried that this will give the wrong impression that you could share any volume. Can we clarify this in the PR description + release note?

Do you mean block PVs?, if so, is it enough calling them "filesystem PVs"? could you expand on which ones cannot be shared using virtiofs?

Yes, the filesystem PVs sound good. What I am more worried about is the privileges issue, users need to be aware there is a limitation to it.

We plan to update the documentation, with all the limitations, as soon this PR is merged

Would you mind posting placeholder PR?

Done: kubevirt/user-guide#734

xpivarc

/hold cancel

kubevirt-commenter-bot · 2023-11-14T21:16:56Z

/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.

kubevirt-commenter-bot · 2023-11-15T00:16:57Z

/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.

kubevirt-commenter-bot · 2023-11-15T07:16:59Z

/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.

kubevirt-bot · 2023-11-15T08:10:39Z

@germag: 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	`8573154`	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.

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/XL labels Oct 31, 2023

kubevirt-bot requested review from 0xFelix and kbidarkar October 31, 2023 13:46

kubevirt-bot requested review from alicefr and jcanocan October 31, 2023 13:46

kubevirt-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 31, 2023

0xFelix reviewed Oct 31, 2023

View reviewed changes

germag force-pushed the virtiofsd-unprivileged branch from 890c0cc to 3ca05dc Compare October 31, 2023 18:38

kubevirt-bot added size/L and removed size/XL labels Oct 31, 2023

germag force-pushed the virtiofsd-unprivileged branch from 3ca05dc to 24d357e Compare October 31, 2023 18:41

alicefr reviewed Nov 1, 2023

View reviewed changes

kubevirt-bot requested a review from xpivarc November 1, 2023 09:03

kubevirt-bot assigned alicefr Nov 1, 2023

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

germag force-pushed the virtiofsd-unprivileged branch from 24d357e to ca74377 Compare November 2, 2023 09:10

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2023

germag force-pushed the virtiofsd-unprivileged branch from ca74377 to 8e9ad37 Compare November 2, 2023 09:27

germag force-pushed the virtiofsd-unprivileged branch from 8e9ad37 to 757ec2e Compare November 2, 2023 17:30

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

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

0xFelix approved these changes Nov 10, 2023

View reviewed changes

kubevirt-bot assigned 0xFelix Nov 10, 2023

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

xpivarc reviewed Nov 10, 2023

View reviewed changes

germag force-pushed the virtiofsd-unprivileged branch from 757ec2e to 8573154 Compare November 10, 2023 13:32

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2023

jcanocan mentioned this pull request Nov 14, 2023

virtiofs: adds non-privileged virtiofsd sharing kubevirt/user-guide#734

Merged

xpivarc approved these changes Nov 14, 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 Nov 14, 2023

kubevirt-bot assigned xpivarc Nov 14, 2023

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

kubevirt-bot merged commit d39845c into kubevirt:main Nov 15, 2023
36 of 37 checks passed

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virtiofs: Run unprivileged without feature gate #10657

virtiofs: Run unprivileged without feature gate #10657

germag commented Oct 31, 2023 •

edited

germag commented Oct 31, 2023

germag commented Oct 31, 2023

0xFelix Oct 31, 2023

germag Oct 31, 2023

0xFelix Nov 2, 2023

germag Nov 2, 2023 •

edited

germag Nov 2, 2023

0xFelix Nov 2, 2023

alicefr Nov 1, 2023

alicefr commented Nov 1, 2023

germag commented Nov 2, 2023

jcanocan commented Nov 3, 2023

germag commented Nov 3, 2023

alicefr commented Nov 10, 2023

kubevirt-bot commented Nov 10, 2023

0xFelix left a comment

xpivarc left a comment

germag commented Nov 10, 2023

xpivarc commented Nov 10, 2023

germag commented Nov 10, 2023

xpivarc commented Nov 10, 2023

germag commented Nov 14, 2023

jcanocan commented Nov 14, 2023

xpivarc left a comment

kubevirt-commenter-bot commented Nov 14, 2023

kubevirt-commenter-bot commented Nov 15, 2023

kubevirt-commenter-bot commented Nov 15, 2023

kubevirt-bot commented Nov 15, 2023 •

edited

virtiofs: Run unprivileged without feature gate #10657

virtiofs: Run unprivileged without feature gate #10657

Conversation

germag commented Oct 31, 2023 • edited

germag commented Oct 31, 2023

germag commented Oct 31, 2023

0xFelix Oct 31, 2023

Choose a reason for hiding this comment

germag Oct 31, 2023

Choose a reason for hiding this comment

0xFelix Nov 2, 2023

Choose a reason for hiding this comment

germag Nov 2, 2023 • edited

Choose a reason for hiding this comment

germag Nov 2, 2023

Choose a reason for hiding this comment

0xFelix Nov 2, 2023

Choose a reason for hiding this comment

alicefr Nov 1, 2023

Choose a reason for hiding this comment

alicefr commented Nov 1, 2023

germag commented Nov 2, 2023

jcanocan commented Nov 3, 2023

germag commented Nov 3, 2023

alicefr commented Nov 10, 2023

kubevirt-bot commented Nov 10, 2023

0xFelix left a comment

Choose a reason for hiding this comment

xpivarc left a comment

Choose a reason for hiding this comment

germag commented Nov 10, 2023

xpivarc commented Nov 10, 2023

germag commented Nov 10, 2023

xpivarc commented Nov 10, 2023

germag commented Nov 14, 2023

jcanocan commented Nov 14, 2023

xpivarc left a comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Nov 14, 2023

kubevirt-commenter-bot commented Nov 15, 2023

kubevirt-commenter-bot commented Nov 15, 2023

kubevirt-bot commented Nov 15, 2023 • edited

germag commented Oct 31, 2023 •

edited

germag Nov 2, 2023 •

edited

kubevirt-bot commented Nov 15, 2023 •

edited