diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index d5479dfaeca..35d1eb7ddc6 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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. -func validateAppArmorAnnotationsAndFields(objectMeta metav1.ObjectMeta, podSpec *core.PodSpec, specPath *field.Path) field.ErrorList { +// validateAppArmorAnnotationsAndFieldsMatchOnCreate validates that AppArmor fields and annotations are consistent. +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 - if c.SecurityContext != nil { - field = c.SecurityContext.AppArmorProfile + containerProfile := podProfile + if c.SecurityContext != nil && c.SecurityContext.AppArmorProfile != nil { + 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 { - allErrs = append(allErrs, err) + + switch containerProfile.Type { + 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 } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d87015678d5..9e722e30aa4 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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{ diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index e2c6a204efc..346688387dc 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/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 - if ctr.SecurityContext != nil && ctr.SecurityContext.AppArmorProfile != nil { - field = ctr.SecurityContext.AppArmorProfile - hasField = true + var containerProfile *api.AppArmorProfile + if ctr.SecurityContext != nil { + containerProfile = ctr.SecurityContext.AppArmorProfile } // sync field and annotation if !hasAnnotation { newAnnotation := "" - if hasField { - newAnnotation = appArmorAnnotationForField(field) + if containerProfile != nil { + 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{} } diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index 19c8f6a1739..e7ac79a201e 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/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{