[PodSecurity] Include error audit annotation on all non-forbidden errors

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

@@ -19,7 +19,6 @@ package admission
 import (
 	"context"
 	"fmt"
-	"net/http"
 	"reflect"
 	"sort"
 	"time"
@@ -233,13 +232,13 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes)
 	}
 	obj, err := attrs.GetObject()
 	if err != nil {
-		klog.ErrorS(err, "failed to get object")
+		klog.ErrorS(err, "failed to decode object")
-		return internalErrorResponse("failed to get object")
+		return errorResponse(err, &apierrors.NewBadRequest("failed to decode object").ErrStatus)
 	}
 	namespace, ok := obj.(*corev1.Namespace)
 	if !ok {
 		klog.InfoS("failed to assert namespace type", "type", reflect.TypeOf(obj))
-		return badRequestResponse("failed to decode namespace")
+		return errorResponse(nil, &apierrors.NewBadRequest("failed to decode namespace").ErrStatus)
 	}
 
 	newPolicy, newErrs := a.PolicyToEvaluate(namespace.Labels)
@@ -257,12 +256,12 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes)
 		oldObj, err := attrs.GetOldObject()
 		if err != nil {
 			klog.ErrorS(err, "failed to decode old object")
-			return badRequestResponse("failed to decode old object")
+			return errorResponse(err, &apierrors.NewBadRequest("failed to decode  old object").ErrStatus)
 		}
 		oldNamespace, ok := oldObj.(*corev1.Namespace)
 		if !ok {
 			klog.InfoS("failed to assert old namespace type", "type", reflect.TypeOf(oldObj))
-			return badRequestResponse("failed to decode old namespace")
+			return errorResponse(nil, &apierrors.NewBadRequest("failed to decode  old namespace").ErrStatus)
 		}
 		oldPolicy, oldErrs := a.PolicyToEvaluate(oldNamespace.Labels)
 
@@ -335,7 +334,7 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi
 	if err != nil {
 		klog.ErrorS(err, "failed to fetch pod namespace", "namespace", attrs.GetNamespace())
 		a.Metrics.RecordError(true, attrs)
-		return internalErrorResponse(fmt.Sprintf("failed to lookup namespace %s", attrs.GetNamespace()))
+		return errorResponse(err, &apierrors.NewInternalError(fmt.Errorf("failed to lookup namespace %s", attrs.GetNamespace())).ErrStatus)
 	}
 	nsPolicy, nsPolicyErrs := a.PolicyToEvaluate(namespace.Labels)
 	if len(nsPolicyErrs) == 0 && nsPolicy.Enforce.Level == api.LevelPrivileged && nsPolicy.Warn.Level == api.LevelPrivileged && nsPolicy.Audit.Level == api.LevelPrivileged {
@@ -347,26 +346,26 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi
 	if err != nil {
 		klog.ErrorS(err, "failed to decode object")
 		a.Metrics.RecordError(true, attrs)
-		return badRequestResponse("failed to decode object")
+		return errorResponse(err, &apierrors.NewBadRequest("failed to decode object").ErrStatus)
 	}
 	pod, ok := obj.(*corev1.Pod)
 	if !ok {
 		klog.InfoS("failed to assert pod type", "type", reflect.TypeOf(obj))
 		a.Metrics.RecordError(true, attrs)
-		return badRequestResponse("failed to decode pod")
+		return errorResponse(nil, &apierrors.NewBadRequest("failed to decode pod").ErrStatus)
 	}
 	if attrs.GetOperation() == admissionv1.Update {
 		oldObj, err := attrs.GetOldObject()
 		if err != nil {
 			klog.ErrorS(err, "failed to decode old object")
 			a.Metrics.RecordError(true, attrs)
-			return badRequestResponse("failed to decode old object")
+			return errorResponse(err, &apierrors.NewBadRequest("failed to decode old object").ErrStatus)
 		}
 		oldPod, ok := oldObj.(*corev1.Pod)
 		if !ok {
 			klog.InfoS("failed to assert old pod type", "type", reflect.TypeOf(oldObj))
 			a.Metrics.RecordError(true, attrs)
-			return badRequestResponse("failed to decode old pod")
+			return errorResponse(nil, &apierrors.NewBadRequest("failed to decode old pod").ErrStatus)
 		}
 		if !isSignificantPodUpdate(pod, oldPod) {
 			// Nothing we care about changed, so always allow the update.
@@ -401,7 +400,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu
 		a.Metrics.RecordError(true, attrs)
 		response := allowedResponse()
 		response.AuditAnnotations = map[string]string{
-			"error": fmt.Sprintf("failed to lookup namespace %s", attrs.GetNamespace()),
+			"error": fmt.Sprintf("failed to lookup namespace %s: %v", attrs.GetNamespace(), err),
 		}
 		return response
 	}
@@ -416,7 +415,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu
 		a.Metrics.RecordError(true, attrs)
 		response := allowedResponse()
 		response.AuditAnnotations = map[string]string{
-			"error": "failed to decode object",
+			"error": fmt.Sprintf("failed to decode object: %v", err),
 		}
 		return response
 	}
@@ -426,7 +425,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu
 		a.Metrics.RecordError(true, attrs)
 		response := allowedResponse()
 		response.AuditAnnotations = map[string]string{
-			"error": "failed to extract pod template",
+			"error": fmt.Sprintf("failed to extract pod template: %v", err),
 		}
 		return response
 	}
@@ -464,8 +463,8 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli
 
 		result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Enforce, podMetadata, podSpec))
 		if !result.Allowed {
-			response = forbiddenResponse(fmt.Sprintf(
+			response = forbiddenResponse(attrs, fmt.Errorf(
-				"pod violates PodSecurity %q: %s",
+				"violates PodSecurity %q: %s",
 				nsPolicy.Enforce.String(),
 				result.ForbiddenDetail(),
 			))
@@ -613,76 +612,6 @@ func (a *Admission) PolicyToEvaluate(labels map[string]string) (api.Policy, fiel
 	return api.PolicyToEvaluate(labels, a.defaultPolicy)
 }
 
-var (
-	_sharedAllowedResponse                        = allowedResponse()
-	_sharedAllowedByUserExemptionResponse         = allowedByExemptResponse("user")
-	_sharedAllowedByNamespaceExemptionResponse    = allowedByExemptResponse("namespace")
-	_sharedAllowedByRuntimeClassExemptionResponse = allowedByExemptResponse("runtimeClass")
-)
-
-func sharedAllowedResponse() *admissionv1.AdmissionResponse {
-	return _sharedAllowedResponse
-}
-
-func sharedAllowedByUserExemptionResponse() *admissionv1.AdmissionResponse {
-	return _sharedAllowedByUserExemptionResponse
-}
-
-func sharedAllowedByNamespaceExemptionResponse() *admissionv1.AdmissionResponse {
-	return _sharedAllowedByNamespaceExemptionResponse
-}
-
-func sharedAllowedByRuntimeClassExemptionResponse() *admissionv1.AdmissionResponse {
-	return _sharedAllowedByRuntimeClassExemptionResponse
-}
-
-// allowedResponse is the response used when the admission decision is allow.
-func allowedResponse() *admissionv1.AdmissionResponse {
-	return &admissionv1.AdmissionResponse{Allowed: true}
-}
-
-func allowedByExemptResponse(exemptionReason string) *admissionv1.AdmissionResponse {
-	return &admissionv1.AdmissionResponse{
-		Allowed:          true,
-		AuditAnnotations: map[string]string{api.ExemptionReasonAnnotationKey: exemptionReason},
-	}
-}
-
-func failureResponse(msg string, reason metav1.StatusReason, code int32) *admissionv1.AdmissionResponse {
-	return &admissionv1.AdmissionResponse{
-		Allowed: false,
-		Result: &metav1.Status{
-			Status:  metav1.StatusFailure,
-			Reason:  reason,
-			Message: msg,
-			Code:    code,
-		},
-	}
-}
-
-// forbiddenResponse is the response used when the admission decision is deny for policy violations.
-func forbiddenResponse(msg string) *admissionv1.AdmissionResponse {
-	return failureResponse(msg, metav1.StatusReasonForbidden, http.StatusForbidden)
-}
-
-// invalidResponse is the response used for namespace requests when namespace labels are invalid.
-func invalidResponse(attrs api.Attributes, fieldErrors field.ErrorList) *admissionv1.AdmissionResponse {
-	return &admissionv1.AdmissionResponse{
-		Allowed: false,
-		Result:  &apierrors.NewInvalid(attrs.GetKind().GroupKind(), attrs.GetName(), fieldErrors).ErrStatus,
-	}
-}
-
-// badRequestResponse is the response used when a request cannot be processed.
-func badRequestResponse(msg string) *admissionv1.AdmissionResponse {
-	return failureResponse(msg, metav1.StatusReasonBadRequest, http.StatusBadRequest)
-}
-
-// internalErrorResponse is the response used for unexpected errors
-func internalErrorResponse(msg string) *admissionv1.AdmissionResponse {
-	return failureResponse(msg, metav1.StatusReasonInternalError, http.StatusInternalServerError)
-}
-
 // isSignificantPodUpdate determines whether a pod update should trigger a policy evaluation.
 // Relevant mutable pod fields as of 1.21 are image and seccomp annotations:
 // * https://github.com/kubernetes/kubernetes/blob/release-1.21/pkg/apis/core/validation/validation.go#L3947-L3949

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

@@ -688,14 +688,14 @@ func TestValidatePodAndController(t *testing.T) {
 	type testCase struct {
 		desc string
 
-		namespace    string
+		namespace string
-		username     string
+		username  string
-		runtimeClass string
 
-		operation admissionv1.Operation
+		// pod and oldPod are used to populate obj and oldObj respectively, according to the test type (pod or deployment).
-		pod       *corev1.Pod
+		pod    *corev1.Pod
-		oldPod    *corev1.Pod
+		oldPod *corev1.Pod
 
+		operation   admissionv1.Operation
 		resource    schema.GroupVersionResource
 		kind        schema.GroupVersionKind
 		obj         runtime.Object
@@ -744,11 +744,12 @@ func TestValidatePodAndController(t *testing.T) {
 			expectedAuditAnnotationKeys: []string{"exempt"},
 		},
 		{
-			desc:          "namespace not found",
+			desc:                        "namespace not found",
-			namespace:     "missing-ns",
+			namespace:                   "missing-ns",
-			pod:           restrictedPod.DeepCopy(),
+			pod:                         restrictedPod.DeepCopy(),
-			expectAllowed: false,
+			expectAllowed:               false,
-			expectReason:  metav1.StatusReasonInternalError,
+			expectReason:                metav1.StatusReasonInternalError,
+			expectedAuditAnnotationKeys: []string{"error"},
 		},
 		{
 			desc:          "short-circuit privileged:latest (implicit)",
@@ -763,39 +764,43 @@ func TestValidatePodAndController(t *testing.T) {
 			expectAllowed: true,
 		},
 		{
-			desc:          "failed decode",
+			desc:                        "failed decode",
-			namespace:     baselineNs,
+			namespace:                   baselineNs,
-			objErr:        fmt.Errorf("expected (failed decode)"),
+			objErr:                      fmt.Errorf("expected (failed decode)"),
-			expectAllowed: false,
+			expectAllowed:               false,
-			expectReason:  metav1.StatusReasonBadRequest,
+			expectReason:                metav1.StatusReasonBadRequest,
+			expectedAuditAnnotationKeys: []string{"error"},
 		},
 		{
-			desc:          "invalid object",
+			desc:                        "invalid object",
-			namespace:     baselineNs,
+			namespace:                   baselineNs,
-			operation:     admissionv1.Update,
+			operation:                   admissionv1.Update,
-			obj:           &corev1.Namespace{},
+			obj:                         &corev1.Namespace{},
-			expectAllowed: false,
+			expectAllowed:               false,
-			expectReason:  metav1.StatusReasonBadRequest,
+			expectReason:                metav1.StatusReasonBadRequest,
+			expectedAuditAnnotationKeys: []string{"error"},
 		},
 		{
-			desc:           "failed decode old object",
+			desc:                        "failed decode old object",
-			namespace:      baselineNs,
+			namespace:                   baselineNs,
-			operation:      admissionv1.Update,
+			operation:                   admissionv1.Update,
-			pod:            restrictedPod.DeepCopy(),
+			pod:                         restrictedPod.DeepCopy(),
-			oldObjErr:      fmt.Errorf("expected (failed decode)"),
+			oldObjErr:                   fmt.Errorf("expected (failed decode)"),
-			expectAllowed:  false,
+			expectAllowed:               false,
-			expectReason:   metav1.StatusReasonBadRequest,
+			expectReason:                metav1.StatusReasonBadRequest,
-			skipDeployment: true, // Updates aren't special cased for controller resources.
+			expectedAuditAnnotationKeys: []string{"error"},
+			skipDeployment:              true, // Updates aren't special cased for controller resources.
 		},
 		{
-			desc:           "invalid old object",
+			desc:                        "invalid old object",
-			namespace:      baselineNs,
+			namespace:                   baselineNs,
-			operation:      admissionv1.Update,
+			operation:                   admissionv1.Update,
-			pod:            restrictedPod.DeepCopy(),
+			pod:                         restrictedPod.DeepCopy(),
-			oldObj:         &corev1.Namespace{},
+			oldObj:                      &corev1.Namespace{},
-			expectAllowed:  false,
+			expectAllowed:               false,
-			expectReason:   metav1.StatusReasonBadRequest,
+			expectReason:                metav1.StatusReasonBadRequest,
-			skipDeployment: true, // Updates aren't special cased for controller resources.
+			expectedAuditAnnotationKeys: []string{"error"},
+			skipDeployment:              true, // Updates aren't special cased for controller resources.
 		},
 		{
 			desc:           "insignificant update",
@@ -907,12 +912,8 @@ func TestValidatePodAndController(t *testing.T) {
 		deploymentTest.desc = "deployment:" + tc.desc
 		deploymentTest.resource = schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}
 		deploymentTest.kind = schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}
-		deploymentTest.expectAllowed = true // Deployments policies are non-enforcing.
+		deploymentTest.expectAllowed = true // PodController validation is always non-enforcing.
 		deploymentTest.expectReason = ""
-		if tc.expectReason != "" && tc.expectReason != metav1.StatusReasonForbidden {
-			// Error case, expect an error annotation.
-			deploymentTest.expectedAuditAnnotationKeys = append(deploymentTest.expectedAuditAnnotationKeys, "error")
-		}
 
 		if tc.pod != nil {
 			podTest.obj = tc.pod

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

@@ -0,0 +1,93 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package admission
+
+import (
+	"fmt"
+
+	admissionv1 "k8s.io/api/admission/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/pod-security-admission/api"
+)
+
+var (
+	_sharedAllowedResponse                        = allowedResponse()
+	_sharedAllowedByUserExemptionResponse         = allowedByExemptResponse("user")
+	_sharedAllowedByNamespaceExemptionResponse    = allowedByExemptResponse("namespace")
+	_sharedAllowedByRuntimeClassExemptionResponse = allowedByExemptResponse("runtimeClass")
+)
+
+func sharedAllowedResponse() *admissionv1.AdmissionResponse {
+	return _sharedAllowedResponse
+}
+
+func sharedAllowedByUserExemptionResponse() *admissionv1.AdmissionResponse {
+	return _sharedAllowedByUserExemptionResponse
+}
+
+func sharedAllowedByNamespaceExemptionResponse() *admissionv1.AdmissionResponse {
+	return _sharedAllowedByNamespaceExemptionResponse
+}
+
+func sharedAllowedByRuntimeClassExemptionResponse() *admissionv1.AdmissionResponse {
+	return _sharedAllowedByRuntimeClassExemptionResponse
+}
+
+// allowedResponse is the response used when the admission decision is allow.
+func allowedResponse() *admissionv1.AdmissionResponse {
+	return &admissionv1.AdmissionResponse{Allowed: true}
+}
+
+func allowedByExemptResponse(exemptionReason string) *admissionv1.AdmissionResponse {
+	return &admissionv1.AdmissionResponse{
+		Allowed:          true,
+		AuditAnnotations: map[string]string{api.ExemptionReasonAnnotationKey: exemptionReason},
+	}
+}
+
+// forbiddenResponse is the response used when the admission decision is deny for policy violations.
+func forbiddenResponse(attrs api.Attributes, err error) *admissionv1.AdmissionResponse {
+	return &admissionv1.AdmissionResponse{
+		Allowed: false,
+		Result:  &apierrors.NewForbidden(attrs.GetResource().GroupResource(), attrs.GetName(), err).ErrStatus,
+	}
+}
+
+// invalidResponse is the response used for namespace requests when namespace labels are invalid.
+func invalidResponse(attrs api.Attributes, fieldErrors field.ErrorList) *admissionv1.AdmissionResponse {
+	return &admissionv1.AdmissionResponse{
+		Allowed: false,
+		Result:  &apierrors.NewInvalid(attrs.GetKind().GroupKind(), attrs.GetName(), fieldErrors).ErrStatus,
+	}
+}
+
+// errorResponse is the response used to capture generic errors.
+func errorResponse(err error, status *metav1.Status) *admissionv1.AdmissionResponse {
+	var errDetail string
+	if err != nil {
+		errDetail = fmt.Sprintf("%s: %v", status.Message, err)
+	} else {
+		errDetail = status.Message
+	}
+	return &admissionv1.AdmissionResponse{
+		Allowed:          false,
+		Result:           status,
+		AuditAnnotations: map[string]string{"error": errDetail},
+	}
+}