From d64165c8033ed0bfcdec2d1404a9208f34d1c70b Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 18 Feb 2021 16:51:32 +0100 Subject: [PATCH] generic ephemeral volumes: fix and test apiserver feature gate The implementation should have preserved an existing ephemeral volume source during an update even when the feature gate is currently disabled, but due to a cut-and-paste error it was checking for CSI volumes instead. The new test detected that. It's based on https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3 --- pkg/api/pod/util.go | 15 +++- pkg/registry/core/pod/strategy_test.go | 96 +++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 0694bc2e925..4f0f7a78a2f 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -565,7 +565,7 @@ func dropDisabledCSIVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) { // dropDisabledEphemeralVolumeSourceAlphaFields removes disabled alpha fields from []EphemeralVolumeSource. // This should be called from PrepareForCreate/PrepareForUpdate for all pod specs resources containing a EphemeralVolumeSource func dropDisabledEphemeralVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) { - if !utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) && !csiInUse(oldPodSpec) { + if !utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) && !ephemeralInUse(oldPodSpec) { for i := range podSpec.Volumes { podSpec.Volumes[i].Ephemeral = nil } @@ -711,6 +711,19 @@ func csiInUse(podSpec *api.PodSpec) bool { return false } +// ephemeralInUse returns true if any pod's spec include inline CSI volumes. +func ephemeralInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + for i := range podSpec.Volumes { + if podSpec.Volumes[i].Ephemeral != nil { + return true + } + } + return false +} + // podPriorityInUse returns true if status is not nil and number of PodIPs is greater than one func multiplePodIPsInUse(podStatus *api.PodStatus) bool { if podStatus == nil { diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index f7f0dad477b..77616b9f341 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -25,7 +25,7 @@ import ( "testing" "github.com/stretchr/testify/require" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1103,3 +1103,97 @@ func TestApplySeccompVersionSkew(t *testing.T) { test.validation(t, test.pod) } } + +// TestEphemeralVolumeEnablement checks the behavior of the API server +// when the GenericEphemeralVolume feature is turned on and then off: +// the Ephemeral struct must be preserved even during updates. +func TestEphemeralVolumeEnablement(t *testing.T) { + // Enable the Feature Gate during the first pod creation + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, true)() + + pod := createPodWithGenericEphemeralVolume() + expectedPod := pod.DeepCopy() + + Strategy.PrepareForCreate(context.Background(), pod) + require.Equal(t, expectedPod.Spec, pod.Spec, "pod spec") + + errs := Strategy.Validate(context.Background(), pod) + require.Empty(t, errs, "errors from validation") + + // Now let's disable the Feature Gate, update some other field from the Pod and expect the volume to remain present + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, false)() + updatePod := testUpdatePod(t, pod, "aaa") + + // And let's enable the FG again, add another from and check if the volume is still present + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, true)() + testUpdatePod(t, updatePod, "bbb") +} + +// TestEphemeralVolumeDisabled checks the behavior of the API server +// when the GenericEphemeralVolume is off: the Ephemeral struct gets dropped, +// validation fails. +func TestEphemeralVolumeDisabled(t *testing.T) { + // Disable the Feature Gate during the first pod creation + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, false)() + + pod := createPodWithGenericEphemeralVolume() + expectedPod := pod.DeepCopy() + expectedPod.Spec.Volumes[0].VolumeSource.Ephemeral = nil + + Strategy.PrepareForCreate(context.Background(), pod) + require.Equal(t, expectedPod.Spec, pod.Spec, "pod spec") + + errs := Strategy.Validate(context.Background(), pod) + require.NotEmpty(t, errs, "no errors from validation") +} + +func testUpdatePod(t *testing.T, oldPod *api.Pod, labelValue string) *api.Pod { + updatedPod := oldPod.DeepCopy() + updatedPod.Labels = map[string]string{"XYZ": labelValue} + expectedPod := updatedPod.DeepCopy() + Strategy.PrepareForUpdate(context.Background(), updatedPod, oldPod) + require.Equal(t, expectedPod.Spec, updatedPod.Spec, "updated pod spec") + errs := Strategy.Validate(context.Background(), updatedPod) + require.Empty(t, errs, "errors from validation") + return updatedPod +} + +func createPodWithGenericEphemeralVolume() *api.Pod { + return &api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "pod", + }, + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyAlways, + DNSPolicy: api.DNSClusterFirst, + Containers: []api.Container{{ + Name: "foo", + Image: "example", + TerminationMessagePolicy: api.TerminationMessageReadFile, + ImagePullPolicy: api.PullAlways, + }}, + Volumes: []api.Volume{ + { + Name: "ephemeral", + VolumeSource: api.VolumeSource{ + Ephemeral: &api.EphemeralVolumeSource{ + VolumeClaimTemplate: &api.PersistentVolumeClaimTemplate{ + Spec: api.PersistentVolumeClaimSpec{ + AccessModes: []api.PersistentVolumeAccessMode{ + api.ReadWriteOnce, + }, + Resources: api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceStorage: resource.MustParse("1Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + } +}