Reconstruct SELinux mount label #113596

jsafrane · 2022-11-03T16:41:27Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Reconstruct SELinux mount option -o context=XYZ and add it to volume manager's Actual State of World (ASW) when kubelet starts. Kubelet then can see if a mounted volume in ASW has the right context that new Pods need and can unmount it and mount back to get the right SELinux context.

SELinux context for CSI volumes is determined from json file, for in-tree volumes it's taken directly from the mount table.

~~In addition, there is a new e2e test that checks the reconstruction gets the right SELinux context and volume from an old pod is un-mounted when needed.~~ E2e tests will be a separate PR.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added reconstruction of SELinux mount context after kubelet restart. Feature SELinuxMountReadWriteOncePod is now fully implemented and kubelet does not lose its cache of SELinux contexts after kubelet process restart.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

kubernetes/enhancements#3548

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1710-selinux-relabeling

k8s-ci-robot · 2022-11-03T16:45:53Z

@jsafrane: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

jsafrane · 2022-11-03T16:52:11Z

/sig storage
/kind feature

jsafrane · 2022-11-04T09:16:39Z

The e2e test needs some work, we can merge it after feature freeze.

xing-yang · 2022-11-07T18:20:10Z

/assign @gnufied @jingxu97

gnufied · 2022-11-07T19:48:22Z

pkg/kubelet/volumemanager/reconciler/reconstruct.go

@@ -183,6 +183,7 @@ func (rc *reconciler) markVolumeState(volume *reconstructedVolume, volumeState o
 		VolumeGidVolume:     volume.volumeGidValue,
 		VolumeSpec:          volume.volumeSpec,
 		VolumeMountState:    volumeState,
+		SELinuxMountContext: volume.seLinuxMountContext,
 	}


Does non-feature gated code needs this?

It does not, removed.

gnufied · 2022-11-07T20:10:04Z

pkg/kubelet/volumemanager/reconciler/reconstruct_new.go

@@ -139,7 +142,7 @@ func (rc *reconciler) updateStatesNew(reconstructedVolumes map[v1.UniqueVolumeNa
 				klog.ErrorS(err, "Could not find device mount path for volume", "volumeName", gvl.volumeName)
 				continue
 			}
-			err = rc.actualStateOfWorld.MarkDeviceAsUncertain(gvl.volumeName, gvl.devicePath, deviceMountPath, "")
+			err = rc.actualStateOfWorld.MarkDeviceAsUncertain(gvl.volumeName, gvl.devicePath, deviceMountPath, seLinuxMountContext)
 			if err != nil {


Should we have tests that verify if selinux information is recorded as expected?

Unit test added

gnufied · 2022-11-07T20:11:27Z

pkg/volume/csi/csi_plugin.go

 	klog.V(4).Info(log("plugin.ConstructVolumeSpec extracted [%#v]", volData))

-	var spec *volume.Spec
+	ret := volume.ReconstructedVolume{
+		SELinuxMountContext: volData[volDataKey.seLinuxMountContext],


Should this be feature gated as others?

Feature gate added

jsafrane · 2022-11-07T22:01:40Z

Rebased, I'll address the feature gates + unit tests tomorrow.

When reconstructing volumes from disk after kubelet restart, reconstruct also context=XYZ mount option and add it to the ActualStateOfWorld.

SELinux context discovered from Pod is not final, it can be cleared when a volume plugin does not support SELinux or the volume is not ReadWriteOncePod. Update the existing log line + add a new one for easier debugging.

gnufied · 2022-11-08T16:05:44Z

pkg/kubelet/volumemanager/reconciler/reconstruct_new.go

@@ -139,7 +142,7 @@ func (rc *reconciler) updateStatesNew(reconstructedVolumes map[v1.UniqueVolumeNa
 				klog.ErrorS(err, "Could not find device mount path for volume", "volumeName", gvl.volumeName)
 				continue
 			}
-			err = rc.actualStateOfWorld.MarkDeviceAsUncertain(gvl.volumeName, gvl.devicePath, deviceMountPath, "")
+			err = rc.actualStateOfWorld.MarkDeviceAsUncertain(gvl.volumeName, gvl.devicePath, deviceMountPath, seLinuxMountContext)
 			if err != nil {
 				klog.ErrorS(err, "Could not mark device is uncertain to actual state of world", "volumeName", gvl.volumeName, "deviceMountPath", deviceMountPath)
 				continue


I am trying to verify(and I forgot some details), but is staging (ie. MountDevice) and Setup create different json files or they create same files? Are we consistently saving selinux context in both places? Is that feature gated?

They create different jsons and I think SELinux is only in the SetUp one.

I see it other way around - only MountDevice seems to be setting - volDataKey.seLinuxMountContext: deviceMounterArgs.SELinuxLabel, and nothing I could see in Setup. IMO that also should be feature gated.

Correction: it was in MountDevice, I added it to both + both feature-gated.

And make it feature gated in both places.

k8s-ci-robot · 2022-11-08T17:12:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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:

~~pkg/kubelet/volumemanager/OWNERS~~ [jsafrane]
~~pkg/volume/OWNERS~~ [jsafrane]
~~test/e2e/storage/OWNERS~~ [jsafrane]

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

gnufied · 2022-11-08T18:50:15Z

/lgtm

k8s-ci-robot · 2022-11-08T19:33:06Z

@jsafrane: 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-kubernetes-e2e-capz-conformance	0a34042f29529d2f540d68e6b55559a0bb3016bf	link	false	`/test pull-kubernetes-e2e-capz-conformance`
pull-kubernetes-e2e-capz-windows-containerd	`167d27a`	link	false	`/test pull-kubernetes-e2e-capz-windows-containerd`
pull-kubernetes-e2e-gce-csi-serial	`167d27a`	link	false	`/test pull-kubernetes-e2e-gce-csi-serial`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

jsafrane changed the title ~~WIP: reconstruct SELinux mount~~ WIP: reconstruct SELinux mount label Nov 3, 2022

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 3, 2022

k8s-ci-robot requested review from andyzhangx and chrishenzie November 3, 2022 16:46

jsafrane force-pushed the selinux-reconstruction branch from 591650a to f2897ea Compare November 3, 2022 16:48

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 3, 2022

jsafrane force-pushed the selinux-reconstruction branch 2 times, most recently from ca66acb to cb52b67 Compare November 7, 2022 10:41

k8s-ci-robot assigned jingxu97 Nov 7, 2022

jsafrane force-pushed the selinux-reconstruction branch from cb52b67 to 0a34042 Compare November 7, 2022 19:45

gnufied reviewed Nov 7, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2022

jsafrane force-pushed the selinux-reconstruction branch from 0a34042 to 8d7a6bc Compare November 7, 2022 22:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 7, 2022

jsafrane changed the title ~~WIP: reconstruct SELinux mount label~~ Reconstruct SELinux mount label Nov 7, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2022

jsafrane added 2 commits November 8, 2022 11:17

Reconstruct SELinux mount option

e575e60

When reconstructing volumes from disk after kubelet restart, reconstruct also context=XYZ mount option and add it to the ActualStateOfWorld.

Update SELinux context log

cf912a2

SELinux context discovered from Pod is not final, it can be cleared when a volume plugin does not support SELinux or the volume is not ReadWriteOncePod. Update the existing log line + add a new one for easier debugging.

jsafrane force-pushed the selinux-reconstruction branch from 8d7a6bc to cf912a2 Compare November 8, 2022 10:18

jsafrane added 2 commits November 8, 2022 12:42

Add SELinux disruptive test

802979c

Add mock CSI driver test for SELinux mount

d6c3673

gnufied reviewed Nov 8, 2022

View reviewed changes

Save SELinux context both in MountDevice and SetUp

167d27a

And make it feature gated in both places.

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022

k8s-ci-robot merged commit da735b5 into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

gnufied mentioned this pull request Nov 8, 2022

Fix broken readwriteOncePod serial tests #113753

Merged

aojea mentioned this pull request Nov 11, 2022

[Failing test] gce-cos-master-serial #113849

Closed

jsafrane mentioned this pull request Nov 11, 2022

Fix subpath disruptive tests #113853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconstruct SELinux mount label #113596

Reconstruct SELinux mount label #113596

jsafrane commented Nov 3, 2022 •

edited

k8s-ci-robot commented Nov 3, 2022

jsafrane commented Nov 3, 2022

jsafrane commented Nov 4, 2022

xing-yang commented Nov 7, 2022

gnufied Nov 7, 2022

jsafrane Nov 8, 2022

gnufied Nov 7, 2022

jsafrane Nov 8, 2022

gnufied Nov 7, 2022

jsafrane Nov 8, 2022

jsafrane commented Nov 7, 2022

gnufied Nov 8, 2022

jsafrane Nov 8, 2022

gnufied Nov 8, 2022

jsafrane Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

gnufied commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

Reconstruct SELinux mount label #113596

Reconstruct SELinux mount label #113596

Conversation

jsafrane commented Nov 3, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Nov 3, 2022

jsafrane commented Nov 3, 2022

jsafrane commented Nov 4, 2022

xing-yang commented Nov 7, 2022

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

Choose a reason for hiding this comment

jsafrane commented Nov 7, 2022

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

k8s-ci-robot commented Nov 8, 2022

gnufied commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

jsafrane commented Nov 3, 2022 •

edited