From 68dadd40d638c814d96977442751eaccd31d98c1 Mon Sep 17 00:00:00 2001 From: Raisaat Rashid Date: Wed, 23 Jun 2021 18:24:35 -0400 Subject: [PATCH] Fix pkg/api/pod/util tests to ensure feature gate is set Fixing this led to finding a bug in how the TestDropProbeGracePeriod unit tests were written, so this patch also includes a fix for that. Co-Authored-By: Elana Hashman --- pkg/api/pod/util_test.go | 184 ++++++++++++++++++++------------------- 1 file changed, 95 insertions(+), 89 deletions(-) diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index a11439a5256..cf8cb5ae593 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -1090,62 +1090,66 @@ func TestDropSubPathExpr(t *testing.T) { }, } - enabled := true - for _, oldPodInfo := range podInfo { - for _, newPodInfo := range podInfo { - oldPodHasSubpaths, oldPod := oldPodInfo.hasSubpaths, oldPodInfo.pod() - newPodHasSubpaths, newPod := newPodInfo.hasSubpaths, newPodInfo.pod() - if newPod == nil { - continue + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasSubpaths, oldPod := oldPodInfo.hasSubpaths, oldPodInfo.pod() + newPodHasSubpaths, newPod := newPodInfo.hasSubpaths, 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.VolumeSubpath, 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", cmp.Diff(oldPod, oldPodInfo.pod())) + } + + switch { + case enabled || oldPodHasSubpaths: + // 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", cmp.Diff(newPod, newPodInfo.pod())) + } + case newPodHasSubpaths: + // 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, podWithoutSubpaths()) { + t.Errorf("new pod had subpaths: %v", cmp.Diff(newPod, podWithoutSubpaths())) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) + } + } + }) } - - t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) { - - 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", cmp.Diff(oldPod, oldPodInfo.pod())) - } - - switch { - case enabled || oldPodHasSubpaths: - // 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", cmp.Diff(newPod, newPodInfo.pod())) - } - case newPodHasSubpaths: - // 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, podWithoutSubpaths()) { - t.Errorf("new pod had subpaths: %v", cmp.Diff(newPod, podWithoutSubpaths())) - } - default: - // new pod should not need to be changed - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - } - }) } } } func TestDropProbeGracePeriod(t *testing.T) { - gracePeriod := int64(10) - probe := api.Probe{TerminationGracePeriodSeconds: &gracePeriod} podWithProbeGracePeriod := func() *api.Pod { + livenessGracePeriod := int64(10) + livenessProbe := api.Probe{TerminationGracePeriodSeconds: &livenessGracePeriod} + startupGracePeriod := int64(10) + startupProbe := api.Probe{TerminationGracePeriodSeconds: &startupGracePeriod} return &api.Pod{ Spec: api.PodSpec{ RestartPolicy: api.RestartPolicyNever, - Containers: []api.Container{{Name: "container1", Image: "testimage", LivenessProbe: &probe, StartupProbe: &probe}}, + Containers: []api.Container{{Name: "container1", Image: "testimage", LivenessProbe: &livenessProbe, StartupProbe: &startupProbe}}, }, } } @@ -1187,50 +1191,52 @@ func TestDropProbeGracePeriod(t *testing.T) { }, } - enabled := true - for _, oldPodInfo := range podInfo { - for _, newPodInfo := range podInfo { - oldPodHasGracePeriod, oldPod := oldPodInfo.hasGracePeriod, oldPodInfo.pod() - newPodHasGracePeriod, newPod := newPodInfo.hasGracePeriod, newPodInfo.pod() - if newPod == nil { - continue + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasGracePeriod, oldPod := oldPodInfo.hasGracePeriod, oldPodInfo.pod() + newPodHasGracePeriod, newPod := newPodInfo.hasGracePeriod, 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.ProbeTerminationGracePeriod, 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", cmp.Diff(oldPod, oldPodInfo.pod())) + } + + switch { + case enabled || oldPodHasGracePeriod: + // new pod should not be changed if the feature is enabled, or if the old pod had terminationGracePeriod + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) + } + case newPodHasGracePeriod: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have terminationGracePeriod + if !reflect.DeepEqual(newPod, podWithoutProbeGracePeriod()) { + t.Errorf("new pod had probe-level terminationGracePeriod: %v", cmp.Diff(newPod, podWithoutProbeGracePeriod())) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) + } + } + }) } - - t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) { - - 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", cmp.Diff(oldPod, oldPodInfo.pod())) - } - - switch { - case enabled || oldPodHasGracePeriod: - // 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", cmp.Diff(newPod, newPodInfo.pod())) - } - case newPodHasGracePeriod: - // 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, podWithoutProbeGracePeriod()) { - t.Errorf("new pod had probe-level terminationGracePeriod: %v", cmp.Diff(newPod, podWithoutProbeGracePeriod())) - } - default: - // new pod should not need to be changed - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - } - }) } } }