From 9f52f4d26ccf8db47587b30eee7a0487ece9c164 Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Tue, 24 Feb 2026 11:53:18 +0100 Subject: [PATCH 1/3] controller/selinuxwarning: Pre-parse SELinux label When calling ControllerSELinuxTranslator.Conflicts(), the SELinux label is repeatedly split into []string to detect conflicts. This causes a huge number of allocations when there are many comparisons. This is now made more efficient by pre-parsing the SELinux label and storing it in podInfo as [4]string for fast comparison when needed. --- .../selinuxwarning/cache/volumecache.go | 7 +- .../selinuxwarning/cache/volumecache_test.go | 2 + .../internal/parse/selinux_label.go | 32 ++++++ .../internal/parse/selinux_label_test.go | 106 ++++++++++++++++++ .../translator/selinux_translator.go | 21 ++-- 5 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 pkg/controller/volume/selinuxwarning/internal/parse/selinux_label.go create mode 100644 pkg/controller/volume/selinuxwarning/internal/parse/selinux_label_test.go diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache.go b/pkg/controller/volume/selinuxwarning/cache/volumecache.go index 4b19c985c86..dba6c5b93de 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache.go @@ -23,6 +23,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/controller/volume/selinuxwarning/internal/parse" "k8s.io/kubernetes/pkg/controller/volume/selinuxwarning/translator" ) @@ -81,6 +82,8 @@ type podInfo struct { // SELinux seLinuxLabel to be applied to the volume in the Pod. // Either as mount option or recursively by the container runtime. seLinuxLabel string + // Pre-parsed SELinux label parts for fast conflict detection. + seLinuxParts [4]string // SELinuxChangePolicy of the Pod. changePolicy v1.PodSELinuxChangePolicy } @@ -89,6 +92,7 @@ func newPodInfoListForPod(podKey cache.ObjectName, seLinuxLabel string, changePo return map[cache.ObjectName]podInfo{ podKey: { seLinuxLabel: seLinuxLabel, + seLinuxParts: parse.ParseSELinuxLabel(seLinuxLabel), changePolicy: changePolicy, }, } @@ -116,6 +120,7 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa // The volume is already known podInfo := podInfo{ seLinuxLabel: label, + seLinuxParts: parse.ParseSELinuxLabel(label), changePolicy: changePolicy, } oldPodInfo, found := volume.pods[podKey] @@ -148,7 +153,7 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa OtherPropertyValue: string(changePolicy), }) } - if c.seLinuxTranslator.Conflicts(otherPodInfo.seLinuxLabel, label) { + if c.seLinuxTranslator.ConflictsParsed(otherPodInfo.seLinuxParts, podInfo.seLinuxParts) { // Send conflict to both pods conflicts = append(conflicts, Conflict{ PropertyName: "SELinuxLabel", diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go index 5bba301b692..71f916647b2 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" "k8s.io/klog/v2/ktesting" + "k8s.io/kubernetes/pkg/controller/volume/selinuxwarning/internal/parse" "k8s.io/kubernetes/pkg/controller/volume/selinuxwarning/translator" ) @@ -436,6 +437,7 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { } expectedPodInfo := podInfo{ seLinuxLabel: tt.podToAdd.label, + seLinuxParts: parse.ParseSELinuxLabel(tt.podToAdd.label), changePolicy: tt.podToAdd.changePolicy, } if !reflect.DeepEqual(existingInfo, expectedPodInfo) { diff --git a/pkg/controller/volume/selinuxwarning/internal/parse/selinux_label.go b/pkg/controller/volume/selinuxwarning/internal/parse/selinux_label.go new file mode 100644 index 00000000000..0fd48ed8b67 --- /dev/null +++ b/pkg/controller/volume/selinuxwarning/internal/parse/selinux_label.go @@ -0,0 +1,32 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package parse + +import "strings" + +// ParseSELinuxLabel parses a SELinux label string into its components. +// Format: "user:role:type:level" -> [user, role, type, level] +// Missing components are represented as empty strings. +func ParseSELinuxLabel(label string) [4]string { + var parts [4]string + if label == "" { + return parts + } + split := strings.SplitN(label, ":", 4) + copy(parts[:], split) + return parts +} diff --git a/pkg/controller/volume/selinuxwarning/internal/parse/selinux_label_test.go b/pkg/controller/volume/selinuxwarning/internal/parse/selinux_label_test.go new file mode 100644 index 00000000000..e82feed748f --- /dev/null +++ b/pkg/controller/volume/selinuxwarning/internal/parse/selinux_label_test.go @@ -0,0 +1,106 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package parse + +import ( + "reflect" + "testing" +) + +func TestParseSELinuxLabel(t *testing.T) { + tests := []struct { + name string + label string + expectedParts []string + }{ + { + name: "empty label", + label: "", + expectedParts: []string{"", "", "", ""}, + }, + { + name: "complete label with all components", + label: "system_u:system_r:container_t:s0:c0,c1", + expectedParts: []string{"system_u", "system_r", "container_t", "s0:c0,c1"}, + }, + { + name: "label with user, role, and type only", + label: "system_u:system_r:container_t", + expectedParts: []string{"system_u", "system_r", "container_t", ""}, + }, + { + name: "label with user and role only", + label: "system_u:system_r", + expectedParts: []string{"system_u", "system_r", "", ""}, + }, + { + name: "label with user only", + label: "system_u", + expectedParts: []string{"system_u", "", "", ""}, + }, + { + name: "label missing user but with role and type", + label: ":system_r:container_t", + expectedParts: []string{"", "system_r", "container_t", ""}, + }, + { + name: "label missing user and role but with type", + label: "::container_t", + expectedParts: []string{"", "", "container_t", ""}, + }, + { + name: "label missing user and role but with type and level", + label: "::container_t:s0", + expectedParts: []string{"", "", "container_t", "s0"}, + }, + { + name: "label with all empty components except level", + label: ":::s0:c0,c1", + expectedParts: []string{"", "", "", "s0:c0,c1"}, + }, + { + name: "label with special characters in components", + label: "user_with_underscore:role-with-dash:type.with.dots:s0:c0.c1", + expectedParts: []string{"user_with_underscore", "role-with-dash", "type.with.dots", "s0:c0.c1"}, + }, + { + name: "label with extra colons in level component", + label: "user:role:type:s0:c0,c1:extra", + expectedParts: []string{"user", "role", "type", "s0:c0,c1:extra"}, + }, + { + name: "multiple colons only", + label: ":::", + expectedParts: []string{"", "", "", ""}, + }, + { + name: "five colons", + label: ":::::", + expectedParts: []string{"", "", "", "::"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + parts := ParseSELinuxLabel(tt.label) + partsSlice := parts[:] + if !reflect.DeepEqual(partsSlice, tt.expectedParts) { + t.Errorf("ParseSELinuxLabel(%q) = %v, expected parts = %v", tt.label, partsSlice, tt.expectedParts) + } + }) + } +} diff --git a/pkg/controller/volume/selinuxwarning/translator/selinux_translator.go b/pkg/controller/volume/selinuxwarning/translator/selinux_translator.go index 99ce3e97dd7..db599c98cd7 100644 --- a/pkg/controller/volume/selinuxwarning/translator/selinux_translator.go +++ b/pkg/controller/volume/selinuxwarning/translator/selinux_translator.go @@ -20,6 +20,7 @@ import ( "strings" v1 "k8s.io/api/core/v1" + "k8s.io/kubernetes/pkg/controller/volume/selinuxwarning/internal/parse" "k8s.io/kubernetes/pkg/volume/util" ) @@ -70,18 +71,16 @@ func (c *ControllerSELinuxTranslator) SELinuxOptionsToFileLabel(opts *v1.SELinux // However: "system_u:system_r:container_t:s0:c1,c2" *does* conflict with ":::s0:c98,c99". // And ":::s0:c1,c2" *does* conflict with "" or ":::", because it's never defaulted by the OS. func (c *ControllerSELinuxTranslator) Conflicts(labelA, labelB string) bool { - partsA := strings.SplitN(labelA, ":", 4) - partsB := strings.SplitN(labelB, ":", 4) + return c.ConflictsParsed(parse.ParseSELinuxLabel(labelA), parse.ParseSELinuxLabel(labelB)) +} - // Reorder, so partsA is always longer than partsB - if len(partsA) < len(partsB) { - partsB, partsA = partsA, partsB - } - - for len(partsB) < len(partsA) { - partsB = append(partsB, "") - } - for i := range partsA { +// ConflictsParsed returns true if two pre-parsed SELinux labels conflict. +// This is an optimized version of Conflicts() that operates on pre-split labels +// to avoid repeated string allocations in hot paths (e.g., metrics collection). +// partsA and partsB must be 4-element arrays in the format: [user, role, type, level] +func (c *ControllerSELinuxTranslator) ConflictsParsed(partsA, partsB [4]string) bool { + // Compare each component + for i := range 4 { if partsA[i] == partsB[i] { continue } From fd088212052ffc4b94db03c76e8e10780e06ccda Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Tue, 24 Feb 2026 11:36:31 +0100 Subject: [PATCH 2/3] controller/selinuxwarning/cache: Add reverse index Added podToVolumes reverse index to optimize DeletePod. Currently we simply iterate through all the volumes and remove the pod being deleted from there. This is inefficient and takes longer the longer the volume list becomes. Keeping a map pod -> volumes makes removing a pod fast. We can just jump to the relevant volumes directly and remove the pod from there. --- .../selinuxwarning/cache/volumecache.go | 46 ++++++++++++++++++- .../selinuxwarning/cache/volumecache_test.go | 44 ++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache.go b/pkg/controller/volume/selinuxwarning/cache/volumecache.go index dba6c5b93de..521476b1c44 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache.go @@ -17,10 +17,12 @@ limitations under the License. package cache import ( + "slices" "sort" "sync" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/controller/volume/selinuxwarning/internal/parse" @@ -57,6 +59,9 @@ type volumeCache struct { seLinuxTranslator *translator.ControllerSELinuxTranslator // All volumes of all existing Pods. volumes map[v1.UniqueVolumeName]usedVolume + // Reverse index: maps each pod to the list of volumes it uses. + // The index is used during pod deletion. + podToVolumes map[cache.ObjectName]sets.Set[v1.UniqueVolumeName] } var _ VolumeCache = &volumeCache{} @@ -66,6 +71,7 @@ func NewVolumeLabelCache(seLinuxTranslator *translator.ControllerSELinuxTranslat return &volumeCache{ seLinuxTranslator: seLinuxTranslator, volumes: make(map[v1.UniqueVolumeName]usedVolume), + podToVolumes: make(map[cache.ObjectName]sets.Set[v1.UniqueVolumeName]), } } @@ -114,6 +120,9 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa pods: newPodInfoListForPod(podKey, label, changePolicy), } c.volumes[volumeName] = volume + + // Add to reverse index + c.registerPodVolume(podKey, volumeName) return conflicts } @@ -133,6 +142,9 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa // Add the updated pod info to the cache volume.pods[podKey] = podInfo + // Add to reverse index + c.registerPodVolume(podKey, volumeName) + // Emit conflicts for the pod for otherPodKey, otherPodInfo := range volume.pods { if otherPodInfo.changePolicy != changePolicy { @@ -181,12 +193,28 @@ func (c *volumeCache) DeletePod(logger klog.Logger, podKey cache.ObjectName) { defer c.mutex.Unlock() defer c.dump(logger) - for volumeName, volume := range c.volumes { + // Use reverse index to only iterate through volumes this pod actually uses. + for volumeName := range c.podToVolumes[podKey] { + volume, found := c.volumes[volumeName] + if !found { + continue + } delete(volume.pods, podKey) if len(volume.pods) == 0 { delete(c.volumes, volumeName) } } + delete(c.podToVolumes, podKey) +} + +// registerPodVolume adds volumeName to the pod volume index. +// Make sure to hold c.mutex when calling this function. +func (c *volumeCache) registerPodVolume(podKey cache.ObjectName, volumeName v1.UniqueVolumeName) { + if podVolumes, ok := c.podToVolumes[podKey]; ok { + podVolumes.Insert(volumeName) + } else { + c.podToVolumes[podKey] = sets.New(volumeName) + } } func (c *volumeCache) dump(logger klog.Logger) { @@ -220,6 +248,22 @@ func (c *volumeCache) dump(logger klog.Logger) { logger.Info(" pod", "pod", podKey, "seLinuxLabel", podInfo.seLinuxLabel, "changePolicy", podInfo.changePolicy) } } + + // Collect all pods, sort them and print the associated volumes. + podKeys := make([]cache.ObjectName, 0, len(c.podToVolumes)) + for podKey := range c.podToVolumes { + podKeys = append(podKeys, podKey) + } + sort.Slice(podKeys, func(i, j int) bool { + return podKeys[i].String() < podKeys[j].String() + }) + + logger.Info("VolumeCache reverse index dump:") + for _, podKey := range podKeys { + podVolumes := sets.List(c.podToVolumes[podKey]) + slices.Sort(podVolumes) + logger.Info(" pod", "pod", podKey, "volumes", podVolumes) + } } // GetPodsForCSIDriver returns all pods that use volumes with the given CSI driver. diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go index 71f916647b2..b1121cd2834 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go @@ -46,6 +46,39 @@ func sortConflicts(conflicts []Conflict) { }) } +// verifyReverseIndexConsistency checks that forward and reverse indexes are symmetric +func verifyReverseIndexConsistency(t *testing.T, c *volumeCache) { + t.Helper() + + // For every (pod, volume) in reverse index, verify it exists in forward index. + for podKey, volumes := range c.podToVolumes { + for volumeName := range volumes { + volume, found := c.volumes[volumeName] + if !found { + t.Errorf("Reverse index has pod %s -> volume %s, but volume not in forward index", podKey, volumeName) + continue + } + if _, found := volume.pods[podKey]; !found { + t.Errorf("Reverse index has pod %s -> volume %s, but pod not in volume's pod list", podKey, volumeName) + } + } + } + + // For every (volume, pod) in forward index, verify it exists in reverse index. + for volumeName, volume := range c.volumes { + for podKey := range volume.pods { + podVolumes, found := c.podToVolumes[podKey] + if !found { + t.Errorf("Forward index has volume %s -> pod %s, but pod not in reverse index", volumeName, podKey) + continue + } + if _, found := podVolumes[volumeName]; !found { + t.Errorf("Forward index has volume %s -> pod %s, but volume not in pod's volume list", volumeName, podKey) + } + } + } +} + // Delete all items in a bigger cache and check it's empty func TestVolumeCache_DeleteAll(t *testing.T) { var podsToDelete []cache.ObjectName @@ -70,6 +103,8 @@ func TestVolumeCache_DeleteAll(t *testing.T) { t.Log("Before deleting all pods:") c.dump(dumpLogger) + verifyReverseIndexConsistency(t, c) + // Act: delete all pods for _, podKey := range podsToDelete { c.DeletePod(logger, podKey) @@ -80,6 +115,12 @@ func TestVolumeCache_DeleteAll(t *testing.T) { t.Errorf("Expected cache to be empty, got %d volumes", len(c.volumes)) c.dump(dumpLogger) } + + // Assert: the reverse index is also empty + if len(c.podToVolumes) != 0 { + t.Errorf("Expected reverse index to be empty, got %d pods", len(c.podToVolumes)) + } + verifyReverseIndexConsistency(t, c) } type podWithVolume struct { @@ -444,6 +485,9 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { t.Errorf("pod %s has unexpected info: %+v", podKey, existingInfo) } + // Verify reverse index consistency + verifyReverseIndexConsistency(t, c) + // Act again: get the conflicts via SendConflicts ch := make(chan Conflict) go func() { From 4a0532be209e6a56c6369a2f988fc9f0ae9e5f76 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 11 May 2026 17:34:32 -0400 Subject: [PATCH 3/3] Cache selinux conflicts Also prevent duplicate metric emissions --- .../selinuxwarning/cache/volumecache.go | 80 ++++--- .../selinuxwarning/cache/volumecache_test.go | 223 ++++++++++++++++-- .../volume/selinuxwarning/metrics.go | 8 +- .../selinux_warning_controller_test.go | 8 +- 4 files changed, 260 insertions(+), 59 deletions(-) diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache.go b/pkg/controller/volume/selinuxwarning/cache/volumecache.go index 521476b1c44..ebd756584c0 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache.go @@ -48,8 +48,8 @@ type VolumeCache interface { // change their SELinux support dynamically. GetPodsForCSIDriver(driverName string) []cache.ObjectName - // SendConflicts sends all current conflicts to the given channel. - SendConflicts(logger klog.Logger, ch chan<- Conflict) + // GetConflicts returns the current set of active conflicts (both directions). + GetConflicts(logger klog.Logger) []Conflict } // VolumeCache stores all volumes used by Pods and their properties that the controller needs to track, @@ -62,6 +62,8 @@ type volumeCache struct { // Reverse index: maps each pod to the list of volumes it uses. // The index is used during pod deletion. podToVolumes map[cache.ObjectName]sets.Set[v1.UniqueVolumeName] + // Currently active conflicts per volume (both directions, symmetric pairs). + conflicts map[v1.UniqueVolumeName][]Conflict } var _ VolumeCache = &volumeCache{} @@ -72,6 +74,7 @@ func NewVolumeLabelCache(seLinuxTranslator *translator.ControllerSELinuxTranslat seLinuxTranslator: seLinuxTranslator, volumes: make(map[v1.UniqueVolumeName]usedVolume), podToVolumes: make(map[cache.ObjectName]sets.Set[v1.UniqueVolumeName]), + conflicts: make(map[v1.UniqueVolumeName][]Conflict), } } @@ -164,6 +167,7 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa OtherPod: podKey, OtherPropertyValue: string(changePolicy), }) + } if c.seLinuxTranslator.ConflictsParsed(otherPodInfo.seLinuxParts, podInfo.seLinuxParts) { // Send conflict to both pods @@ -184,6 +188,21 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa }) } } + // Update the conflict cache for this volume: remove stale conflicts for this pod, then add new ones + volumeConflicts := c.conflicts[volumeName] + updated := make([]Conflict, 0, len(volumeConflicts)) + for _, existing := range volumeConflicts { + if existing.Pod != podKey && existing.OtherPod != podKey { + updated = append(updated, existing) + } + } + updated = append(updated, conflicts...) + if len(updated) == 0 { + delete(c.conflicts, volumeName) + } else { + c.conflicts[volumeName] = updated + } + return conflicts } @@ -193,6 +212,25 @@ func (c *volumeCache) DeletePod(logger klog.Logger, podKey cache.ObjectName) { defer c.mutex.Unlock() defer c.dump(logger) + for volumeName := range c.podToVolumes[podKey] { + conflicts, found := c.conflicts[volumeName] + if !found { + continue + } + updated := make([]Conflict, 0, len(conflicts)) + for _, existing := range conflicts { + // preserve other conflicts belonging to volume + if existing.Pod != podKey && existing.OtherPod != podKey { + updated = append(updated, existing) + } + } + if len(updated) == 0 { + delete(c.conflicts, volumeName) + } else { + c.conflicts[volumeName] = updated + } + } + // Use reverse index to only iterate through volumes this pod actually uses. for volumeName := range c.podToVolumes[podKey] { volume, found := c.volumes[volumeName] @@ -283,42 +321,16 @@ func (c *volumeCache) GetPodsForCSIDriver(driverName string) []cache.ObjectName return pods } -// SendConflicts sends all current conflicts to the given channel. -func (c *volumeCache) SendConflicts(logger klog.Logger, ch chan<- Conflict) { +// GetConflicts returns the current set of active conflicts (both directions, symmetric pairs). +func (c *volumeCache) GetConflicts(logger klog.Logger) []Conflict { c.mutex.RLock() defer c.mutex.RUnlock() logger.V(4).Info("Scraping conflicts") c.dump(logger) - for _, volume := range c.volumes { - // compare pods that use the same volume with each other - for podKey, podInfo := range volume.pods { - for otherPodKey, otherPodInfo := range volume.pods { - if podKey == otherPodKey { - continue - } - // create conflict only for the first pod. The other pod will get the same conflict in its own iteration of `volume.pods` loop. - if podInfo.changePolicy != otherPodInfo.changePolicy { - ch <- Conflict{ - PropertyName: "SELinuxChangePolicy", - EventReason: "SELinuxChangePolicyConflict", - Pod: podKey, - PropertyValue: string(podInfo.changePolicy), - OtherPod: otherPodKey, - OtherPropertyValue: string(otherPodInfo.changePolicy), - } - } - if c.seLinuxTranslator.Conflicts(podInfo.seLinuxLabel, otherPodInfo.seLinuxLabel) { - ch <- Conflict{ - PropertyName: "SELinuxLabel", - EventReason: "SELinuxLabelConflict", - Pod: podKey, - PropertyValue: podInfo.seLinuxLabel, - OtherPod: otherPodKey, - OtherPropertyValue: otherPodInfo.seLinuxLabel, - } - } - } - } + result := sets.New[Conflict]() + for _, volConflicts := range c.conflicts { + result.Insert(volConflicts...) } + return result.UnsortedList() } diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go index b1121cd2834..82fd27e815f 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go @@ -147,8 +147,8 @@ func addReverseConflict(conflicts []Conflict) []Conflict { return newConflicts } -// Test AddVolume and SendConflicts together, they both provide []conflict with the same data -func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { +// Test that AddVolume and GetConflicts return the same []conflict data +func TestVolumeCache_AddVolumeGetConflicts(t *testing.T) { existingPods := []podWithVolume{ { podNamespace: "ns1", @@ -488,18 +488,8 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { // Verify reverse index consistency verifyReverseIndexConsistency(t, c) - // Act again: get the conflicts via SendConflicts - ch := make(chan Conflict) - go func() { - c.SendConflicts(logger, ch) - close(ch) - }() - - // Assert - receivedConflicts := []Conflict{} - for c := range ch { - receivedConflicts = append(receivedConflicts, c) - } + // Verify that GetConflicts returns the same conflicts + receivedConflicts := c.GetConflicts(logger) sortConflicts(receivedConflicts) if !reflect.DeepEqual(receivedConflicts, expectedConflicts) { t.Errorf("SendConflicts returned unexpected conflicts: %+v", receivedConflicts) @@ -509,6 +499,211 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { } } +// Test that conflicts are tracked per-volume: a pod with conflicts on +// multiple volumes retains all of them after successive AddVolume calls. +func TestVolumeCache_MultiVolumeConflicts(t *testing.T) { + logger, _ := getTestLoggers(t) + seLinuxTranslator := &translator.ControllerSELinuxTranslator{} + c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache) + + podA := cache.ObjectName{Namespace: "ns", Name: "podA"} + podB := cache.ObjectName{Namespace: "ns", Name: "podB"} + podC := cache.ObjectName{Namespace: "ns", Name: "podC"} + + // podB uses vol1 with label1 + c.AddVolume(logger, "vol1", podB, "system_u:system_r:labelB", v1.SELinuxChangePolicyMountOption, "driver1") + // podC uses vol2 with label2 + c.AddVolume(logger, "vol2", podC, "system_u:system_r:labelC", v1.SELinuxChangePolicyMountOption, "driver1") + + // podA uses vol1 with a different label (conflict with podB) + conflicts1 := c.AddVolume(logger, "vol1", podA, "system_u:system_r:labelA", v1.SELinuxChangePolicyMountOption, "driver1") + if len(conflicts1) == 0 { + t.Fatal("Expected conflicts on vol1 between podA and podB") + } + + // podA also uses vol2 with a different label (conflict with podC) + conflicts2 := c.AddVolume(logger, "vol2", podA, "system_u:system_r:labelA", v1.SELinuxChangePolicyMountOption, "driver1") + if len(conflicts2) == 0 { + t.Fatal("Expected conflicts on vol2 between podA and podC") + } + + // GetConflicts must return conflicts from BOTH volumes + allConflicts := c.GetConflicts(logger) + expectedCount := len(conflicts1) + len(conflicts2) + if len(allConflicts) != expectedCount { + t.Errorf("GetConflicts returned %d conflicts, expected %d (vol1: %d + vol2: %d)", + len(allConflicts), expectedCount, len(conflicts1), len(conflicts2)) + } + + // After deleting podA, all conflicts should be gone + c.DeletePod(logger, podA) + remaining := c.GetConflicts(logger) + if len(remaining) != 0 { + t.Errorf("Expected no conflicts after deleting podA, got %d: %+v", len(remaining), remaining) + } + + // Verify deduplication: podD and podE conflict on two volumes with the same labels. + // Identical Conflict entries from different volumes must be deduplicated by GetConflicts. + podD := cache.ObjectName{Namespace: "ns", Name: "podD"} + podE := cache.ObjectName{Namespace: "ns", Name: "podE"} + + c.AddVolume(logger, "vol3", podD, "system_u:system_r:labelD", v1.SELinuxChangePolicyMountOption, "driver1") + c.AddVolume(logger, "vol4", podD, "system_u:system_r:labelD", v1.SELinuxChangePolicyMountOption, "driver1") + + conflictsVol3 := c.AddVolume(logger, "vol3", podE, "system_u:system_r:labelE", v1.SELinuxChangePolicyMountOption, "driver1") + conflictsVol4 := c.AddVolume(logger, "vol4", podE, "system_u:system_r:labelE", v1.SELinuxChangePolicyMountOption, "driver1") + + if len(conflictsVol3) != len(conflictsVol4) { + t.Fatalf("Expected same number of conflicts from vol3 and vol4 (%d vs %d)", len(conflictsVol3), len(conflictsVol4)) + } + if len(conflictsVol3) == 0 { + t.Fatal("Expected conflicts between podD and podE") + } + + allConflicts = c.GetConflicts(logger) + deCount := 0 + for _, conflict := range allConflicts { + if conflict.Pod == podD || conflict.Pod == podE || conflict.OtherPod == podD || conflict.OtherPod == podE { + deCount++ + } + } + if deCount != len(conflictsVol3) { + t.Errorf("Expected %d deduplicated conflicts for podD/podE (from 2 volumes), got %d", len(conflictsVol3), deCount) + } +} + +func TestVolumeCache_DeletePodConflicts(t *testing.T) { + podA := cache.ObjectName{Namespace: "ns", Name: "podA"} + podB := cache.ObjectName{Namespace: "ns", Name: "podB"} + podC := cache.ObjectName{Namespace: "ns", Name: "podC"} + podD := cache.ObjectName{Namespace: "ns", Name: "podD"} + + tests := []struct { + name string + // Pods to add before deletion. + initialPods []podWithVolume + // Pod to delete. + podToDelete cache.ObjectName + // If true, delete the pod a second time to verify idempotency. + deleteTwice bool + // Pod pairs that must still have symmetric conflicts after deletion. + // Each pair [2]cache.ObjectName expects both (A→B) and (B→A) to be present. + expectedSurvivingPairs [][2]cache.ObjectName + }{ + { + name: "delete one of two conflicting pods clears all conflicts", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: nil, + }, + { + name: "delete non-conflicting pod preserves existing conflicts", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podC, + expectedSurvivingPairs: [][2]cache.ObjectName{{podA, podB}}, + }, + { + name: "three pods on same volume delete one leaves remaining pair conflict", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol1", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: [][2]cache.ObjectName{{podB, podC}}, + }, + { + name: "delete pod with conflicts on multiple volumes", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podA", volumeName: "vol2", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: nil, + }, + { + name: "delete pod preserves conflicts on unrelated volumes", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podD", volumeName: "vol2", label: "system_u:system_r:labelD", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: [][2]cache.ObjectName{{podC, podD}}, + }, + { + name: "delete pod that was already deleted is a no-op", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + deleteTwice: true, + expectedSurvivingPairs: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger, _ := getTestLoggers(t) + seLinuxTranslator := &translator.ControllerSELinuxTranslator{} + c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache) + + for _, pod := range tt.initialPods { + c.AddVolume(logger, pod.volumeName, cache.ObjectName{Namespace: pod.podNamespace, Name: pod.podName}, pod.label, pod.changePolicy, "driver1") + } + + c.DeletePod(logger, tt.podToDelete) + if tt.deleteTwice { + c.DeletePod(logger, tt.podToDelete) + } + + remaining := c.GetConflicts(logger) + + // Deleted pod must not appear in any conflict + for _, conflict := range remaining { + if conflict.Pod == tt.podToDelete || conflict.OtherPod == tt.podToDelete { + t.Errorf("found conflict involving deleted pod %s: %+v", tt.podToDelete, conflict) + } + } + + // Verify each expected surviving pair exists in both directions + for _, pair := range tt.expectedSurvivingPairs { + hasForward := false + hasReverse := false + for _, conflict := range remaining { + if conflict.Pod == pair[0] && conflict.OtherPod == pair[1] { + hasForward = true + } + if conflict.Pod == pair[1] && conflict.OtherPod == pair[0] { + hasReverse = true + } + } + if !hasForward || !hasReverse { + t.Errorf("expected symmetric conflict between %s and %s, got %+v", pair[0], pair[1], remaining) + } + } + + // If no pairs are expected, there should be no conflicts at all + if len(tt.expectedSurvivingPairs) == 0 && len(remaining) != 0 { + t.Errorf("expected no conflicts, got %+v", remaining) + } + + verifyReverseIndexConsistency(t, c) + }) + } +} + func TestVolumeCache_GetPodsForCSIDriver(t *testing.T) { seLinuxTranslator := &translator.ControllerSELinuxTranslator{} c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache) diff --git a/pkg/controller/volume/selinuxwarning/metrics.go b/pkg/controller/volume/selinuxwarning/metrics.go index d95665c9162..c285bd78db4 100644 --- a/pkg/controller/volume/selinuxwarning/metrics.go +++ b/pkg/controller/volume/selinuxwarning/metrics.go @@ -59,13 +59,7 @@ func (c *collector) DescribeWithStability(ch chan<- *metrics.Desc) { } func (c *collector) CollectWithStability(ch chan<- metrics.Metric) { - conflictCh := make(chan cache.Conflict) - go func() { - c.cache.SendConflicts(c.logger, conflictCh) - close(conflictCh) - }() - - for conflict := range conflictCh { + for _, conflict := range c.cache.GetConflicts(c.logger) { ch <- metrics.NewLazyConstMetric(seLinuxConflictDesc, metrics.GaugeValue, 1.0, diff --git a/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go b/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go index 9d9998bc62a..1d2e66f4e40 100644 --- a/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go +++ b/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go @@ -783,12 +783,12 @@ func (f *fakeVolumeCache) GetPodsForCSIDriver(driverName string) []cache.ObjectN return pods } -func (f *fakeVolumeCache) SendConflicts(logger klog.Logger, ch chan<- volumecache.Conflict) { +func (f *fakeVolumeCache) GetConflicts(logger klog.Logger) []volumecache.Conflict { + result := make([]volumecache.Conflict, 0) for _, conflicts := range f.conflictsToSend { - for _, conflict := range conflicts { - ch <- conflict - } + result = append(result, conflicts...) } + return result } func collectEvents(source <-chan string) []string {