diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 4c4cabe181f..3f0a00ebdd1 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -369,7 +369,7 @@ func dropDisabledFields( return true }) } - if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) { + if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) && !ephemeralContainersInUse(oldPodSpec) { podSpec.EphemeralContainers = nil } @@ -537,6 +537,13 @@ func dropDisabledCSIVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) { } } +func ephemeralContainersInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + return len(podSpec.EphemeralContainers) > 0 +} + // subpathInUse returns true if the pod spec is non-nil and has a volume mount that makes use of the subPath feature func subpathInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index dd5cacfe3ca..daaa1a14a77 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -2085,3 +2085,92 @@ func TestDropStatusPodIPs(t *testing.T) { }() } } + +func TestDropEphemeralContainers(t *testing.T) { + podWithEphemeralContainers := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + EphemeralContainers: []api.EphemeralContainer{{EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "container1", Image: "testimage"}}}, + }, + } + } + podWithoutEphemeralContainers := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + }, + } + } + + podInfo := []struct { + description string + hasEphemeralContainers bool + pod func() *api.Pod + }{ + { + description: "has subpaths", + hasEphemeralContainers: true, + pod: podWithEphemeralContainers, + }, + { + description: "does not have subpaths", + hasEphemeralContainers: false, + pod: podWithoutEphemeralContainers, + }, + { + description: "is nil", + hasEphemeralContainers: false, + pod: func() *api.Pod { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasEphemeralContainers, oldPod := oldPodInfo.hasEphemeralContainers, oldPodInfo.pod() + newPodHasEphemeralContainers, newPod := newPodInfo.hasEphemeralContainers, 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 featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, 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 || oldPodHasEphemeralContainers: + // new pod should not be changed if the feature is enabled, or if the old pod had subpaths + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + case newPodHasEphemeralContainers: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have subpaths + if !reflect.DeepEqual(newPod, podWithoutEphemeralContainers()) { + t.Errorf("new pod had subpaths: %v", diff.ObjectReflectDiff(newPod, podWithoutEphemeralContainers())) + } + 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())) + } + } + }) + } + } + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index a819a701b4e..fd41a417871 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2614,11 +2614,6 @@ func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, return allErrs } - // Return early if EphemeralContainers disabled - if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) { - return append(allErrs, field.Forbidden(fldPath, "disabled by EphemeralContainers feature-gate")) - } - allNames := sets.String{} for _, c := range containers { allNames.Insert(c.Name) @@ -3230,7 +3225,7 @@ func ValidatePodSpec(spec *core.PodSpec, fldPath *field.Path) field.ErrorList { allErrs = append(allErrs, ValidatePreemptionPolicy(spec.PreemptionPolicy, fldPath.Child("preemptionPolicy"))...) } - if spec.Overhead != nil && utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) { + if spec.Overhead != nil { allErrs = append(allErrs, validateOverhead(spec.Overhead, fldPath.Child("overhead"))...) } @@ -4281,7 +4276,7 @@ func ValidatePodTemplateSpec(spec *core.PodTemplateSpec, fldPath *field.Path) fi allErrs = append(allErrs, ValidatePodSpecificAnnotations(spec.Annotations, &spec.Spec, fldPath.Child("annotations"))...) allErrs = append(allErrs, ValidatePodSpec(&spec.Spec, fldPath.Child("spec"))...) - if utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) && len(spec.Spec.EphemeralContainers) > 0 { + if len(spec.Spec.EphemeralContainers) > 0 { allErrs = append(allErrs, field.Forbidden(fldPath.Child("spec", "ephemeralContainers"), "ephemeral containers not allowed in pod template")) } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 7ea25cd1339..8be54ccd507 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -5528,8 +5528,6 @@ func getResourceLimits(cpu, memory string) core.ResourceList { } func TestValidateEphemeralContainers(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - containers := []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}} initContainers := []core.Container{{Name: "ictr", Image: "iimage", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}} vols := map[string]core.VolumeSource{"vol": {EmptyDir: &core.EmptyDirVolumeSource{}}} @@ -6567,10 +6565,6 @@ func TestValidatePodSpec(t *testing.T) { minGroupID := int64(0) maxGroupID := int64(2147483647) - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RuntimeClass, true)() - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodOverhead, true)() - successCases := []core.PodSpec{ { // Populate basic fields, leave defaults for most. Volumes: []core.Volume{{Name: "vol", VolumeSource: core.VolumeSource{EmptyDir: &core.EmptyDirVolumeSource{}}}}, @@ -6910,34 +6904,6 @@ func TestValidatePodSpec(t *testing.T) { t.Errorf("expected failure for %q", k) } } - - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, false)() - - featuregatedCases := map[string]core.PodSpec{ - "disabled by EphemeralContainers feature-gate": { - Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - EphemeralContainers: []core.EphemeralContainer{ - { - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debug", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - }, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - }, - } - for expectedErr, spec := range featuregatedCases { - errs := ValidatePodSpec(&spec, field.NewPath("field")) - if len(errs) == 0 { - t.Errorf("expected failure due to gated feature: %s\n%+v", expectedErr, spec) - } else if actualErr := errs.ToAggregate().Error(); !strings.Contains(actualErr, expectedErr) { - t.Errorf("unexpected error message for gated feature. Expected error: %s\nActual error: %s", expectedErr, actualErr) - } - } } func extendPodSpecwithTolerations(in core.PodSpec, tolerations []core.Toleration) core.PodSpec { @@ -9190,8 +9156,6 @@ func makeValidService() core.Service { } func TestValidatePodEphemeralContainersUpdate(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - tests := []struct { new []core.EphemeralContainer old []core.EphemeralContainer @@ -10559,8 +10523,6 @@ func TestValidateReplicationControllerUpdate(t *testing.T) { } func TestValidateReplicationController(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - validSelector := map[string]string{"a": "b"} validPodTemplate := core.PodTemplate{ Template: core.PodTemplateSpec{ @@ -14379,8 +14341,6 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { } func TestValidateOverhead(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodOverhead, true)() - successCase := []struct { Name string overhead core.ResourceList diff --git a/pkg/apis/node/validation/BUILD b/pkg/apis/node/validation/BUILD index 233cda296f5..5439a0eb0fb 100644 --- a/pkg/apis/node/validation/BUILD +++ b/pkg/apis/node/validation/BUILD @@ -9,10 +9,8 @@ go_library( "//pkg/apis/core:go_default_library", "//pkg/apis/core/validation:go_default_library", "//pkg/apis/node:go_default_library", - "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/apis/node/validation/validation.go b/pkg/apis/node/validation/validation.go index d329bd7fe80..64131ae9dbd 100644 --- a/pkg/apis/node/validation/validation.go +++ b/pkg/apis/node/validation/validation.go @@ -19,11 +19,9 @@ package validation import ( apivalidation "k8s.io/apimachinery/pkg/api/validation" "k8s.io/apimachinery/pkg/util/validation/field" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/apis/core" corevalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/node" - "k8s.io/kubernetes/pkg/features" ) // ValidateRuntimeClass validates the RuntimeClass @@ -34,7 +32,7 @@ func ValidateRuntimeClass(rc *node.RuntimeClass) field.ErrorList { allErrs = append(allErrs, field.Invalid(field.NewPath("handler"), rc.Handler, msg)) } - if rc.Overhead != nil && utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) { + if rc.Overhead != nil { allErrs = append(allErrs, validateOverhead(rc.Overhead, field.NewPath("overhead"))...) }