Merge pull request #105922 from liggitt/podsecurity-warnings

PodSecurity: clean up namespace validation messages, time bounding, and add testing

staging/src/k8s.io/pod-security-admission/admission/admission.go

@@ -41,8 +41,8 @@ import (
 )
 
 const (
-	namespaceMaxPodsToCheck  = 3000
+	defaultNamespaceMaxPodsToCheck  = 3000
-	namespacePodCheckTimeout = 1 * time.Second
+	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)
+		klog.ErrorS(err, "failed to list pods", "namespace", namespace)
-		return []string{"Failed to list pods"}
+		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 {
+	totalPods := len(pods)
-		warnings = append(warnings, fmt.Sprintf("Large namespace: only checking the first %d of %d pods", namespaceMaxPodsToCheck, len(pods)))
+	if len(pods) > a.namespaceMaxPodsToCheck {
-		pods = pods[0: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) {
+		if err := ctx.Err(); err != nil { // deadline exceeded or context was cancelled
-			warnings = append(warnings, fmt.Sprintf("Timeout reached after checking %d pods", i+1))
+			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

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}
+			podLister := &testPodLister{pods: pods, delay: tc.delayList}
-			evaluator := &testEvaluator{}
+			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)"},
+			expectAuditAnnotations: map[string]string{"audit": "would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"},
-			expectWarnings:         []string{"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)"},
+			expectAuditAnnotations: map[string]string{"audit": "would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"},
-			expectWarnings:         []string{"Would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"},
+			expectWarnings:         []string{"would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"},
 		},
 	}