From 2a2758d14e51adf9c99983de4282127394398c62 Mon Sep 17 00:00:00 2001 From: Akshit Grover Date: Wed, 27 Oct 2021 20:38:14 +0530 Subject: [PATCH 1/2] PodSecurity: prioritize unique pods over replicated pods when validating a namespace --- .../admission/admission.go | 35 ++++++++++- .../admission/admission_test.go | 61 +++++++++++++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index 1bb3a9b7af7..467c9fc0e5c 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -24,6 +24,8 @@ import ( "sort" "time" + "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" admissionv1 "k8s.io/api/admission/v1" @@ -522,18 +524,23 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin podWarnings []string podWarningsToCount = make(map[string]podCount) + prioritisedPods = a.prioritisePods(pods) ) + totalPods := len(pods) if len(pods) > a.namespaceMaxPodsToCheck { - pods = pods[0:a.namespaceMaxPodsToCheck] + prioritisedPods = prioritisedPods[0:a.namespaceMaxPodsToCheck] } checkedPods := len(pods) - for i, pod := range pods { + for i, pod := range prioritisedPods { + checkedPods = i + 1 + // short-circuit on exempt runtimeclass if a.exemptRuntimeClass(pod.Spec.RuntimeClassName) { continue } + r := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(enforce, &pod.ObjectMeta, &pod.Spec)) if !r.Allowed { warning := r.ForbiddenReason() @@ -548,7 +555,6 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin podWarningsToCount[warning] = c } if err := ctx.Err(); err != nil { // deadline exceeded or context was cancelled - checkedPods = i + 1 break } } @@ -731,6 +737,29 @@ func (a *Admission) exemptRuntimeClass(runtimeClass *string) bool { // TODO: consider optimizing to O(1) lookup return containsString(*runtimeClass, a.Configuration.Exemptions.RuntimeClasses) } + +// Filter and prioritise pods based on runtimeclass and uniqueness of the controller respectively for evaluation +func (a *Admission) prioritisePods(pods []*corev1.Pod) []*corev1.Pod { + var replicatedPods []*corev1.Pod + var prioritisedPods []*corev1.Pod + evaluatedControllers := make(map[types.UID]bool) + for _, pod := range pods { + // short-circuit if pod from the same controller is evaluated + podOwnerControllerRef := metav1.GetControllerOfNoCopy(pod) + if podOwnerControllerRef == nil { + prioritisedPods = append(prioritisedPods, pod) + continue + } + if evaluatedControllers[podOwnerControllerRef.UID] { + replicatedPods = append(replicatedPods, pod) + continue + } + prioritisedPods = append(prioritisedPods, pod) + evaluatedControllers[podOwnerControllerRef.UID] = true + } + return append(prioritisedPods, replicatedPods...) +} + func containsString(needle string, haystack []string) bool { for _, s := range haystack { if s == needle { diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index f0f4f027e25..e1c1e03fcd3 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -18,11 +18,15 @@ package admission import ( "context" + "math/rand" "reflect" "strings" "testing" "time" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" + "github.com/stretchr/testify/assert" admissionv1 "k8s.io/api/admission/v1" @@ -765,3 +769,60 @@ func (r *FakeRecorder) ExpectEvaluations(t *testing.T, expected []EvaluationReco t.Helper() assert.ElementsMatch(t, expected, r.evaluations) } + +func TestPrioritisePods(t *testing.T) { + isController := true + sampleOwnerReferences := []struct { + ownerRefs []metav1.OwnerReference + }{ + { + ownerRefs: []metav1.OwnerReference{ + { + UID: uuid.NewUUID(), + Controller: &isController, + }, + }, + }, { + ownerRefs: []metav1.OwnerReference{ + { + UID: uuid.NewUUID(), + Controller: &isController, + }, + }, + }, { + ownerRefs: []metav1.OwnerReference{ + { + UID: uuid.NewUUID(), + Controller: &isController, + }, + }, + }, + } + + var pods []*corev1.Pod + randomSource := rand.NewSource(time.Now().Unix()) + for _, sampleOwnerRef := range sampleOwnerReferences { + // Generate multiple pods for a controller + for i := 0; i < rand.New(randomSource).Intn(5)+len(sampleOwnerReferences); i++ { + pods = append(pods, &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: sampleOwnerRef.ownerRefs, + }, + Spec: corev1.PodSpec{}, + }) + } + } + a := &Admission{} + prioritisedPods := a.prioritisePods(pods) + controllerRef := make(map[types.UID]bool) + + for i := 0; i < len(sampleOwnerReferences); i++ { + if controllerRef[metav1.GetControllerOfNoCopy(prioritisedPods[i]).UID] { + assert.Fail(t, "Pods are not prioritised based on uniqueness of the controller") + } + controllerRef[metav1.GetControllerOfNoCopy(prioritisedPods[i]).UID] = true + } + if len(prioritisedPods) != len(pods) { + assert.Fail(t, "Pod count is not the same after prioritization") + } +} \ No newline at end of file From 34463dc71a8decee6ced847b5d48ad1ca75c0c72 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 2 Nov 2021 13:29:04 -0400 Subject: [PATCH 2/2] PodSecurity: update pod prioritization to skip exempt pods, add unit tests --- .../admission/admission.go | 46 +++++++------- .../admission/admission_test.go | 60 +++++++++++++++---- 2 files changed, 71 insertions(+), 35 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index 467c9fc0e5c..18a2086626f 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -24,8 +24,6 @@ import ( "sort" "time" - "k8s.io/apimachinery/pkg/types" - "k8s.io/klog/v2" admissionv1 "k8s.io/api/admission/v1" @@ -36,6 +34,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" admissionapi "k8s.io/pod-security-admission/admission/api" "k8s.io/pod-security-admission/admission/api/validation" @@ -524,23 +523,16 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin podWarnings []string podWarningsToCount = make(map[string]podCount) - prioritisedPods = a.prioritisePods(pods) + prioritizedPods = a.prioritizePods(pods) ) - totalPods := len(pods) - if len(pods) > a.namespaceMaxPodsToCheck { - prioritisedPods = prioritisedPods[0:a.namespaceMaxPodsToCheck] + totalPods := len(prioritizedPods) + if len(prioritizedPods) > a.namespaceMaxPodsToCheck { + prioritizedPods = prioritizedPods[0:a.namespaceMaxPodsToCheck] } - checkedPods := len(pods) - for i, pod := range prioritisedPods { - checkedPods = i + 1 - - // short-circuit on exempt runtimeclass - if a.exemptRuntimeClass(pod.Spec.RuntimeClassName) { - continue - } - + checkedPods := len(prioritizedPods) + for i, pod := range prioritizedPods { r := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(enforce, &pod.ObjectMeta, &pod.Spec)) if !r.Allowed { warning := r.ForbiddenReason() @@ -555,6 +547,7 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin podWarningsToCount[warning] = c } if err := ctx.Err(); err != nil { // deadline exceeded or context was cancelled + checkedPods = i + 1 break } } @@ -738,26 +731,33 @@ func (a *Admission) exemptRuntimeClass(runtimeClass *string) bool { return containsString(*runtimeClass, a.Configuration.Exemptions.RuntimeClasses) } -// Filter and prioritise pods based on runtimeclass and uniqueness of the controller respectively for evaluation -func (a *Admission) prioritisePods(pods []*corev1.Pod) []*corev1.Pod { - var replicatedPods []*corev1.Pod - var prioritisedPods []*corev1.Pod +// Filter and prioritize pods based on runtimeclass and uniqueness of the controller respectively for evaluation. +// The input slice is modified in place and should not be reused. +func (a *Admission) prioritizePods(pods []*corev1.Pod) []*corev1.Pod { + // accumulate the list of prioritized pods in-place to avoid double-allocating + prioritizedPods := pods[:0] + // accumulate any additional replicated pods after the first one encountered for a given controller uid + var duplicateReplicatedPods []*corev1.Pod evaluatedControllers := make(map[types.UID]bool) for _, pod := range pods { + // short-circuit on exempt runtimeclass + if a.exemptRuntimeClass(pod.Spec.RuntimeClassName) { + continue + } // short-circuit if pod from the same controller is evaluated podOwnerControllerRef := metav1.GetControllerOfNoCopy(pod) if podOwnerControllerRef == nil { - prioritisedPods = append(prioritisedPods, pod) + prioritizedPods = append(prioritizedPods, pod) continue } if evaluatedControllers[podOwnerControllerRef.UID] { - replicatedPods = append(replicatedPods, pod) + duplicateReplicatedPods = append(duplicateReplicatedPods, pod) continue } - prioritisedPods = append(prioritisedPods, pod) + prioritizedPods = append(prioritizedPods, pod) evaluatedControllers[podOwnerControllerRef.UID] = true } - return append(prioritisedPods, replicatedPods...) + return append(prioritizedPods, duplicateReplicatedPods...) } func containsString(needle string, haystack []string) bool { diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index e1c1e03fcd3..c3c9b0b12cb 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -24,9 +24,6 @@ import ( "testing" "time" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/uuid" - "github.com/stretchr/testify/assert" admissionv1 "k8s.io/api/admission/v1" @@ -36,6 +33,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" admissionapi "k8s.io/pod-security-admission/admission/api" "k8s.io/pod-security-admission/api" "k8s.io/pod-security-admission/metrics" @@ -438,7 +437,44 @@ func TestValidateNamespace(t *testing.T) { `pod1 (and 3 other pods): message`, }, }, - // TODO: test for prioritizing evaluating pods from unique controllers + { + name: "prioritized pods", + exemptRuntimeClasses: []string{"runtimeclass1"}, + newLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelRestricted)}, + oldLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelBaseline)}, + pods: []*corev1.Pod{ + // ensure exempt pods don't use up the limit of evaluated pods + {ObjectMeta: metav1.ObjectMeta{Name: "exemptpod1", Annotations: map[string]string{"error": "message1"}}, Spec: corev1.PodSpec{RuntimeClassName: pointer.String("runtimeclass1")}}, + {ObjectMeta: metav1.ObjectMeta{Name: "exemptpod2", Annotations: map[string]string{"error": "message1"}}, Spec: corev1.PodSpec{RuntimeClassName: pointer.String("runtimeclass1")}}, + {ObjectMeta: metav1.ObjectMeta{Name: "exemptpod3", Annotations: map[string]string{"error": "message1"}}, Spec: corev1.PodSpec{RuntimeClassName: pointer.String("runtimeclass1")}}, + {ObjectMeta: metav1.ObjectMeta{Name: "exemptpod4", Annotations: map[string]string{"error": "message1"}}, Spec: corev1.PodSpec{RuntimeClassName: pointer.String("runtimeclass1")}}, + // ensure replicas from the same controller don't use up limit of evaluated pods + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset1pod1", Annotations: map[string]string{"error": "replicaset1error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("1"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset1pod2", Annotations: map[string]string{"error": "replicaset1error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("1"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset1pod3", Annotations: map[string]string{"error": "replicaset1error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("1"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset1pod4", Annotations: map[string]string{"error": "replicaset1error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("1"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset2pod1", Annotations: map[string]string{"error": "replicaset2error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("2"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset2pod2", Annotations: map[string]string{"error": "replicaset2error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("2"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset2pod3", Annotations: map[string]string{"error": "replicaset2error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("2"), Controller: pointer.Bool(true)}}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "replicaset2pod4", Annotations: map[string]string{"error": "replicaset2error"}, OwnerReferences: []metav1.OwnerReference{{UID: types.UID("2"), Controller: pointer.Bool(true)}}}}, + // ensure unique pods are prioritized before additional replicas + {ObjectMeta: metav1.ObjectMeta{Name: "uniquepod1", Annotations: map[string]string{"error": "uniquemessage1"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "uniquepod2", Annotations: map[string]string{"error": "uniquemessage2"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "uniquepod3", Annotations: map[string]string{"error": "uniquemessage3"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "uniquepod4", Annotations: map[string]string{"error": "uniquemessage4"}}}, + }, + expectAllowed: true, + expectListPods: true, + expectEvaluate: api.LevelVersion{Level: api.LevelRestricted, Version: api.LatestVersion()}, + expectWarnings: []string{ + `new PodSecurity enforce level only checked against the first 4 of 12 existing pods`, + `existing pods in namespace "test" violate the new PodSecurity enforce level "restricted:latest"`, + `replicaset1pod1: replicaset1error`, + `replicaset2pod1: replicaset2error`, + `uniquepod1: uniquemessage1`, + `uniquepod2: uniquemessage2`, + }, + }, } for _, tc := range testcases { @@ -540,7 +576,7 @@ func TestValidateNamespace(t *testing.T) { t.Errorf("expected to evaluate %v, got %v", tc.expectEvaluate, evaluator.lv) } if !reflect.DeepEqual(result.Warnings, tc.expectWarnings) { - t.Errorf("expected warnings:\n%v\ngot\n%v", tc.expectWarnings, result.Warnings) + t.Errorf("expected warnings:\n%v\ngot\n%v", strings.Join(tc.expectWarnings, "\n"), strings.Join(result.Warnings, "\n")) } }) } @@ -770,7 +806,7 @@ func (r *FakeRecorder) ExpectEvaluations(t *testing.T, expected []EvaluationReco assert.ElementsMatch(t, expected, r.evaluations) } -func TestPrioritisePods(t *testing.T) { +func TestPrioritizePods(t *testing.T) { isController := true sampleOwnerReferences := []struct { ownerRefs []metav1.OwnerReference @@ -813,16 +849,16 @@ func TestPrioritisePods(t *testing.T) { } } a := &Admission{} - prioritisedPods := a.prioritisePods(pods) + prioritizedPods := a.prioritizePods(pods) controllerRef := make(map[types.UID]bool) for i := 0; i < len(sampleOwnerReferences); i++ { - if controllerRef[metav1.GetControllerOfNoCopy(prioritisedPods[i]).UID] { - assert.Fail(t, "Pods are not prioritised based on uniqueness of the controller") + if controllerRef[metav1.GetControllerOfNoCopy(prioritizedPods[i]).UID] { + assert.Fail(t, "Pods are not prioritized based on uniqueness of the controller") } - controllerRef[metav1.GetControllerOfNoCopy(prioritisedPods[i]).UID] = true + controllerRef[metav1.GetControllerOfNoCopy(prioritizedPods[i]).UID] = true } - if len(prioritisedPods) != len(pods) { + if len(prioritizedPods) != len(pods) { assert.Fail(t, "Pod count is not the same after prioritization") } -} \ No newline at end of file +}