From 2d86cbf26168bca704689098eb6d5d03c2bdc45d Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 5 Mar 2024 17:04:36 -0800 Subject: [PATCH] Separate feature-gate for AppArmor fields --- pkg/api/pod/util.go | 20 +++--- pkg/api/pod/util_test.go | 85 ++++++++++++++++---------- pkg/apis/core/validation/validation.go | 3 + pkg/features/kube_features.go | 6 ++ pkg/registry/core/pod/strategy.go | 4 ++ pkg/security/apparmor/helpers.go | 6 ++ 6 files changed, 86 insertions(+), 38 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 82853271bb4..2a26008588b 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -539,12 +539,14 @@ func dropDisabledFields( podSpec = &api.PodSpec{} } - if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmor) && !appArmorInUse(oldPodAnnotations, oldPodSpec) { + if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmor) && !appArmorAnnotationsInUse(oldPodAnnotations) { for k := range podAnnotations { if strings.HasPrefix(k, api.DeprecatedAppArmorAnnotationKeyPrefix) { delete(podAnnotations, k) } } + } + if (!utilfeature.DefaultFeatureGate.Enabled(features.AppArmor) || !utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields)) && !appArmorFieldsInUse(oldPodSpec) { if podSpec.SecurityContext != nil { podSpec.SecurityContext.AppArmorProfile = nil } @@ -947,17 +949,21 @@ func procMountInUse(podSpec *api.PodSpec) bool { return inUse } -// appArmorInUse returns true if the pod has apparmor related information -func appArmorInUse(podAnnotations map[string]string, podSpec *api.PodSpec) bool { - if podSpec == nil { - return false - } - +// appArmorAnnotationsInUse returns true if the pod has apparmor annotations +func appArmorAnnotationsInUse(podAnnotations map[string]string) bool { for k := range podAnnotations { if strings.HasPrefix(k, api.DeprecatedAppArmorAnnotationKeyPrefix) { return true } } + return false +} + +// appArmorFieldsInUse returns true if the pod has apparmor fields set +func appArmorFieldsInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } if podSpec.SecurityContext != nil && podSpec.SecurityContext.AppArmorProfile != nil { return true } diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 8a3139adf15..e8025b2fe80 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -707,19 +707,34 @@ func TestDropProcMount(t *testing.T) { func TestDropAppArmor(t *testing.T) { tests := []struct { - description string - hasAppArmor bool - pod api.Pod + description string + hasAnnotations bool + hasFields bool + pod api.Pod }{{ - description: "with AppArmor Annotations", - hasAppArmor: true, + description: "with AppArmor Annotations", + hasAnnotations: true, pod: api.Pod{ ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1", v1.DeprecatedAppArmorBetaContainerAnnotationKeyPrefix + "foo": "default"}}, Spec: api.PodSpec{}, }, + }, { + description: "with AppArmor Annotations & fields", + hasAnnotations: true, + hasFields: true, + pod: api.Pod{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1", v1.DeprecatedAppArmorBetaContainerAnnotationKeyPrefix + "foo": "default"}}, + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{ + AppArmorProfile: &api.AppArmorProfile{ + Type: api.AppArmorProfileTypeRuntimeDefault, + }, + }, + }, + }, }, { description: "with pod AppArmor profile", - hasAppArmor: true, + hasFields: true, pod: api.Pod{ ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1"}}, Spec: api.PodSpec{ @@ -732,7 +747,7 @@ func TestDropAppArmor(t *testing.T) { }, }, { description: "with container AppArmor profile", - hasAppArmor: true, + hasFields: true, pod: api.Pod{ ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1"}}, Spec: api.PodSpec{ @@ -747,7 +762,6 @@ func TestDropAppArmor(t *testing.T) { }, }, { description: "without AppArmor", - hasAppArmor: false, pod: api.Pod{ ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"a": "1"}}, Spec: api.PodSpec{}, @@ -756,34 +770,43 @@ func TestDropAppArmor(t *testing.T) { for _, test := range tests { for _, enabled := range []bool{true, false} { - t.Run(fmt.Sprintf("%v/enabled=%v", test.description, enabled), func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AppArmor, enabled)() + for _, fieldsEnabled := range []bool{true, false} { + t.Run(fmt.Sprintf("%v/enabled=%v/fields=%v", test.description, enabled, fieldsEnabled), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AppArmor, enabled)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AppArmorFields, fieldsEnabled)() - newPod := test.pod.DeepCopy() + newPod := test.pod.DeepCopy() - if actual := appArmorInUse(newPod.Annotations, &newPod.Spec); actual != test.hasAppArmor { - t.Errorf("appArmorInUse does not match expectation: %t != %t", actual, test.hasAppArmor) - } - - DropDisabledPodFields(newPod, newPod) - require.Equal(t, &test.pod, newPod, "unchanged pod should never be mutated") - - DropDisabledPodFields(newPod, nil) - - if enabled { - assert.Equal(t, &test.pod, newPod, "pod should not be mutated when AppArmor is enabled") - } else { - if appArmorInUse(newPod.Annotations, &newPod.Spec) { - t.Errorf("newPod should not be using appArmor after dropping disabled fields") + if hasAnnotations := appArmorAnnotationsInUse(newPod.Annotations); hasAnnotations != test.hasAnnotations { + t.Errorf("appArmorAnnotationsInUse does not match expectation: %t != %t", hasAnnotations, test.hasAnnotations) + } + if hasFields := appArmorFieldsInUse(&newPod.Spec); hasFields != test.hasFields { + t.Errorf("appArmorFieldsInUse does not match expectation: %t != %t", hasFields, test.hasFields) } - if test.hasAppArmor { - assert.NotEqual(t, &test.pod, newPod, "pod should be mutated to drop AppArmor") - } else { - assert.Equal(t, &test.pod, newPod, "pod without AppArmor should not be mutated") + DropDisabledPodFields(newPod, newPod) + require.Equal(t, &test.pod, newPod, "unchanged pod should never be mutated") + + DropDisabledPodFields(newPod, nil) + + if enabled && fieldsEnabled { + assert.Equal(t, &test.pod, newPod, "pod should not be mutated when both feature gates are enabled") + return } - } - }) + + expectAnnotations := test.hasAnnotations && enabled + assert.Equal(t, expectAnnotations, appArmorAnnotationsInUse(newPod.Annotations), "AppArmor annotations expectation") + if expectAnnotations == test.hasAnnotations { + assert.Equal(t, test.pod.Annotations, newPod.Annotations, "annotations should not be mutated") + } + + expectFields := test.hasFields && enabled && fieldsEnabled + assert.Equal(t, expectFields, appArmorFieldsInUse(&newPod.Spec), "AppArmor fields expectation") + if expectFields == test.hasFields { + assert.Equal(t, &test.pod.Spec, &newPod.Spec, "PodSpec should not be mutated") + } + }) + } } } } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 762735a75d6..f99c9ce3dbf 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4736,6 +4736,9 @@ func ValidateAppArmorProfileFormat(profile string) error { // validateAppArmorAnnotationsAndFieldsMatchOnCreate validates that AppArmor fields and annotations are consistent. func validateAppArmorAnnotationsAndFieldsMatchOnCreate(objectMeta metav1.ObjectMeta, podSpec *core.PodSpec, specPath *field.Path) field.ErrorList { + if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields) { + return nil + } if podSpec.OS != nil && podSpec.OS.Name == core.Windows { // Skip consistency check for windows pods. return nil diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index c22193519c3..92d7be0c2c8 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -63,6 +63,10 @@ const ( // beta: v1.4 AppArmor featuregate.Feature = "AppArmor" + // owner: @tallclair + // beta: v1.30 + AppArmorFields featuregate.Feature = "AppArmorFields" + // owner: @danwinship // alpha: v1.27 // beta: v1.29 @@ -975,6 +979,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS AppArmor: {Default: true, PreRelease: featuregate.Beta}, + AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, + CloudDualStackNodeIPs: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 ClusterTrustBundle: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 927d15f69b0..0ce8332a4a5 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -764,6 +764,10 @@ func applySchedulingGatedCondition(pod *api.Pod) { // applyAppArmorVersionSkew implements the version skew behavior described in: // https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/24-apparmor#version-skew-strategy func applyAppArmorVersionSkew(pod *api.Pod) { + if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields) { + return + } + if pod.Spec.OS != nil && pod.Spec.OS.Name == api.Windows { return } diff --git a/pkg/security/apparmor/helpers.go b/pkg/security/apparmor/helpers.go index eeaa3955dd3..aa6ad5b4299 100644 --- a/pkg/security/apparmor/helpers.go +++ b/pkg/security/apparmor/helpers.go @@ -20,7 +20,9 @@ import ( "strings" v1 "k8s.io/api/core/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" podutil "k8s.io/kubernetes/pkg/api/v1/pod" + "k8s.io/kubernetes/pkg/features" ) // Checks whether app armor is required for the pod to run. AppArmor is considered required if any @@ -52,6 +54,10 @@ func isRequired(pod *v1.Pod) bool { // GetProfileName returns the name of the profile to use with the container. func GetProfile(pod *v1.Pod, container *v1.Container) *v1.AppArmorProfile { + if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields) { + return getProfileFromPodAnnotations(pod.Annotations, container.Name) + } + if container.SecurityContext != nil && container.SecurityContext.AppArmorProfile != nil { return container.SecurityContext.AppArmorProfile }