API-1894: Ignore updates related to Scheduling Gates

pkg/securitycontextconstraints/sccadmission/admission.go

@@ -81,7 +81,7 @@ func NewConstraint() *constraint {
 // any change that claims the pod is no longer privileged will be removed.  That should hold until
 // we get a true old/new set of objects in.
 func (c *constraint) Admit(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error {
-	if ignore, err := shouldIgnore(a); err != nil {
+	if ignore, err := shouldSkipSCCEvaluation(a); err != nil {
 		return err
 	} else if ignore {
 		return nil
@@ -131,7 +131,7 @@ func (c *constraint) Admit(ctx context.Context, a admission.Attributes, _ admiss
 }
 
 func (c *constraint) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error {
-	if ignore, err := shouldIgnore(a); err != nil {
+	if ignore, err := shouldSkipSCCEvaluation(a); err != nil {
 		return err
 	} else if ignore {
 		return nil
@@ -420,7 +420,14 @@ var ignoredAnnotations = sets.NewString(
 	"k8s.ovn.org/pod-networks",
 )
 
-func shouldIgnore(a admission.Attributes) (bool, error) {
+// shouldSkipSCCEvaluation skips evaluation for:
+// - non-Pod resources,
+// - specific subresources that don't affect Pod security context (e.g., exec, attach, log),
+// - Windows Pods (SCC is not applied),
+// - update operations that only change fields like SchedulingGates or non-critical metadata.
+// If the request is malformed (e.g., object can't be cast to a Pod), it fails closed to avoid
+// bypassing security enforcement unintentionally.
+func shouldSkipSCCEvaluation(a admission.Attributes) (bool, error) {
 	if a.GetResource().GroupResource() != coreapi.Resource("pods") {
 		return true, nil
 	}
@@ -448,24 +455,28 @@ func shouldIgnore(a admission.Attributes) (bool, error) {
 			return false, admission.NewForbidden(a, fmt.Errorf("object was marked as kind pod but was unable to be converted: %v", a.GetOldObject()))
 		}
 
-		// never ignore any spec changes
-		if !kapihelper.Semantic.DeepEqual(pod.Spec, oldPod.Spec) {
+		// Create deep copies to avoid mutating the original objects
+		podWithoutSchedulingGates := pod.DeepCopy()
+		// Skip SchedulingGates when comparing specs
+		podWithoutSchedulingGates.Spec.SchedulingGates = oldPod.Spec.SchedulingGates
+		if !kapihelper.Semantic.DeepEqual(podWithoutSchedulingGates.Spec, oldPod.Spec) {
 			return false, nil
 		}
 
 		// see if we are only doing meta changes that should be ignored during admission
 		// for example, the OVN controller adds informative networking annotations that shouldn't cause the pod to go through admission again
-		if shouldIgnoreMetaChanges(pod, oldPod) {
+		if shouldIgnoreMetaChanges(podWithoutSchedulingGates, oldPod) {
 			return true, nil
 		}
 	}
 
 	return false, nil
 }
 
-func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool {
+// newPodCopy is expected to be a copy of the original Pod that we can safely mutate
+func shouldIgnoreMetaChanges(newPodCopy, oldPod *coreapi.Pod) bool {
 	// check if we're adding or changing only annotations from the ignore list
-	for key, newVal := range newPod.ObjectMeta.Annotations {
+	for key, newVal := range newPodCopy.ObjectMeta.Annotations {
 		if oldVal, ok := oldPod.ObjectMeta.Annotations[key]; ok && newVal == oldVal {
 			continue
 		}
@@ -477,7 +488,7 @@ func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool {
 
 	// check if we're removing only annotations from the ignore list
 	for key := range oldPod.ObjectMeta.Annotations {
-		if _, ok := newPod.ObjectMeta.Annotations[key]; ok {
+		if _, ok := newPodCopy.ObjectMeta.Annotations[key]; ok {
 			continue
 		}
 
@@ -486,12 +497,11 @@ func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool {
 		}
 	}
 
-	newPodCopy := newPod.DeepCopyObject()
-	newPodCopyMeta, err := meta.Accessor(newPodCopy)
+	newPodCopyWithoutSchedulingGatesCopyMeta, err := meta.Accessor(newPodCopy)
 	if err != nil {
 		return false
 	}
-	newPodCopyMeta.SetAnnotations(oldPod.ObjectMeta.Annotations)
+	newPodCopyWithoutSchedulingGatesCopyMeta.SetAnnotations(oldPod.ObjectMeta.Annotations)
 
 	// see if we are only updating the ownerRef. Garbage collection does this
 	// and we should allow it in general, since you had the power to update and the power to delete.

pkg/securitycontextconstraints/sccadmission/admission_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/diff"
@@ -204,7 +205,7 @@ func TestAdmitCaps(t *testing.T) {
 	}
 }
 
-func TestShouldIgnore(t *testing.T) {
+func TestShouldSkipSCCEvaluation(t *testing.T) {
 	podCreationToAttributes := func(p *coreapi.Pod) admission.Attributes {
 		return admission.NewAttributesRecord(
 			p, nil,
@@ -268,6 +269,15 @@ func TestShouldIgnore(t *testing.T) {
 			shouldIgnore:        true,
 			admissionAttributes: withStatusUpdate(goodPod()),
 		},
+		{
+			description:  "schedulingGates updates should be ignored",
+			shouldIgnore: true,
+			admissionAttributes: withUpdate(schedulingGatePod(), "",
+				func(p *coreapi.Pod) *coreapi.Pod {
+					p.Spec.SchedulingGates = []coreapi.PodSchedulingGate{}
+					return p
+				}),
+		},
 		{
 			description:  "don't ignore normal updates",
 			shouldIgnore: false,
@@ -305,7 +315,7 @@ func TestShouldIgnore(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			ignored, err := shouldIgnore(test.admissionAttributes)
+			ignored, err := shouldSkipSCCEvaluation(test.admissionAttributes)
 			if err != nil {
 				t.Errorf("expected the test to not error but it errored with %v", err)
 			}
@@ -386,6 +396,95 @@ func TestShouldIgnoreMetaChanges(t *testing.T) {
 	}
 }
 
+func TestShouldSkipSCCEvaluationDoesNotMutateOriginalPod(t *testing.T) {
+	tests := []struct {
+		name       string
+		setupPod   func() *coreapi.Pod
+		setupAttrs func(pod *coreapi.Pod) admission.Attributes
+	}{
+		{
+			name: "scheduling gates update should not mutate original pod",
+			setupPod: func() *coreapi.Pod {
+				return schedulingGatePod()
+			},
+			setupAttrs: func(pod *coreapi.Pod) admission.Attributes {
+				oldPod := pod.DeepCopy()
+				updatedPod := pod.DeepCopy()
+				updatedPod.Spec.SchedulingGates = []coreapi.PodSchedulingGate{}
+
+				return admission.NewAttributesRecord(
+					updatedPod, oldPod,
+					coreapi.Kind("Pod").WithVersion("version"),
+					pod.Namespace, pod.Name,
+					coreapi.Resource("pods").WithVersion("version"),
+					"",
+					admission.Update,
+					nil,
+					false,
+					&user.DefaultInfo{},
+				)
+			},
+		},
+		{
+			name: "metadata changes should not mutate original pod",
+			setupPod: func() *coreapi.Pod {
+				pod := goodPod()
+				pod.Annotations = map[string]string{
+					"k8s.ovn.org/pod-networks": "original-value",
+					"other-annotation":         "other-value",
+				}
+				return pod
+			},
+			setupAttrs: func(pod *coreapi.Pod) admission.Attributes {
+				oldPod := pod.DeepCopy()
+				updatedPod := pod.DeepCopy()
+				updatedPod.Annotations["k8s.ovn.org/pod-networks"] = "updated-value"
+
+				return admission.NewAttributesRecord(
+					updatedPod, oldPod,
+					coreapi.Kind("Pod").WithVersion("version"),
+					pod.Namespace, pod.Name,
+					coreapi.Resource("pods").WithVersion("version"),
+					"",
+					admission.Update,
+					nil,
+					false,
+					&user.DefaultInfo{},
+				)
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			originalPod := test.setupPod()
+			originalPodCopy := originalPod.DeepCopy()
+			attrs := test.setupAttrs(originalPod)
+
+			// Call shouldSkipSCCEvaluation with the admission attributes
+			_, err := shouldSkipSCCEvaluation(attrs)
+			if err != nil {
+				t.Fatalf("shouldSkipSCCEvaluation returned unexpected error: %v", err)
+			}
+
+			// Verify that the original pod hasn't been mutated
+			if !apiequality.Semantic.DeepEqual(originalPod, originalPodCopy) {
+				t.Errorf("Original pod was mutated during shouldSkipSCCEvaluation call")
+				t.Errorf("Original: %+v", originalPodCopy)
+				t.Errorf("After call: %+v", originalPod)
+			}
+
+			// Also verify that the old pod in the attributes hasn't been mutated
+			oldPodFromAttrs := attrs.GetOldObject().(*coreapi.Pod)
+			if !apiequality.Semantic.DeepEqual(oldPodFromAttrs, originalPodCopy) {
+				t.Errorf("Old pod from admission attributes was mutated during shouldSkipSCCEvaluation call")
+				t.Errorf("Expected: %+v", originalPodCopy)
+				t.Errorf("Actual: %+v", oldPodFromAttrs)
+			}
+		})
+	}
+}
+
 func testSCCAdmit(testCaseName string, sccs []*securityv1.SecurityContextConstraints, pod *coreapi.Pod, shouldPass bool, t *testing.T) {
 	t.Helper()
 
@@ -2008,6 +2107,14 @@ func goodPod() *coreapi.Pod {
 	}
 }
 
+// schedulingGatePod is empty pod with scheduling gate. schedulingGates modifications
+// should be safely ignored.
+func schedulingGatePod() *coreapi.Pod {
+	p := goodPod()
+	p.Spec.SchedulingGates = []coreapi.PodSchedulingGate{{Name: "testGate"}}
+	return p
+}
+
 // windowsPod returns windows pod without any SCCs which are specific to Linux. The admission of Windows pod
 // should be safely ignored.
 func windowsPod() *coreapi.Pod {