diff --git a/pkg/api/pod/BUILD b/pkg/api/pod/BUILD index 4bdc4c39d56..4210587bc89 100644 --- a/pkg/api/pod/BUILD +++ b/pkg/api/pod/BUILD @@ -13,6 +13,7 @@ go_library( deps = [ "//pkg/apis/core:go_default_library", "//pkg/features:go_default_library", + "//pkg/security/apparmor:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], @@ -38,7 +39,9 @@ go_test( deps = [ "//pkg/apis/core:go_default_library", "//pkg/features:go_default_library", + "//pkg/security/apparmor:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 8723e59c41e..750464faab5 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -17,10 +17,13 @@ limitations under the License. package pod import ( + "strings" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" + "k8s.io/kubernetes/pkg/security/apparmor" ) // Visitor is called with each object name, and returns true if visiting should continue @@ -282,6 +285,14 @@ func dropDisabledFields( podSpec = &api.PodSpec{} } + if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmor) && !appArmorInUse(oldPodAnnotations) { + for k := range podAnnotations { + if strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) { + delete(podAnnotations, k) + } + } + } + if !utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) && !podPriorityInUse(oldPodSpec) { // Set to nil pod's priority fields if the feature is disabled and the old pod // does not specify any values for these fields. @@ -433,6 +444,16 @@ func procMountInUse(podSpec *api.PodSpec) bool { return false } +// appArmorInUse returns true if the pod has apparmor related information +func appArmorInUse(podAnnotations map[string]string) bool { + for k := range podAnnotations { + if strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) { + return true + } + } + return false +} + // podPriorityInUse returns true if the pod spec is non-nil and has Priority or PriorityClassName set. func podPriorityInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index dc1027dc9f2..52d8805e232 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -23,6 +23,7 @@ import ( "testing" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" @@ -30,6 +31,7 @@ import ( utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" + "k8s.io/kubernetes/pkg/security/apparmor" ) func TestPodSecrets(t *testing.T) { @@ -912,6 +914,88 @@ func TestDropEmptyDirSizeLimit(t *testing.T) { } } +func TestDropAppArmor(t *testing.T) { + podWithAppArmor := func() *api.Pod { + return &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1", apparmor.ContainerAnnotationKeyPrefix + "foo": "default"}}, + Spec: api.PodSpec{}, + } + } + podWithoutAppArmor := func() *api.Pod { + return &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1"}}, + Spec: api.PodSpec{}, + } + } + + podInfo := []struct { + description string + hasAppArmor bool + pod func() *api.Pod + }{ + { + description: "has AppArmor", + hasAppArmor: true, + pod: podWithAppArmor, + }, + { + description: "does not have AppArmor", + hasAppArmor: false, + pod: podWithoutAppArmor, + }, + { + description: "is nil", + hasAppArmor: false, + pod: func() *api.Pod { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasAppArmor, oldPod := oldPodInfo.hasAppArmor, oldPodInfo.pod() + newPodHasAppArmor, newPod := newPodInfo.hasAppArmor, newPodInfo.pod() + if newPod == nil { + continue + } + + t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AppArmor, enabled)() + + DropDisabledPodFields(newPod, oldPod) + + // old pod should never be changed + if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { + t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod())) + } + + switch { + case enabled || oldPodHasAppArmor: + // new pod should not be changed if the feature is enabled, or if the old pod had AppArmor + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + case newPodHasAppArmor: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have AppArmor + if !reflect.DeepEqual(newPod, podWithoutAppArmor()) { + t.Errorf("new pod had EmptyDir SizeLimit: %v", diff.ObjectReflectDiff(newPod, podWithoutAppArmor())) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + } + }) + } + } + } +} + func TestDropRunAsGroup(t *testing.T) { group := func() *int64 { testGroup := int64(1000) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 2b56826fbb3..cfd9bd7493e 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3348,11 +3348,6 @@ func ValidateAppArmorPodAnnotations(annotations map[string]string, spec *core.Po if !strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) { continue } - // TODO: this belongs to admission, not general pod validation: - if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmor) { - allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "AppArmor is disabled by feature-gate")) - continue - } containerName := strings.TrimPrefix(k, apparmor.ContainerAnnotationKeyPrefix) if !podSpecHasContainer(spec, containerName) { allErrs = append(allErrs, field.Invalid(fldPath.Key(k), containerName, "container not found"))