From 07c3a156b21067a209293cc8615441e7275d4d2c Mon Sep 17 00:00:00 2001 From: bmordeha Date: Mon, 25 Dec 2023 17:14:35 +0200 Subject: [PATCH] Ignore updates related to Scheduling Gates to allow the installation of external operators that manage pod scheduling. Scheduling Gates don't affect pod privileges, so there's no need to block them through SCC admission. Signed-off-by: bmordeha --- .../sccadmission/admission.go | 34 ++++-- .../sccadmission/admission_test.go | 111 +++++++++++++++++- 2 files changed, 131 insertions(+), 14 deletions(-) diff --git a/pkg/securitycontextconstraints/sccadmission/admission.go b/pkg/securitycontextconstraints/sccadmission/admission.go index 02ceaacd4..1a17b1802 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission.go +++ b/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,14 +455,17 @@ 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 } } @@ -463,9 +473,10 @@ func shouldIgnore(a admission.Attributes) (bool, error) { 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. diff --git a/pkg/securitycontextconstraints/sccadmission/admission_test.go b/pkg/securitycontextconstraints/sccadmission/admission_test.go index 4d7b1fe2e..daa8253a3 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission_test.go +++ b/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 {