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
Reconstruct SELinux mount label #113596
Reconstruct SELinux mount label #113596
Conversation
@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 The 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. |
591650a
to
f2897ea
Compare
/sig storage |
The e2e test needs some work, we can merge it after feature freeze. |
ca66acb
to
cb52b67
Compare
cb52b67
to
0a34042
Compare
@@ -183,6 +183,7 @@ func (rc *reconciler) markVolumeState(volume *reconstructedVolume, volumeState o | |||
VolumeGidVolume: volume.volumeGidValue, | |||
VolumeSpec: volume.volumeSpec, | |||
VolumeMountState: volumeState, | |||
SELinuxMountContext: volume.seLinuxMountContext, | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does non-feature gated code needs this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not, removed.
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have tests that verify if selinux information is recorded as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test added
pkg/volume/csi/csi_plugin.go
Outdated
klog.V(4).Info(log("plugin.ConstructVolumeSpec extracted [%#v]", volData)) | ||
|
||
var spec *volume.Spec | ||
ret := volume.ReconstructedVolume{ | ||
SELinuxMountContext: volData[volDataKey.seLinuxMountContext], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be feature gated as others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature gate added
0a34042
to
8d7a6bc
Compare
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.
8d7a6bc
to
cf912a2
Compare
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They create different jsons and I think SELinux is only in the SetUp one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: it was in MountDevice, I added it to both + both feature-gated.
And make it feature gated in both places.
[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:
Approvers can indicate their approval by writing |
/lgtm |
@jsafrane: The following tests failed, say
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. |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
kubernetes/enhancements#3548