diff --git a/pkg/api/pod/BUILD b/pkg/api/pod/BUILD index 4bdc4c39d5..4210587bc8 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 1c2c6a9fe0..750464faab 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 @@ -232,9 +235,64 @@ func UpdatePodCondition(status *api.PodStatus, condition *api.PodCondition) bool return !isEqual } -// DropDisabledFields removes disabled fields from the pod spec. -// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pod spec. -func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) { +// DropDisabledTemplateFields removes disabled fields from the pod template metadata and spec. +// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a PodTemplateSpec +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 { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index eca0309b86..52d8805e23 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) { @@ -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()) { diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 2b56826fbb..cfd9bd7493 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")) diff --git a/pkg/registry/apps/daemonset/strategy.go b/pkg/registry/apps/daemonset/strategy.go index 08ffd63f73..4edba00eec 100644 --- a/pkg/registry/apps/daemonset/strategy.go +++ b/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 diff --git a/pkg/registry/apps/deployment/strategy.go b/pkg/registry/apps/deployment/strategy.go index cccd62116f..f5e9cfe711 100644 --- a/pkg/registry/apps/deployment/strategy.go +++ b/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 diff --git a/pkg/registry/apps/replicaset/strategy.go b/pkg/registry/apps/replicaset/strategy.go index 37bf07eaa3..72d7007498 100644 --- a/pkg/registry/apps/replicaset/strategy.go +++ b/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 diff --git a/pkg/registry/apps/statefulset/strategy.go b/pkg/registry/apps/statefulset/strategy.go index 1a8608900b..c3dc71d118 100644 --- a/pkg/registry/apps/statefulset/strategy.go +++ b/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. diff --git a/pkg/registry/batch/cronjob/strategy.go b/pkg/registry/batch/cronjob/strategy.go index 3ccbee4eb4..d23a5cdbdf 100644 --- a/pkg/registry/batch/cronjob/strategy.go +++ b/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. diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 5ce5e51178..b0a5e8a347 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/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. diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 52fb9ec45a..307870e1fb 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/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. diff --git a/pkg/registry/core/podtemplate/strategy.go b/pkg/registry/core/podtemplate/strategy.go index 13d841f782..8ce614cbf3 100644 --- a/pkg/registry/core/podtemplate/strategy.go +++ b/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. diff --git a/pkg/registry/core/replicationcontroller/strategy.go b/pkg/registry/core/replicationcontroller/strategy.go index 2c0cc34a40..7c05fee212 100644 --- a/pkg/registry/core/replicationcontroller/strategy.go +++ b/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.DropDisabledFields(&controller.Spec.Template.Spec, nil) - } + pod.DropDisabledTemplateFields(controller.Spec.Template, 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 - 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) + pod.DropDisabledTemplateFields(newController.Spec.Template, oldController.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