Match annotations against pod AppArmor field

pkg/apis/core/validation/validation.go

@@ -4729,8 +4729,8 @@ func ValidateAppArmorProfileFormat(profile string) error {
 	return nil
 }
 
-// validateAppArmorAnnotationsAndFields validates that AppArmor fields and annotations are consistent.
+// validateAppArmorAnnotationsAndFieldsMatchOnCreate validates that AppArmor fields and annotations are consistent.
-func validateAppArmorAnnotationsAndFields(objectMeta metav1.ObjectMeta, podSpec *core.PodSpec, specPath *field.Path) field.ErrorList {
+func validateAppArmorAnnotationsAndFieldsMatchOnCreate(objectMeta metav1.ObjectMeta, podSpec *core.PodSpec, specPath *field.Path) field.ErrorList {
 	if podSpec.OS != nil && podSpec.OS.Name == core.Windows {
 		// Skip consistency check for windows pods.
 		return nil
@@ -4738,22 +4738,41 @@ func validateAppArmorAnnotationsAndFields(objectMeta metav1.ObjectMeta, podSpec
 
 	allErrs := field.ErrorList{}
 
+	var podProfile *core.AppArmorProfile
+	if podSpec.SecurityContext != nil {
+		podProfile = podSpec.SecurityContext.AppArmorProfile
+	}
 	podshelper.VisitContainersWithPath(podSpec, specPath, func(c *core.Container, cFldPath *field.Path) bool {
-		var field *core.AppArmorProfile
+		containerProfile := podProfile
-		if c.SecurityContext != nil {
+		if c.SecurityContext != nil && c.SecurityContext.AppArmorProfile != nil {
-			field = c.SecurityContext.AppArmorProfile
+			containerProfile = c.SecurityContext.AppArmorProfile
 		}
 
-		if field == nil {
+		if containerProfile == nil {
 			return true
 		}
 
 		key := core.AppArmorContainerAnnotationKeyPrefix + c.Name
 		if annotation, found := objectMeta.Annotations[key]; found {
 			apparmorPath := cFldPath.Child("securityContext").Child("appArmorProfile")
-			err := validateAppArmorAnnotationsAndFieldsMatch(annotation, field, apparmorPath)
+
-			if err != nil {
+			switch containerProfile.Type {
-				allErrs = append(allErrs, err)
+			case core.AppArmorProfileTypeUnconfined:
+				if annotation != core.AppArmorProfileNameUnconfined {
+					allErrs = append(allErrs, field.Forbidden(apparmorPath.Child("type"), "apparmor type in annotation and field must match"))
+				}
+
+			case core.AppArmorProfileTypeRuntimeDefault:
+				if annotation != core.AppArmorProfileRuntimeDefault {
+					allErrs = append(allErrs, field.Forbidden(apparmorPath.Child("type"), "apparmor type in annotation and field must match"))
+				}
+
+			case core.AppArmorProfileTypeLocalhost:
+				if !strings.HasPrefix(annotation, core.AppArmorProfileLocalhostPrefix) {
+					allErrs = append(allErrs, field.Forbidden(apparmorPath.Child("type"), "apparmor type in annotation and field must match"))
+				} else if containerProfile.LocalhostProfile == nil || strings.TrimPrefix(annotation, core.AppArmorProfileLocalhostPrefix) != *containerProfile.LocalhostProfile {
+					allErrs = append(allErrs, field.Forbidden(apparmorPath.Child("localhostProfile"), "apparmor profile in annotation and field must match"))
+				}
 			}
 		}
 		return true
@@ -4762,33 +4781,6 @@ func validateAppArmorAnnotationsAndFields(objectMeta metav1.ObjectMeta, podSpec
 	return allErrs
 }
 
-func validateAppArmorAnnotationsAndFieldsMatch(annotationValue string, apparmorField *core.AppArmorProfile, fldPath *field.Path) *field.Error {
-	if apparmorField == nil {
-		return nil
-	}
-
-	switch apparmorField.Type {
-	case core.AppArmorProfileTypeUnconfined:
-		if annotationValue != core.AppArmorProfileNameUnconfined {
-			return field.Forbidden(fldPath.Child("type"), "apparmor type in annotation and field must match")
-		}
-
-	case core.AppArmorProfileTypeRuntimeDefault:
-		if annotationValue != core.AppArmorProfileRuntimeDefault {
-			return field.Forbidden(fldPath.Child("type"), "apparmor type in annotation and field must match")
-		}
-
-	case core.AppArmorProfileTypeLocalhost:
-		if !strings.HasPrefix(annotationValue, core.AppArmorProfileLocalhostPrefix) {
-			return field.Forbidden(fldPath.Child("type"), "apparmor type in annotation and field must match")
-		} else if apparmorField.LocalhostProfile == nil || strings.TrimPrefix(annotationValue, core.AppArmorProfileLocalhostPrefix) != *apparmorField.LocalhostProfile {
-			return field.Forbidden(fldPath.Child("localhostProfile"), "apparmor profile in annotation and field must match")
-		}
-	}
-
-	return nil
-}
-
 func podSpecHasContainer(spec *core.PodSpec, containerName string) bool {
 	var hasContainer bool
 	podshelper.VisitContainersWithPath(spec, field.NewPath("spec"), func(c *core.Container, _ *field.Path) bool {
@@ -4943,7 +4935,7 @@ func ValidatePodCreate(pod *core.Pod, opts PodValidationOptions) field.ErrorList
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("nodeName"), "cannot be set until all schedulingGates have been cleared"))
 	}
 	allErrs = append(allErrs, validateSeccompAnnotationsAndFields(pod.ObjectMeta, &pod.Spec, fldPath)...)
-	allErrs = append(allErrs, validateAppArmorAnnotationsAndFields(pod.ObjectMeta, &pod.Spec, fldPath)...)
+	allErrs = append(allErrs, validateAppArmorAnnotationsAndFieldsMatchOnCreate(pod.ObjectMeta, &pod.Spec, fldPath)...)
 
 	return allErrs
 }

pkg/apis/core/validation/validation_test.go

@@ -10446,6 +10446,26 @@ func TestValidatePod(t *testing.T) {
 				DNSPolicy:     core.DNSDefault,
 			},
 		},
+		"matching AppArmor pod field and annotations": {
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					core.AppArmorContainerAnnotationKeyPrefix + "ctr": core.AppArmorProfileLocalhostPrefix + "foo",
+				},
+			},
+			Spec: core.PodSpec{
+				SecurityContext: &core.PodSecurityContext{
+					AppArmorProfile: &core.AppArmorProfile{
+						Type:             core.AppArmorProfileTypeLocalhost,
+						LocalhostProfile: ptr.To("foo"),
+					},
+				},
+				Containers:    []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}},
+				RestartPolicy: core.RestartPolicyAlways,
+				DNSPolicy:     core.DNSDefault,
+			},
+		},
 		"syntactically valid sysctls": {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "123",
@@ -12162,6 +12182,28 @@ func TestValidatePod(t *testing.T) {
 				},
 			},
 		},
+		"mismatched AppArmor pod field and annotation types": {
+			expectedError: "Forbidden: apparmor type in annotation and field must match",
+			spec: core.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "123",
+					Namespace: "ns",
+					Annotations: map[string]string{
+						core.AppArmorContainerAnnotationKeyPrefix + "ctr": core.AppArmorProfileRuntimeDefault,
+					},
+				},
+				Spec: core.PodSpec{
+					SecurityContext: &core.PodSecurityContext{
+						AppArmorProfile: &core.AppArmorProfile{
+							Type: core.AppArmorProfileTypeUnconfined,
+						},
+					},
+					Containers:    []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}},
+					RestartPolicy: core.RestartPolicyAlways,
+					DNSPolicy:     core.DNSDefault,
+				},
+			},
+		},
 		"mismatched AppArmor localhost profiles": {
 			expectedError: "Forbidden: apparmor profile in annotation and field must match",
 			spec: core.Pod{

pkg/registry/core/pod/strategy.go

@@ -27,6 +27,7 @@ import (
 	"time"
 
 	apiv1 "k8s.io/api/core/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
@@ -779,17 +780,16 @@ func applyAppArmorVersionSkew(pod *api.Pod) {
 			key := api.AppArmorContainerAnnotationKeyPrefix + ctr.Name
 			annotation, hasAnnotation := pod.Annotations[key]
 
-			field, hasField := (*api.AppArmorProfile)(nil), false
+			var containerProfile *api.AppArmorProfile
-			if ctr.SecurityContext != nil && ctr.SecurityContext.AppArmorProfile != nil {
+			if ctr.SecurityContext != nil {
-				field = ctr.SecurityContext.AppArmorProfile
+				containerProfile = ctr.SecurityContext.AppArmorProfile
-				hasField = true
 			}
 
 			// sync field and annotation
 			if !hasAnnotation {
 				newAnnotation := ""
-				if hasField {
+				if containerProfile != nil {
-					newAnnotation = appArmorAnnotationForField(field)
+					newAnnotation = appArmorAnnotationForField(containerProfile)
 				} else if podProfile != nil {
 					newAnnotation = appArmorAnnotationForField(podProfile)
 				}
@@ -800,10 +800,11 @@ func applyAppArmorVersionSkew(pod *api.Pod) {
 					}
 					pod.Annotations[key] = newAnnotation
 				}
-			} else if !hasField {
+			} else if containerProfile == nil {
 				newField := apparmorFieldForAnnotation(annotation)
 
-				if newField != nil {
+				// Only copy the annotation to the field if it is different from the pod-level profile.
+				if newField != nil && !apiequality.Semantic.DeepEqual(newField, podProfile) {
 					if ctr.SecurityContext == nil {
 						ctr.SecurityContext = &api.SecurityContext{}
 					}

pkg/registry/core/pod/strategy_test.go

@@ -2141,7 +2141,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Nil(t, pod.Spec.SecurityContext.AppArmorProfile)
 		},
 	}, {
-		description: "Field type unconfined and no annotation present",
+		description: "Pod field unconfined and no annotation present",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				SecurityContext: &api.PodSecurityContext{
@@ -2160,7 +2160,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			}, pod.Annotations)
 		},
 	}, {
-		description: "Field type default and no annotation present",
+		description: "Pod field default and no annotation present",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				SecurityContext: &api.PodSecurityContext{
@@ -2179,7 +2179,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			}, pod.Annotations)
 		},
 	}, {
-		description: "Field type localhost and no annotation present",
+		description: "Pod field localhost and no annotation present",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				SecurityContext: &api.PodSecurityContext{
@@ -2199,7 +2199,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			}, pod.Annotations)
 		},
 	}, {
-		description: "Field type localhost but profile is nil",
+		description: "Pod field localhost but profile is nil",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				SecurityContext: &api.PodSecurityContext{
@@ -2215,7 +2215,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Len(t, pod.Annotations, 0)
 		},
 	}, {
-		description: "Security context not nil (container)",
+		description: "Container security context not nil",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				Containers: []api.Container{{
@@ -2228,7 +2228,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Len(t, pod.Annotations, 0)
 		},
 	}, {
-		description: "Field type RuntimeDefault and no annotation present (container)",
+		description: "Container field RuntimeDefault and no annotation present",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				Containers: []api.Container{{
@@ -2249,7 +2249,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.Type)
 		},
 	}, {
-		description: "Field type localhost and no annotation present (container)",
+		description: "Container field localhost and no annotation present",
 		pod: &api.Pod{
 			Spec: api.PodSpec{
 				Containers: []api.Container{{
@@ -2341,7 +2341,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.Containers[2].SecurityContext.AppArmorProfile.Type)
 		},
 	}, {
-		description: "Annotation 'unconfined' and no field present (container)",
+		description: "Annotation 'unconfined' and no fields present",
 		pod: &api.Pod{
 			ObjectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
@@ -2380,7 +2380,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Nil(t, pod.Spec.SecurityContext)
 		},
 	}, {
-		description: "Annotation 'runtime/default' and no field present (container)",
+		description: "Annotation 'runtime/default' and no fields present",
 		pod: &api.Pod{
 			ObjectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
@@ -2408,7 +2408,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.SecurityContext.AppArmorProfile.Type)
 		},
 	}, {
-		description: "Multiple containers by annotations (container)",
+		description: "Multiple containers by annotations",
 		pod: &api.Pod{
 			ObjectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
@@ -2442,7 +2442,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Equal(t, api.AppArmorProfileTypeLocalhost, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.Type)
 			assert.Equal(t, testProfile, *pod.Spec.Containers[0].SecurityContext.AppArmorProfile.LocalhostProfile)
 			assert.Nil(t, pod.Spec.Containers[1].SecurityContext)
-			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.Containers[2].SecurityContext.AppArmorProfile.Type)
+			assert.Nil(t, pod.Spec.Containers[2].SecurityContext)
 			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.SecurityContext.AppArmorProfile.Type)
 		},
 	}, {
@@ -2472,6 +2472,98 @@ func TestApplyAppArmorVersionSkew(t *testing.T) {
 			assert.Nil(t, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.LocalhostProfile)
 			assert.Nil(t, pod.Spec.SecurityContext)
 		},
+	}, {
+		description: "Pod field and matching annotations",
+		pod: &api.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					api.AppArmorContainerAnnotationKeyPrefix + "ctr": api.AppArmorProfileRuntimeDefault,
+				},
+			},
+			Spec: api.PodSpec{
+				SecurityContext: &api.PodSecurityContext{
+					AppArmorProfile: &api.AppArmorProfile{
+						Type: api.AppArmorProfileTypeRuntimeDefault,
+					},
+				},
+				Containers: []api.Container{{
+					Name: "ctr",
+				}},
+			},
+		},
+		validation: func(t *testing.T, pod *api.Pod) {
+			assert.Equal(t, map[string]string{
+				api.AppArmorContainerAnnotationKeyPrefix + "ctr": api.AppArmorProfileRuntimeDefault,
+			}, pod.Annotations)
+			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.SecurityContext.AppArmorProfile.Type)
+			// Annotation shouldn't be synced to container security context
+			assert.Nil(t, pod.Spec.Containers[0].SecurityContext)
+		},
+	}, {
+		description: "Annotation overrides pod field",
+		pod: &api.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					api.AppArmorContainerAnnotationKeyPrefix + "ctr": api.AppArmorProfileNameUnconfined,
+				},
+			},
+			Spec: api.PodSpec{
+				SecurityContext: &api.PodSecurityContext{
+					AppArmorProfile: &api.AppArmorProfile{
+						Type: api.AppArmorProfileTypeRuntimeDefault,
+					},
+				},
+				Containers: []api.Container{{
+					Name: "ctr",
+				}},
+			},
+		},
+		validation: func(t *testing.T, pod *api.Pod) {
+			assert.Equal(t, map[string]string{
+				api.AppArmorContainerAnnotationKeyPrefix + "ctr": api.AppArmorProfileNameUnconfined,
+			}, pod.Annotations)
+			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.SecurityContext.AppArmorProfile.Type)
+			assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.Type)
+		},
+	}, {
+		description: "Mixed annotations and fields",
+		pod: &api.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					api.AppArmorContainerAnnotationKeyPrefix + "unconf-annot": api.AppArmorProfileNameUnconfined,
+				},
+			},
+			Spec: api.PodSpec{
+				SecurityContext: &api.PodSecurityContext{
+					AppArmorProfile: &api.AppArmorProfile{
+						Type: api.AppArmorProfileTypeRuntimeDefault,
+					},
+				},
+				Containers: []api.Container{{
+					Name: "unconf-annot",
+				}, {
+					Name: "unconf-field",
+					SecurityContext: &api.SecurityContext{
+						AppArmorProfile: &api.AppArmorProfile{
+							Type: api.AppArmorProfileTypeUnconfined,
+						},
+					},
+				}, {
+					Name: "default-pod",
+				}},
+			},
+		},
+		validation: func(t *testing.T, pod *api.Pod) {
+			assert.Equal(t, map[string]string{
+				api.AppArmorContainerAnnotationKeyPrefix + "unconf-annot": api.AppArmorProfileNameUnconfined,
+				api.AppArmorContainerAnnotationKeyPrefix + "unconf-field": api.AppArmorProfileNameUnconfined,
+				api.AppArmorContainerAnnotationKeyPrefix + "default-pod":  api.AppArmorProfileRuntimeDefault,
+			}, pod.Annotations)
+			assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.SecurityContext.AppArmorProfile.Type)
+			assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.Type)
+			assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.Containers[1].SecurityContext.AppArmorProfile.Type)
+			assert.Nil(t, pod.Spec.Containers[2].SecurityContext)
+		},
 	}, {
 		description: "Invalid annotation value",
 		pod: &api.Pod{