From 4e1d4caa8fd8bc2bb34b7f7890bc17e309e9f7e1 Mon Sep 17 00:00:00 2001 From: Rajath Agasthya Date: Tue, 8 Jan 2019 14:26:21 -0800 Subject: [PATCH] Move PodShareProcessNamespace feature gate out of validation --- pkg/api/pod/util.go | 16 ++++ pkg/api/pod/util_test.go | 100 ++++++++++++++++++++ pkg/apis/core/validation/validation.go | 8 +- pkg/apis/core/validation/validation_test.go | 20 ---- 4 files changed, 118 insertions(+), 26 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 750464faab5..592888034fb 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -293,6 +293,12 @@ func dropDisabledFields( } } + if !utilfeature.DefaultFeatureGate.Enabled(features.PodShareProcessNamespace) && !shareProcessNamespaceInUse(oldPodSpec) { + if podSpec.SecurityContext != nil { + podSpec.SecurityContext.ShareProcessNamespace = nil + } + } + 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. @@ -454,6 +460,16 @@ func appArmorInUse(podAnnotations map[string]string) bool { return false } +func shareProcessNamespaceInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + if podSpec.SecurityContext != nil && podSpec.SecurityContext.ShareProcessNamespace != nil { + 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 52d8805e232..8f2f082bc51 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -914,6 +914,106 @@ func TestDropEmptyDirSizeLimit(t *testing.T) { } } +func TestDropPodShareProcessNamespace(t *testing.T) { + podWithShareProcessNamespace := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{ + ShareProcessNamespace: &[]bool{true}[0], + }, + }, + } + } + podWithoutShareProcessNamespace := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{}, + }, + } + } + podWithoutSecurityContext := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{}, + } + } + + podInfo := []struct { + description string + hasShareProcessNamespace bool + pod func() *api.Pod + }{ + { + description: "has ShareProcessNamespace", + hasShareProcessNamespace: true, + pod: podWithShareProcessNamespace, + }, + { + description: "does not have ShareProcessNamespace", + hasShareProcessNamespace: false, + pod: podWithoutShareProcessNamespace, + }, + { + description: "does not have SecurityContext", + hasShareProcessNamespace: false, + pod: podWithoutSecurityContext, + }, + { + description: "is nil", + hasShareProcessNamespace: false, + pod: func() *api.Pod { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasShareProcessNamespace, oldPod := oldPodInfo.hasShareProcessNamespace, oldPodInfo.pod() + newPodHasShareProcessNamespace, newPod := newPodInfo.hasShareProcessNamespace, 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.PodShareProcessNamespace, enabled)() + + var oldPodSpec *api.PodSpec + if oldPod != nil { + oldPodSpec = &oldPod.Spec + } + dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil) + + // 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 || oldPodHasShareProcessNamespace: + // new pod should not be changed if the feature is enabled, or if the old pod had ShareProcessNamespace set + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + case newPodHasShareProcessNamespace: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have ShareProcessNamespace + if !reflect.DeepEqual(newPod, podWithoutShareProcessNamespace()) { + t.Errorf("new pod had ShareProcessNamespace: %v", diff.ObjectReflectDiff(newPod, podWithoutShareProcessNamespace())) + } + 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 TestDropAppArmor(t *testing.T) { podWithAppArmor := func() *api.Pod { return &api.Pod{ diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 44da6a3bd92..089fe8fcb9a 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3439,12 +3439,8 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec * allErrs = append(allErrs, field.Invalid(fldPath.Child("supplementalGroups").Index(g), gid, msg)) } } - if securityContext.ShareProcessNamespace != nil { - if !utilfeature.DefaultFeatureGate.Enabled(features.PodShareProcessNamespace) { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("shareProcessNamespace"), "Process Namespace Sharing is disabled by PodShareProcessNamespace feature-gate")) - } else if securityContext.HostPID && *securityContext.ShareProcessNamespace { - allErrs = append(allErrs, field.Invalid(fldPath.Child("shareProcessNamespace"), *securityContext.ShareProcessNamespace, "ShareProcessNamespace and HostPID cannot both be enabled")) - } + if securityContext.ShareProcessNamespace != nil && securityContext.HostPID && *securityContext.ShareProcessNamespace { + allErrs = append(allErrs, field.Invalid(fldPath.Child("shareProcessNamespace"), *securityContext.ShareProcessNamespace, "ShareProcessNamespace and HostPID cannot both be enabled")) } if len(securityContext.Sysctls) != 0 { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index df88524842a..de218a5e155 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -5917,7 +5917,6 @@ func TestValidatePodSpec(t *testing.T) { maxGroupID := int64(2147483647) defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodPriority, true)() - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodShareProcessNamespace, true)() defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RuntimeClass, true)() successCases := []core.PodSpec{ @@ -6252,25 +6251,6 @@ func TestValidatePodSpec(t *testing.T) { t.Errorf("expected failure for %q", k) } } - - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodShareProcessNamespace, false)() - - featuregatedCases := map[string]core.PodSpec{ - "set ShareProcessNamespace": { - Volumes: []core.Volume{{Name: "vol", VolumeSource: core.VolumeSource{EmptyDir: &core.EmptyDirVolumeSource{}}}}, - Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - SecurityContext: &core.PodSecurityContext{ - ShareProcessNamespace: &[]bool{true}[0], - }, - }, - } - for k, v := range featuregatedCases { - if errs := ValidatePodSpec(&v, field.NewPath("field")); len(errs) == 0 { - t.Errorf("expected failure due to gated feature: %q", k) - } - } } func extendPodSpecwithTolerations(in core.PodSpec, tolerations []core.Toleration) core.PodSpec {