PodSecurity: appArmorProfile: cleanup

Also allow  values
Add unit test exercising forbidden reason/detail
Clean up forbidden reason construction

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

@@ -18,11 +18,11 @@ package policy
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/pod-security-admission/api"
 )
 
@@ -34,7 +34,7 @@ profile, or restrict overrides to an allowed set of profiles.
 **Restricted Fields:**
 metadata.annotations['container.apparmor.security.beta.kubernetes.io/*']
 
-**Allowed Values:** 'runtime/default', undefined
+**Allowed Values:** 'runtime/default', 'localhost/*', empty, undefined
 */
 func init() {
 	addCheck(CheckAppArmorProfile)
@@ -56,31 +56,24 @@ func CheckAppArmorProfile() Check {
 }
 
 func allowedProfile(profile string) bool {
-	return profile == corev1.AppArmorBetaProfileRuntimeDefault ||
+	return len(profile) == 0 ||
+		profile == corev1.AppArmorBetaProfileRuntimeDefault ||
 		strings.HasPrefix(profile, corev1.AppArmorBetaProfileNamePrefix)
 }
 
 func appArmorProfile_1_0(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {
-	forbiddenValues := sets.NewString()
+	var forbiddenValues []string
-
-	// undefined is an allowed value for 'container.apparmor.security.beta.kubernetes.io/*'
-	if len(podMetadata.Annotations) == 0 {
-		return CheckResult{Allowed: true}
-	}
-
 	for k, v := range podMetadata.Annotations {
 		if strings.HasPrefix(k, corev1.AppArmorBetaContainerAnnotationKeyPrefix) && !allowedProfile(v) {
-			forbiddenValues.Insert(fmt.Sprintf("%s:%s", k, v))
+			forbiddenValues = append(forbiddenValues, fmt.Sprintf("%s=%q", k, v))
 		}
 	}
-
 	if len(forbiddenValues) > 0 {
+		sort.Strings(forbiddenValues)
 		return CheckResult{
 			Allowed:         false,
-			ForbiddenReason: "forbidden AppArmor profile",
+			ForbiddenReason: pluralize("forbidden AppArmor profile", "forbidden AppArmor profiles", len(forbiddenValues)),
-			ForbiddenDetail: fmt.Sprintf("forbidden AppArmor annotations %q",
+			ForbiddenDetail: strings.Join(forbiddenValues, ", "),
-				forbiddenValues,
-			),
 		}
 	}
 

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

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 The Kubernetes Authors.
+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.
@@ -17,6 +17,7 @@ limitations under the License.
 package policy
 
 import (
+	"strings"
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
@@ -78,3 +79,50 @@ func TestCheckAppArmor(t *testing.T) {
 		})
 	}
 }
+
+func TestAppArmorProfile(t *testing.T) {
+	tests := []struct {
+		name         string
+		pod          *corev1.Pod
+		expectReason string
+		expectDetail string
+	}{
+		{
+			name: "multiple containers",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						`container.apparmor.security.beta.kubernetes.io/`:  `bogus`,
+						`container.apparmor.security.beta.kubernetes.io/a`: ``,
+						`container.apparmor.security.beta.kubernetes.io/b`: `runtime/default`,
+						`container.apparmor.security.beta.kubernetes.io/c`: `localhost/`,
+						`container.apparmor.security.beta.kubernetes.io/d`: `localhost/foo`,
+						`container.apparmor.security.beta.kubernetes.io/e`: `unconfined`,
+						`container.apparmor.security.beta.kubernetes.io/f`: `unknown`,
+					},
+				},
+			},
+			expectReason: `forbidden AppArmor profiles`,
+			expectDetail: strings.Join([]string{
+				`container.apparmor.security.beta.kubernetes.io/="bogus"`,
+				`container.apparmor.security.beta.kubernetes.io/e="unconfined"`,
+				`container.apparmor.security.beta.kubernetes.io/f="unknown"`,
+			}, ", "),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			result := appArmorProfile_1_0(&tc.pod.ObjectMeta, &tc.pod.Spec)
+			if result.Allowed {
+				t.Fatal("expected disallowed")
+			}
+			if e, a := tc.expectReason, result.ForbiddenReason; e != a {
+				t.Errorf("expected\n%s\ngot\n%s", e, a)
+			}
+			if e, a := tc.expectDetail, result.ForbiddenDetail; e != a {
+				t.Errorf("expected\n%s\ngot\n%s", e, a)
+			}
+		})
+	}
+}