Separate feature-gate for AppArmor fields

Author: tallclair

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
 	}

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)
-				}
+					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)
+					}
 
-				DropDisabledPodFields(newPod, newPod)
-				require.Equal(t, &test.pod, newPod, "unchanged pod should never be mutated")
+					DropDisabledPodFields(newPod, newPod)
+					require.Equal(t, &test.pod, newPod, "unchanged pod should never be mutated")
 
-				DropDisabledPodFields(newPod, nil)
+					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 enabled && fieldsEnabled {
+						assert.Equal(t, &test.pod, newPod, "pod should not be mutated when both feature gates are enabled")
+						return
 					}
 
-					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")
+					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")
+					}
+				})
+			}
 		}
 	}
 }

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

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},

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
 	}

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
 	}