From 2f5903b4cf4ef0fc387438c8d4248b1110c80a53 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Thu, 12 Oct 2023 14:00:00 +0200 Subject: [PATCH 1/2] Move SELinux warning metric to be counted once per pod volume_manager_selinux_volume_context_mismatch_warnings_total should be counted only once per volume + pod. The previous location is evaluated periodically, so bump the metric only when a new pod is added to volume. --- .../cache/desired_state_of_world.go | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go index 4406b941a04..7c417516b24 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go @@ -306,7 +306,7 @@ func (dsw *desiredStateOfWorld) AddPodToVolume( } klog.V(4).InfoS("expected volume SELinux label context", "volume", volumeSpec.Name(), "label", seLinuxFileLabel) - if vol, volumeExists := dsw.volumesToMount[volumeName]; !volumeExists { + if _, volumeExists := dsw.volumesToMount[volumeName]; !volumeExists { var sizeLimit *resource.Quantity if volumeSpec.Volume != nil { if util.IsLocalEphemeralVolume(*volumeSpec.Volume) { @@ -350,12 +350,21 @@ func (dsw *desiredStateOfWorld) AddPodToVolume( } } dsw.volumesToMount[volumeName] = vmt - } else { - // volume exists + } + + oldPodMount, ok := dsw.volumesToMount[volumeName].podsToMount[podName] + mountRequestTime := time.Now() + if ok && !volumePlugin.RequiresRemount(volumeSpec) { + mountRequestTime = oldPodMount.mountRequestTime + } + + if !ok { + // The volume exists, but not with this pod. + // It will be added below as podToMount, now just report SELinux metric. if pluginSupportsSELinuxContextMount { - if seLinuxFileLabel != vol.originalSELinuxLabel { - // TODO: update the error message after tests, e.g. add at least the conflicting pod names. - fullErr := fmt.Errorf("conflicting SELinux labels of volume %s: %q and %q", volumeSpec.Name(), vol.originalSELinuxLabel, seLinuxFileLabel) + existingVolume := dsw.volumesToMount[volumeName] + if seLinuxFileLabel != existingVolume.originalSELinuxLabel { + fullErr := fmt.Errorf("conflicting SELinux labels of volume %s: %q and %q", volumeSpec.Name(), existingVolume.originalSELinuxLabel, seLinuxFileLabel) supported := util.VolumeSupportsSELinuxMount(volumeSpec) err := handleSELinuxMetricError( fullErr, @@ -369,12 +378,6 @@ func (dsw *desiredStateOfWorld) AddPodToVolume( } } - oldPodMount, ok := dsw.volumesToMount[volumeName].podsToMount[podName] - mountRequestTime := time.Now() - if ok && !volumePlugin.RequiresRemount(volumeSpec) { - mountRequestTime = oldPodMount.mountRequestTime - } - // Create new podToMount object. If it already exists, it is refreshed with // updated values (this is required for volumes that require remounting on // pod update, like Downward API volumes). From e511edf11f73d82c2d910e5bb5d11ae3916b31a8 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Thu, 12 Oct 2023 15:57:01 +0200 Subject: [PATCH 2/2] Fix SELinux unit tests Use device mountable volume, to make it impossible to share the same global mount with different SELinux contexts. And fix pod2Name to actually refer to pod2. --- .../cache/desired_state_of_world_test.go | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go index 86b0238b7a3..e465f9d6d4c 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go +++ b/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go @@ -612,10 +612,12 @@ func Test_AddPodToVolume_Positive_SELinuxNoRWOP(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SELinuxMountReadWriteOncePod, true)() // Arrange plugins := []volume.VolumePlugin{ - &volumetesting.FakeBasicVolumePlugin{ - Plugin: volumetesting.FakeVolumePlugin{ - PluginName: "basic", - SupportsSELinux: true, + &volumetesting.FakeDeviceMountableVolumePlugin{ + FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "basic", + SupportsSELinux: true, + }, }, }, } @@ -692,10 +694,12 @@ func Test_AddPodToVolume_Positive_NoSELinuxPlugin(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SELinuxMountReadWriteOncePod, true)() // Arrange plugins := []volume.VolumePlugin{ - &volumetesting.FakeBasicVolumePlugin{ - Plugin: volumetesting.FakeVolumePlugin{ - PluginName: "basic", - SupportsSELinux: false, + &volumetesting.FakeDeviceMountableVolumePlugin{ + FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "basic", + SupportsSELinux: false, + }, }, }, } @@ -773,10 +777,12 @@ func Test_AddPodToVolume_Positive_ExistingPodSameSELinuxRWOP(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SELinuxMountReadWriteOncePod, true)() // Arrange plugins := []volume.VolumePlugin{ - &volumetesting.FakeBasicVolumePlugin{ - Plugin: volumetesting.FakeVolumePlugin{ - PluginName: "basic", - SupportsSELinux: true, + &volumetesting.FakeDeviceMountableVolumePlugin{ + FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "basic", + SupportsSELinux: true, + }, }, }, } @@ -873,10 +879,12 @@ func Test_AddPodToVolume_Negative_ExistingPodDifferentSELinuxRWOP(t *testing.T) defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SELinuxMountReadWriteOncePod, true)() // Arrange plugins := []volume.VolumePlugin{ - &volumetesting.FakeBasicVolumePlugin{ - Plugin: volumetesting.FakeVolumePlugin{ - PluginName: "basic", - SupportsSELinux: true, + &volumetesting.FakeDeviceMountableVolumePlugin{ + FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "basic", + SupportsSELinux: true, + }, }, }, } @@ -957,7 +965,7 @@ func Test_AddPodToVolume_Negative_ExistingPodDifferentSELinuxRWOP(t *testing.T) pod2.Name = "pod2" pod2.UID = "pod2uid" pod2.Spec.SecurityContext.SELinuxOptions = &seLinux2 - pod2Name := util.GetUniquePodName(pod) + pod2Name := util.GetUniquePodName(pod2) // Act _, err = dsw.AddPodToVolume( @@ -967,7 +975,7 @@ func Test_AddPodToVolume_Negative_ExistingPodDifferentSELinuxRWOP(t *testing.T) t.Fatalf("Second AddPodToVolume succeeded, expected a failure") } // Verify the original SELinux context is still in DSW - verifyPodExistsInVolumeDsw(t, pod2Name, generatedVolumeName, "system_u:object_r:container_file_t:s0:c1,c2", dsw) + verifyPodExistsInVolumeDsw(t, podName, generatedVolumeName, "system_u:object_r:container_file_t:s0:c1,c2", dsw) } func verifyVolumeExistsDsw(