Skip to content

Commit 7014033

Browse files
gnufiedopenshift-cherrypick-robot
gnufied
authored and
openshift-cherrypick-robot
committed
UPSTREAM: 138981: Cache selinux conflicts
Also prevent duplicate metric emissions
1 parent ca371c7 commit 7014033

4 files changed

Lines changed: 261 additions & 60 deletions

File tree

‎pkg/controller/volume/selinuxwarning/cache/volumecache.go‎

Lines changed: 46 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@ type VolumeCache interface {
4848
// change their SELinux support dynamically.
4949
GetPodsForCSIDriver(driverName string) []cache.ObjectName
5050

51-
// SendConflicts sends all current conflicts to the given channel.
52-
SendConflicts(logger klog.Logger, ch chan<- Conflict)
51+
// GetConflicts returns the current set of active conflicts (both directions).
52+
GetConflicts(logger klog.Logger) []Conflict
5353
}
5454

5555
// VolumeCache stores all volumes used by Pods and their properties that the controller needs to track,
@@ -62,6 +62,8 @@ type volumeCache struct {
6262
// Reverse index: maps each pod to the list of volumes it uses.
6363
// The index is used during pod deletion.
6464
podToVolumes map[cache.ObjectName]sets.Set[v1.UniqueVolumeName]
65+
// Currently active conflicts per volume (both directions, symmetric pairs).
66+
conflicts map[v1.UniqueVolumeName][]Conflict
6567
}
6668

6769
var _ VolumeCache = &volumeCache{}
@@ -72,6 +74,7 @@ func NewVolumeLabelCache(seLinuxTranslator *translator.ControllerSELinuxTranslat
7274
seLinuxTranslator: seLinuxTranslator,
7375
volumes: make(map[v1.UniqueVolumeName]usedVolume),
7476
podToVolumes: make(map[cache.ObjectName]sets.Set[v1.UniqueVolumeName]),
77+
conflicts: make(map[v1.UniqueVolumeName][]Conflict),
7578
}
7679
}
7780

@@ -164,6 +167,7 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa
164167
OtherPod: podKey,
165168
OtherPropertyValue: string(changePolicy),
166169
})
170+
167171
}
168172
if c.seLinuxTranslator.ConflictsParsed(otherPodInfo.seLinuxParts, podInfo.seLinuxParts) {
169173
// Send conflict to both pods
@@ -184,6 +188,21 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa
184188
})
185189
}
186190
}
191+
// Update the conflict cache for this volume: remove stale conflicts for this pod, then add new ones
192+
volumeConflicts := c.conflicts[volumeName]
193+
updated := make([]Conflict, 0, len(volumeConflicts))
194+
for _, existing := range volumeConflicts {
195+
if existing.Pod != podKey && existing.OtherPod != podKey {
196+
updated = append(updated, existing)
197+
}
198+
}
199+
updated = append(updated, conflicts...)
200+
if len(updated) == 0 {
201+
delete(c.conflicts, volumeName)
202+
} else {
203+
c.conflicts[volumeName] = updated
204+
}
205+
187206
return conflicts
188207
}
189208

@@ -193,6 +212,25 @@ func (c *volumeCache) DeletePod(logger klog.Logger, podKey cache.ObjectName) {
193212
defer c.mutex.Unlock()
194213
defer c.dump(logger)
195214

215+
for volumeName := range c.podToVolumes[podKey] {
216+
conflicts, found := c.conflicts[volumeName]
217+
if !found {
218+
continue
219+
}
220+
updated := make([]Conflict, 0, len(conflicts))
221+
for _, existing := range conflicts {
222+
// preserve other conflicts belonging to volume
223+
if existing.Pod != podKey && existing.OtherPod != podKey {
224+
updated = append(updated, existing)
225+
}
226+
}
227+
if len(updated) == 0 {
228+
delete(c.conflicts, volumeName)
229+
} else {
230+
c.conflicts[volumeName] = updated
231+
}
232+
}
233+
196234
// Use reverse index to only iterate through volumes this pod actually uses.
197235
for volumeName := range c.podToVolumes[podKey] {
198236
volume, found := c.volumes[volumeName]
@@ -283,42 +321,16 @@ func (c *volumeCache) GetPodsForCSIDriver(driverName string) []cache.ObjectName
283321
return pods
284322
}
285323

286-
// SendConflicts sends all current conflicts to the given channel.
287-
func (c *volumeCache) SendConflicts(logger klog.Logger, ch chan<- Conflict) {
324+
// GetConflicts returns the current set of active conflicts (both directions, symmetric pairs).
325+
func (c *volumeCache) GetConflicts(logger klog.Logger) []Conflict {
288326
c.mutex.RLock()
289327
defer c.mutex.RUnlock()
290328
logger.V(4).Info("Scraping conflicts")
291329
c.dump(logger)
292330

293-
for _, volume := range c.volumes {
294-
// compare pods that use the same volume with each other
295-
for podKey, podInfo := range volume.pods {
296-
for otherPodKey, otherPodInfo := range volume.pods {
297-
if podKey == otherPodKey {
298-
continue
299-
}
300-
// create conflict only for the first pod. The other pod will get the same conflict in its own iteration of `volume.pods` loop.
301-
if podInfo.changePolicy != otherPodInfo.changePolicy {
302-
ch <- Conflict{
303-
PropertyName: "SELinuxChangePolicy",
304-
EventReason: "SELinuxChangePolicyConflict",
305-
Pod: podKey,
306-
PropertyValue: string(podInfo.changePolicy),
307-
OtherPod: otherPodKey,
308-
OtherPropertyValue: string(otherPodInfo.changePolicy),
309-
}
310-
}
311-
if c.seLinuxTranslator.Conflicts(podInfo.seLinuxLabel, otherPodInfo.seLinuxLabel) {
312-
ch <- Conflict{
313-
PropertyName: "SELinuxLabel",
314-
EventReason: "SELinuxLabelConflict",
315-
Pod: podKey,
316-
PropertyValue: podInfo.seLinuxLabel,
317-
OtherPod: otherPodKey,
318-
OtherPropertyValue: otherPodInfo.seLinuxLabel,
319-
}
320-
}
321-
}
322-
}
331+
result := sets.New[Conflict]()
332+
for _, volConflicts := range c.conflicts {
333+
result.Insert(volConflicts...)
323334
}
335+
return result.UnsortedList()
324336
}

‎pkg/controller/volume/selinuxwarning/cache/volumecache_test.go‎

Lines changed: 210 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,8 @@ func addReverseConflict(conflicts []Conflict) []Conflict {
147147
return newConflicts
148148
}
149149

150-
// Test AddVolume and SendConflicts together, they both provide []conflict with the same data
151-
func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) {
150+
// Test that AddVolume and GetConflicts return the same []conflict data
151+
func TestVolumeCache_AddVolumeGetConflicts(t *testing.T) {
152152
existingPods := []podWithVolume{
153153
{
154154
podNamespace: "ns1",
@@ -488,27 +488,222 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) {
488488
// Verify reverse index consistency
489489
verifyReverseIndexConsistency(t, c)
490490

491-
// Act again: get the conflicts via SendConflicts
492-
ch := make(chan Conflict)
493-
go func() {
494-
c.SendConflicts(logger, ch)
495-
close(ch)
496-
}()
497-
498-
// Assert
499-
receivedConflicts := []Conflict{}
500-
for c := range ch {
501-
receivedConflicts = append(receivedConflicts, c)
502-
}
491+
// Verify that GetConflicts returns the same conflicts
492+
receivedConflicts := c.GetConflicts(logger)
503493
sortConflicts(receivedConflicts)
504494
if !reflect.DeepEqual(receivedConflicts, expectedConflicts) {
505-
t.Errorf("SendConflicts returned unexpected conflicts: %+v", receivedConflicts)
495+
t.Errorf("GetConflicts returned unexpected conflicts: %+v", receivedConflicts)
506496
c.dump(dumpLogger)
507497
}
508498
})
509499
}
510500
}
511501

502+
// Test that conflicts are tracked per-volume: a pod with conflicts on
503+
// multiple volumes retains all of them after successive AddVolume calls.
504+
func TestVolumeCache_MultiVolumeConflicts(t *testing.T) {
505+
logger, _ := getTestLoggers(t)
506+
seLinuxTranslator := &translator.ControllerSELinuxTranslator{}
507+
c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache)
508+
509+
podA := cache.ObjectName{Namespace: "ns", Name: "podA"}
510+
podB := cache.ObjectName{Namespace: "ns", Name: "podB"}
511+
podC := cache.ObjectName{Namespace: "ns", Name: "podC"}
512+
513+
// podB uses vol1 with label1
514+
c.AddVolume(logger, "vol1", podB, "system_u:system_r:labelB", v1.SELinuxChangePolicyMountOption, "driver1")
515+
// podC uses vol2 with label2
516+
c.AddVolume(logger, "vol2", podC, "system_u:system_r:labelC", v1.SELinuxChangePolicyMountOption, "driver1")
517+
518+
// podA uses vol1 with a different label (conflict with podB)
519+
conflicts1 := c.AddVolume(logger, "vol1", podA, "system_u:system_r:labelA", v1.SELinuxChangePolicyMountOption, "driver1")
520+
if len(conflicts1) == 0 {
521+
t.Fatal("Expected conflicts on vol1 between podA and podB")
522+
}
523+
524+
// podA also uses vol2 with a different label (conflict with podC)
525+
conflicts2 := c.AddVolume(logger, "vol2", podA, "system_u:system_r:labelA", v1.SELinuxChangePolicyMountOption, "driver1")
526+
if len(conflicts2) == 0 {
527+
t.Fatal("Expected conflicts on vol2 between podA and podC")
528+
}
529+
530+
// GetConflicts must return conflicts from BOTH volumes
531+
allConflicts := c.GetConflicts(logger)
532+
expectedCount := len(conflicts1) + len(conflicts2)
533+
if len(allConflicts) != expectedCount {
534+
t.Errorf("GetConflicts returned %d conflicts, expected %d (vol1: %d + vol2: %d)",
535+
len(allConflicts), expectedCount, len(conflicts1), len(conflicts2))
536+
}
537+
538+
// After deleting podA, all conflicts should be gone
539+
c.DeletePod(logger, podA)
540+
remaining := c.GetConflicts(logger)
541+
if len(remaining) != 0 {
542+
t.Errorf("Expected no conflicts after deleting podA, got %d: %+v", len(remaining), remaining)
543+
}
544+
545+
// Verify deduplication: podD and podE conflict on two volumes with the same labels.
546+
// Identical Conflict entries from different volumes must be deduplicated by GetConflicts.
547+
podD := cache.ObjectName{Namespace: "ns", Name: "podD"}
548+
podE := cache.ObjectName{Namespace: "ns", Name: "podE"}
549+
550+
c.AddVolume(logger, "vol3", podD, "system_u:system_r:labelD", v1.SELinuxChangePolicyMountOption, "driver1")
551+
c.AddVolume(logger, "vol4", podD, "system_u:system_r:labelD", v1.SELinuxChangePolicyMountOption, "driver1")
552+
553+
conflictsVol3 := c.AddVolume(logger, "vol3", podE, "system_u:system_r:labelE", v1.SELinuxChangePolicyMountOption, "driver1")
554+
conflictsVol4 := c.AddVolume(logger, "vol4", podE, "system_u:system_r:labelE", v1.SELinuxChangePolicyMountOption, "driver1")
555+
556+
if len(conflictsVol3) != len(conflictsVol4) {
557+
t.Fatalf("Expected same number of conflicts from vol3 and vol4 (%d vs %d)", len(conflictsVol3), len(conflictsVol4))
558+
}
559+
if len(conflictsVol3) == 0 {
560+
t.Fatal("Expected conflicts between podD and podE")
561+
}
562+
563+
allConflicts = c.GetConflicts(logger)
564+
deCount := 0
565+
for _, conflict := range allConflicts {
566+
if conflict.Pod == podD || conflict.Pod == podE || conflict.OtherPod == podD || conflict.OtherPod == podE {
567+
deCount++
568+
}
569+
}
570+
if deCount != len(conflictsVol3) {
571+
t.Errorf("Expected %d deduplicated conflicts for podD/podE (from 2 volumes), got %d", len(conflictsVol3), deCount)
572+
}
573+
}
574+
575+
func TestVolumeCache_DeletePodConflicts(t *testing.T) {
576+
podA := cache.ObjectName{Namespace: "ns", Name: "podA"}
577+
podB := cache.ObjectName{Namespace: "ns", Name: "podB"}
578+
podC := cache.ObjectName{Namespace: "ns", Name: "podC"}
579+
podD := cache.ObjectName{Namespace: "ns", Name: "podD"}
580+
581+
tests := []struct {
582+
name string
583+
// Pods to add before deletion.
584+
initialPods []podWithVolume
585+
// Pod to delete.
586+
podToDelete cache.ObjectName
587+
// If true, delete the pod a second time to verify idempotency.
588+
deleteTwice bool
589+
// Pod pairs that must still have symmetric conflicts after deletion.
590+
// Each pair [2]cache.ObjectName expects both (A→B) and (B→A) to be present.
591+
expectedSurvivingPairs [][2]cache.ObjectName
592+
}{
593+
{
594+
name: "delete one of two conflicting pods clears all conflicts",
595+
initialPods: []podWithVolume{
596+
{podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
597+
{podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption},
598+
},
599+
podToDelete: podA,
600+
expectedSurvivingPairs: nil,
601+
},
602+
{
603+
name: "delete non-conflicting pod preserves existing conflicts",
604+
initialPods: []podWithVolume{
605+
{podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
606+
{podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption},
607+
{podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption},
608+
},
609+
podToDelete: podC,
610+
expectedSurvivingPairs: [][2]cache.ObjectName{{podA, podB}},
611+
},
612+
{
613+
name: "three pods on same volume delete one leaves remaining pair conflict",
614+
initialPods: []podWithVolume{
615+
{podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
616+
{podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption},
617+
{podNamespace: "ns", podName: "podC", volumeName: "vol1", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption},
618+
},
619+
podToDelete: podA,
620+
expectedSurvivingPairs: [][2]cache.ObjectName{{podB, podC}},
621+
},
622+
{
623+
name: "delete pod with conflicts on multiple volumes",
624+
initialPods: []podWithVolume{
625+
{podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption},
626+
{podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption},
627+
{podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
628+
{podNamespace: "ns", podName: "podA", volumeName: "vol2", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
629+
},
630+
podToDelete: podA,
631+
expectedSurvivingPairs: nil,
632+
},
633+
{
634+
name: "delete pod preserves conflicts on unrelated volumes",
635+
initialPods: []podWithVolume{
636+
{podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
637+
{podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption},
638+
{podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption},
639+
{podNamespace: "ns", podName: "podD", volumeName: "vol2", label: "system_u:system_r:labelD", changePolicy: v1.SELinuxChangePolicyMountOption},
640+
},
641+
podToDelete: podA,
642+
expectedSurvivingPairs: [][2]cache.ObjectName{{podC, podD}},
643+
},
644+
{
645+
name: "delete pod that was already deleted is a no-op",
646+
initialPods: []podWithVolume{
647+
{podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption},
648+
{podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption},
649+
},
650+
podToDelete: podA,
651+
deleteTwice: true,
652+
expectedSurvivingPairs: nil,
653+
},
654+
}
655+
656+
for _, tt := range tests {
657+
t.Run(tt.name, func(t *testing.T) {
658+
logger, _ := getTestLoggers(t)
659+
seLinuxTranslator := &translator.ControllerSELinuxTranslator{}
660+
c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache)
661+
662+
for _, pod := range tt.initialPods {
663+
c.AddVolume(logger, pod.volumeName, cache.ObjectName{Namespace: pod.podNamespace, Name: pod.podName}, pod.label, pod.changePolicy, "driver1")
664+
}
665+
666+
c.DeletePod(logger, tt.podToDelete)
667+
if tt.deleteTwice {
668+
c.DeletePod(logger, tt.podToDelete)
669+
}
670+
671+
remaining := c.GetConflicts(logger)
672+
673+
// Deleted pod must not appear in any conflict
674+
for _, conflict := range remaining {
675+
if conflict.Pod == tt.podToDelete || conflict.OtherPod == tt.podToDelete {
676+
t.Errorf("found conflict involving deleted pod %s: %+v", tt.podToDelete, conflict)
677+
}
678+
}
679+
680+
// Verify each expected surviving pair exists in both directions
681+
for _, pair := range tt.expectedSurvivingPairs {
682+
hasForward := false
683+
hasReverse := false
684+
for _, conflict := range remaining {
685+
if conflict.Pod == pair[0] && conflict.OtherPod == pair[1] {
686+
hasForward = true
687+
}
688+
if conflict.Pod == pair[1] && conflict.OtherPod == pair[0] {
689+
hasReverse = true
690+
}
691+
}
692+
if !hasForward || !hasReverse {
693+
t.Errorf("expected symmetric conflict between %s and %s, got %+v", pair[0], pair[1], remaining)
694+
}
695+
}
696+
697+
// If no pairs are expected, there should be no conflicts at all
698+
if len(tt.expectedSurvivingPairs) == 0 && len(remaining) != 0 {
699+
t.Errorf("expected no conflicts, got %+v", remaining)
700+
}
701+
702+
verifyReverseIndexConsistency(t, c)
703+
})
704+
}
705+
}
706+
512707
func TestVolumeCache_GetPodsForCSIDriver(t *testing.T) {
513708
seLinuxTranslator := &translator.ControllerSELinuxTranslator{}
514709
c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache)

‎pkg/controller/volume/selinuxwarning/metrics.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,7 @@ func (c *collector) DescribeWithStability(ch chan<- *metrics.Desc) {
5959
}
6060

6161
func (c *collector) CollectWithStability(ch chan<- metrics.Metric) {
62-
conflictCh := make(chan cache.Conflict)
63-
go func() {
64-
c.cache.SendConflicts(c.logger, conflictCh)
65-
close(conflictCh)
66-
}()
67-
68-
for conflict := range conflictCh {
62+
for _, conflict := range c.cache.GetConflicts(c.logger) {
6963
ch <- metrics.NewLazyConstMetric(seLinuxConflictDesc,
7064
metrics.GaugeValue,
7165
1.0,

0 commit comments

Comments
 (0)