Merge pull request #72655 from liggitt/pod-annotation-features

Move AppArmor feature-gate checking out of validation

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

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
@@ -232,9 +235,64 @@ func UpdatePodCondition(status *api.PodStatus, condition *api.PodCondition) bool
 	return !isEqual
 }
 
-// DropDisabledFields removes disabled fields from the pod spec.
+// DropDisabledTemplateFields removes disabled fields from the pod template metadata and spec.
-// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pod spec.
+// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a PodTemplateSpec
-func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) {
+func DropDisabledTemplateFields(podTemplate, oldPodTemplate *api.PodTemplateSpec) {
+	var (
+		podSpec           *api.PodSpec
+		podAnnotations    map[string]string
+		oldPodSpec        *api.PodSpec
+		oldPodAnnotations map[string]string
+	)
+	if podTemplate != nil {
+		podSpec = &podTemplate.Spec
+		podAnnotations = podTemplate.Annotations
+	}
+	if oldPodTemplate != nil {
+		oldPodSpec = &oldPodTemplate.Spec
+		oldPodAnnotations = oldPodTemplate.Annotations
+	}
+	dropDisabledFields(podSpec, podAnnotations, oldPodSpec, oldPodAnnotations)
+}
+
+// DropDisabledPodFields removes disabled fields from the pod metadata and spec.
+// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a Pod
+func DropDisabledPodFields(pod, oldPod *api.Pod) {
+	var (
+		podSpec           *api.PodSpec
+		podAnnotations    map[string]string
+		oldPodSpec        *api.PodSpec
+		oldPodAnnotations map[string]string
+	)
+	if pod != nil {
+		podSpec = &pod.Spec
+		podAnnotations = pod.Annotations
+	}
+	if oldPod != nil {
+		oldPodSpec = &oldPod.Spec
+		oldPodAnnotations = oldPod.Annotations
+	}
+	dropDisabledFields(podSpec, podAnnotations, oldPodSpec, oldPodAnnotations)
+}
+
+// dropDisabledFields removes disabled fields from the pod metadata and spec.
+func dropDisabledFields(
+	podSpec *api.PodSpec, podAnnotations map[string]string,
+	oldPodSpec *api.PodSpec, oldPodAnnotations map[string]string,
+) {
+	// the new spec must always be non-nil
+	if podSpec == nil {
+		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.
@@ -386,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 {

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) {
@@ -375,7 +377,7 @@ func TestDropAlphaVolumeDevices(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
@@ -469,7 +471,7 @@ func TestDropSubPath(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
@@ -558,7 +560,7 @@ func TestDropRuntimeClass(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
@@ -652,7 +654,7 @@ func TestDropProcMount(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
@@ -768,7 +770,7 @@ func TestDropPodPriority(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
@@ -878,7 +880,7 @@ func TestDropEmptyDirSizeLimit(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
@@ -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)
@@ -1013,7 +1097,7 @@ func TestDropRunAsGroup(t *testing.T) {
 					if oldPod != nil {
 						oldPodSpec = &oldPod.Spec
 					}
-					DropDisabledFields(&newPod.Spec, oldPodSpec)
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
 
 					// old pod should never be changed
 					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {

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"))

pkg/registry/apps/daemonset/strategy.go

@@ -75,7 +75,7 @@ func (daemonSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Objec
 		daemonSet.Spec.TemplateGeneration = 1
 	}
 
-	pod.DropDisabledFields(&daemonSet.Spec.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&daemonSet.Spec.Template, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -83,7 +83,7 @@ func (daemonSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.
 	newDaemonSet := obj.(*apps.DaemonSet)
 	oldDaemonSet := old.(*apps.DaemonSet)
 
-	pod.DropDisabledFields(&newDaemonSet.Spec.Template.Spec, &oldDaemonSet.Spec.Template.Spec)
+	pod.DropDisabledTemplateFields(&newDaemonSet.Spec.Template, &oldDaemonSet.Spec.Template)
 
 	// update is not allowed to set status
 	newDaemonSet.Status = oldDaemonSet.Status

pkg/registry/apps/deployment/strategy.go

@@ -73,7 +73,7 @@ func (deploymentStrategy) PrepareForCreate(ctx context.Context, obj runtime.Obje
 	deployment.Status = apps.DeploymentStatus{}
 	deployment.Generation = 1
 
-	pod.DropDisabledFields(&deployment.Spec.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&deployment.Spec.Template, nil)
 }
 
 // Validate validates a new deployment.
@@ -97,7 +97,7 @@ func (deploymentStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime
 	oldDeployment := old.(*apps.Deployment)
 	newDeployment.Status = oldDeployment.Status
 
-	pod.DropDisabledFields(&newDeployment.Spec.Template.Spec, &oldDeployment.Spec.Template.Spec)
+	pod.DropDisabledTemplateFields(&newDeployment.Spec.Template, &oldDeployment.Spec.Template)
 
 	// Spec updates bump the generation so that we can distinguish between
 	// scaling events and template changes, annotation updates bump the generation

pkg/registry/apps/replicaset/strategy.go

@@ -80,7 +80,7 @@ func (rsStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 
 	rs.Generation = 1
 
-	pod.DropDisabledFields(&rs.Spec.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&rs.Spec.Template, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -90,7 +90,7 @@ func (rsStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object)
 	// update is not allowed to set status
 	newRS.Status = oldRS.Status
 
-	pod.DropDisabledFields(&newRS.Spec.Template.Spec, &oldRS.Spec.Template.Spec)
+	pod.DropDisabledTemplateFields(&newRS.Spec.Template, &oldRS.Spec.Template)
 
 	// Any changes to the spec increment the generation number, any changes to the
 	// status should reflect the generation number of the corresponding object. We push

pkg/registry/apps/statefulset/strategy.go

@@ -72,7 +72,7 @@ func (statefulSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Obj
 
 	statefulSet.Generation = 1
 
-	pod.DropDisabledFields(&statefulSet.Spec.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&statefulSet.Spec.Template, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -82,7 +82,7 @@ func (statefulSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtim
 	// Update is not allowed to set status
 	newStatefulSet.Status = oldStatefulSet.Status
 
-	pod.DropDisabledFields(&newStatefulSet.Spec.Template.Spec, &oldStatefulSet.Spec.Template.Spec)
+	pod.DropDisabledTemplateFields(&newStatefulSet.Spec.Template, &oldStatefulSet.Spec.Template)
 
 	// Any changes to the spec increment the generation number, any changes to the
 	// status should reflect the generation number of the corresponding object.

pkg/registry/batch/cronjob/strategy.go

@@ -68,7 +68,7 @@ func (cronJobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object)
 	cronJob := obj.(*batch.CronJob)
 	cronJob.Status = batch.CronJobStatus{}
 
-	pod.DropDisabledFields(&cronJob.Spec.JobTemplate.Spec.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&cronJob.Spec.JobTemplate.Spec.Template, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -77,7 +77,7 @@ func (cronJobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob
 	oldCronJob := old.(*batch.CronJob)
 	newCronJob.Status = oldCronJob.Status
 
-	pod.DropDisabledFields(&newCronJob.Spec.JobTemplate.Spec.Template.Spec, &oldCronJob.Spec.JobTemplate.Spec.Template.Spec)
+	pod.DropDisabledTemplateFields(&newCronJob.Spec.JobTemplate.Spec.Template, &oldCronJob.Spec.JobTemplate.Spec.Template)
 }
 
 // Validate validates a new scheduled job.

pkg/registry/batch/job/strategy.go

@@ -80,7 +80,7 @@ func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 		job.Spec.TTLSecondsAfterFinished = nil
 	}
 
-	pod.DropDisabledFields(&job.Spec.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&job.Spec.Template, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -93,7 +93,7 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object
 		newJob.Spec.TTLSecondsAfterFinished = nil
 	}
 
-	pod.DropDisabledFields(&newJob.Spec.Template.Spec, &oldJob.Spec.Template.Spec)
+	pod.DropDisabledTemplateFields(&newJob.Spec.Template, &oldJob.Spec.Template)
 }
 
 // Validate validates a new job.

pkg/registry/core/pod/strategy.go

@@ -73,7 +73,7 @@ func (podStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 		QOSClass: qos.GetPodQOS(pod),
 	}
 
-	podutil.DropDisabledFields(&pod.Spec, nil)
+	podutil.DropDisabledPodFields(pod, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -82,7 +82,7 @@ func (podStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object
 	oldPod := old.(*api.Pod)
 	newPod.Status = oldPod.Status
 
-	podutil.DropDisabledFields(&newPod.Spec, &oldPod.Spec)
+	podutil.DropDisabledPodFields(newPod, oldPod)
 }
 
 // Validate validates a new pod.

pkg/registry/core/podtemplate/strategy.go

@@ -47,7 +47,7 @@ func (podTemplateStrategy) NamespaceScoped() bool {
 func (podTemplateStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 	template := obj.(*api.PodTemplate)
 
-	pod.DropDisabledFields(&template.Template.Spec, nil)
+	pod.DropDisabledTemplateFields(&template.Template, nil)
 }
 
 // Validate validates a new pod template.
@@ -70,7 +70,7 @@ func (podTemplateStrategy) PrepareForUpdate(ctx context.Context, obj, old runtim
 	newTemplate := obj.(*api.PodTemplate)
 	oldTemplate := old.(*api.PodTemplate)
 
-	pod.DropDisabledFields(&newTemplate.Template.Spec, &oldTemplate.Template.Spec)
+	pod.DropDisabledTemplateFields(&newTemplate.Template, &oldTemplate.Template)
 }
 
 // ValidateUpdate is the default update validation for an end user.

pkg/registry/core/replicationcontroller/strategy.go

@@ -80,9 +80,7 @@ func (rcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 
 	controller.Generation = 1
 
-	if controller.Spec.Template != nil {
+	pod.DropDisabledTemplateFields(controller.Spec.Template, nil)
-		pod.DropDisabledFields(&controller.Spec.Template.Spec, nil)
-	}
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -92,16 +90,7 @@ func (rcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object)
 	// update is not allowed to set status
 	newController.Status = oldController.Status
 
-	var newSpec, oldSpec *api.PodSpec
+	pod.DropDisabledTemplateFields(newController.Spec.Template, oldController.Spec.Template)
-	if oldController.Spec.Template != nil {
-		oldSpec = &oldController.Spec.Template.Spec
-	}
-	if newController.Spec.Template != nil {
-		newSpec = &newController.Spec.Template.Spec
-	} else {
-		newSpec = &api.PodSpec{}
-	}
-	pod.DropDisabledFields(newSpec, oldSpec)
 
 	// Any changes to the spec increment the generation number, any changes to the
 	// status should reflect the generation number of the corresponding object. We push