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 4059b62782b..a3a698d8d92 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -41,8 +41,8 @@ import ( ) const ( - namespaceMaxPodsToCheck = 3000 - namespacePodCheckTimeout = 1 * time.Second + defaultNamespaceMaxPodsToCheck = 3000 + defaultNamespacePodCheckTimeout = 1 * time.Second ) // Admission implements the core admission logic for the Pod Security Admission controller. @@ -64,6 +64,9 @@ type Admission struct { PodLister PodLister defaultPolicy api.Policy + + namespaceMaxPodsToCheck int + namespacePodCheckTimeout time.Duration } type NamespaceGetter interface { @@ -152,6 +155,8 @@ func (a *Admission) CompleteConfiguration() error { a.defaultPolicy = p } } + a.namespaceMaxPodsToCheck = defaultNamespaceMaxPodsToCheck + a.namespacePodCheckTimeout = defaultNamespacePodCheckTimeout if a.PodSpecExtractor == nil { a.PodSpecExtractor = &DefaultPodSpecExtractor{} @@ -173,6 +178,9 @@ func (a *Admission) ValidateConfiguration() error { return fmt.Errorf("default policy does not match; CompleteConfiguration() was not called before ValidateConfiguration()") } } + if a.namespaceMaxPodsToCheck == 0 || a.namespacePodCheckTimeout == 0 { + return fmt.Errorf("namespace configuration not set; CompleteConfiguration() was not called before ValidateConfiguration()") + } if a.Metrics == nil { return fmt.Errorf("Metrics recorder required") } @@ -409,14 +417,14 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli } if klog.V(5).Enabled() { - klog.InfoS("Pod Security evaluation", "policy", fmt.Sprintf("%v", nsPolicy), "op", attrs.GetOperation(), "resource", attrs.GetResource(), "namespace", attrs.GetNamespace(), "name", attrs.GetName()) + klog.InfoS("PodSecurity evaluation", "policy", fmt.Sprintf("%v", nsPolicy), "op", attrs.GetOperation(), "resource", attrs.GetResource(), "namespace", attrs.GetNamespace(), "name", attrs.GetName()) } response := allowedResponse() if enforce { if result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Enforce, podMetadata, podSpec)); !result.Allowed { response = forbiddenResponse(fmt.Sprintf( - "Pod violates PodSecurity %q: %s", + "pod violates PodSecurity %q: %s", nsPolicy.Enforce.String(), result.ForbiddenDetail(), )) @@ -429,7 +437,7 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli // TODO: reuse previous evaluation if audit level+version is the same as enforce level+version if result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Audit, podMetadata, podSpec)); !result.Allowed { auditAnnotations["audit"] = fmt.Sprintf( - "Would violate PodSecurity %q: %s", + "would violate PodSecurity %q: %s", nsPolicy.Audit.String(), result.ForbiddenDetail(), ) @@ -442,7 +450,7 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli if result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Warn, podMetadata, podSpec)); !result.Allowed { // TODO: Craft a better user-facing warning message response.Warnings = append(response.Warnings, fmt.Sprintf( - "Would violate PodSecurity %q: %s", + "would violate PodSecurity %q: %s", nsPolicy.Warn.String(), result.ForbiddenDetail(), )) @@ -464,9 +472,10 @@ type podCount struct { } func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace string, enforce api.LevelVersion) []string { - timeout := namespacePodCheckTimeout + // start with the default timeout + timeout := a.namespacePodCheckTimeout if deadline, ok := ctx.Deadline(); ok { - timeRemaining := time.Duration(0.9 * float64(time.Until(deadline))) // Leave a little time to respond. + timeRemaining := time.Until(deadline) / 2 // don't take more than half the remaining time if timeout > timeRemaining { timeout = timeRemaining } @@ -477,8 +486,8 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin pods, err := a.PodLister.ListPods(ctx, namespace) if err != nil { - klog.ErrorS(err, "Failed to list pods", "namespace", namespace) - return []string{"Failed to list pods"} + klog.ErrorS(err, "failed to list pods", "namespace", namespace) + return []string{"failed to list pods while checking new PodSecurity enforce level"} } var ( @@ -487,11 +496,12 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin podWarnings []string podWarningsToCount = make(map[string]podCount) ) - if len(pods) > namespaceMaxPodsToCheck { - warnings = append(warnings, fmt.Sprintf("Large namespace: only checking the first %d of %d pods", namespaceMaxPodsToCheck, len(pods))) - pods = pods[0:namespaceMaxPodsToCheck] + totalPods := len(pods) + if len(pods) > a.namespaceMaxPodsToCheck { + pods = pods[0:a.namespaceMaxPodsToCheck] } + checkedPods := len(pods) for i, pod := range pods { // short-circuit on exempt runtimeclass if a.exemptRuntimeClass(pod.Spec.RuntimeClassName) { @@ -510,12 +520,20 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin c.podCount++ podWarningsToCount[warning] = c } - if time.Now().After(deadline) { - warnings = append(warnings, fmt.Sprintf("Timeout reached after checking %d pods", i+1)) + if err := ctx.Err(); err != nil { // deadline exceeded or context was cancelled + checkedPods = i + 1 break } } + if checkedPods < totalPods { + warnings = append(warnings, fmt.Sprintf("new PodSecurity enforce level only checked against the first %d of %d existing pods", checkedPods, totalPods)) + } + + if len(podWarnings) > 0 { + warnings = append(warnings, fmt.Sprintf("existing pods in namespace %q violate the new PodSecurity enforce level %q", namespace, enforce.String())) + } + // prepend pod names to warnings decoratePodWarnings(podWarningsToCount, podWarnings) // put warnings in a deterministic order 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 90bf7605bf1..da198f8391e 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 @@ -21,6 +21,7 @@ import ( "reflect" "strings" "testing" + "time" "github.com/stretchr/testify/assert" @@ -174,9 +175,14 @@ func TestDefaultHasPodSpec(t *testing.T) { type testEvaluator struct { lv api.LevelVersion + + delay time.Duration } func (t *testEvaluator) EvaluatePod(lv api.LevelVersion, meta *metav1.ObjectMeta, spec *corev1.PodSpec) []policy.CheckResult { + if t.delay > 0 { + time.Sleep(t.delay) + } t.lv = lv if meta.Annotations["error"] != "" { return []policy.CheckResult{{Allowed: false, ForbiddenReason: meta.Annotations["error"]}} @@ -196,10 +202,17 @@ func (t *testNamespaceGetter) GetNamespace(ctx context.Context, name string) (*c type testPodLister struct { called bool pods []*corev1.Pod + delay time.Duration } func (t *testPodLister) ListPods(ctx context.Context, namespace string) ([]*corev1.Pod, error) { t.called = true + if t.delay > 0 { + time.Sleep(t.delay) + } + if err := ctx.Err(); err != nil { + return nil, err + } return t.pods, nil } @@ -218,6 +231,10 @@ func TestValidateNamespace(t *testing.T) { oldLabels map[string]string // list of pods to return pods []*corev1.Pod + // time to sleep while listing + delayList time.Duration + // time to sleep while evaluating + delayEvaluation time.Duration expectAllowed bool expectError string @@ -352,7 +369,11 @@ func TestValidateNamespace(t *testing.T) { expectAllowed: true, expectListPods: true, expectEvaluate: api.LevelVersion{Level: api.LevelRestricted, Version: api.LatestVersion()}, - expectWarnings: []string{"noruntimeclasspod (and 2 other pods): message", "runtimeclass3pod: message, message2"}, + expectWarnings: []string{ + `existing pods in namespace "test" violate the new PodSecurity enforce level "restricted:latest"`, + "noruntimeclasspod (and 2 other pods): message", + "runtimeclass3pod: message, message2", + }, }, { name: "update with runtimeclass exempt pods", @@ -362,11 +383,57 @@ func TestValidateNamespace(t *testing.T) { expectAllowed: true, expectListPods: true, expectEvaluate: api.LevelVersion{Level: api.LevelRestricted, Version: api.LatestVersion()}, - expectWarnings: []string{"noruntimeclasspod (and 1 other pod): message", "runtimeclass3pod: message, message2"}, + expectWarnings: []string{ + `existing pods in namespace "test" violate the new PodSecurity enforce level "restricted:latest"`, + "noruntimeclasspod (and 1 other pod): message", + "runtimeclass3pod: message, message2", + }, + }, + { + name: "timeout on list", + newLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelRestricted)}, + oldLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelBaseline)}, + delayList: time.Second + 100*time.Millisecond, + expectAllowed: true, + expectListPods: true, + expectWarnings: []string{ + `failed to list pods while checking new PodSecurity enforce level`, + }, + }, + { + name: "timeout on evaluate", + newLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelRestricted)}, + oldLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelBaseline)}, + delayEvaluation: (time.Second + 100*time.Millisecond) / 2, // leave time for two evaluations + expectAllowed: true, + expectListPods: true, + expectEvaluate: api.LevelVersion{Level: api.LevelRestricted, Version: api.LatestVersion()}, + expectWarnings: []string{ + `new PodSecurity enforce level only checked against the first 2 of 4 existing pods`, + `existing pods in namespace "test" violate the new PodSecurity enforce level "restricted:latest"`, + `noruntimeclasspod (and 1 other pod): message`, + }, + }, + { + name: "bound number of pods", + newLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelRestricted)}, + oldLabels: map[string]string{api.EnforceLevelLabel: string(api.LevelBaseline)}, + pods: []*corev1.Pod{ + {ObjectMeta: metav1.ObjectMeta{Name: "pod1", Annotations: map[string]string{"error": "message"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "pod2", Annotations: map[string]string{"error": "message"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "pod3", Annotations: map[string]string{"error": "message"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "pod4", Annotations: map[string]string{"error": "message"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "pod5", Annotations: map[string]string{"error": "message"}}}, + }, + 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 5 existing pods`, + `existing pods in namespace "test" violate the new PodSecurity enforce level "restricted:latest"`, + `pod1 (and 3 other pods): message`, + }, }, - - // TODO: test for bounding evalution time with a warning - // TODO: test for bounding pod count with a warning // TODO: test for prioritizing evaluating pods from unique controllers } @@ -428,8 +495,8 @@ func TestValidateNamespace(t *testing.T) { }, } } - podLister := &testPodLister{pods: pods} - evaluator := &testEvaluator{} + podLister := &testPodLister{pods: pods, delay: tc.delayList} + evaluator := &testEvaluator{delay: tc.delayEvaluation} a := &Admission{ PodLister: podLister, Evaluator: evaluator, @@ -441,6 +508,9 @@ func TestValidateNamespace(t *testing.T) { }, Metrics: NewMockRecorder(), defaultPolicy: defaultPolicy, + + namespacePodCheckTimeout: time.Second, + namespaceMaxPodsToCheck: 4, } result := a.ValidateNamespace(context.TODO(), attrs) if result.Allowed != tc.expectAllowed { @@ -567,16 +637,16 @@ func TestValidatePodController(t *testing.T) { desc: "bad deploy creates produce correct user-visible warnings and correct auditAnnotations", newObject: &badDeploy, gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - expectAuditAnnotations: map[string]string{"audit": "Would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, - expectWarnings: []string{"Would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, + expectAuditAnnotations: map[string]string{"audit": "would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, + expectWarnings: []string{"would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, }, { desc: "bad spec updates don't block on enforce failures and returns correct information", newObject: &badDeploy, oldObject: &goodDeploy, gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - expectAuditAnnotations: map[string]string{"audit": "Would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, - expectWarnings: []string{"Would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, + expectAuditAnnotations: map[string]string{"audit": "would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, + expectWarnings: []string{"would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, }, }