From b56da85a7758ed1eb787829e9151c4decf46fb85 Mon Sep 17 00:00:00 2001 From: Shihang Zhang Date: Fri, 20 Mar 2020 10:21:24 -0700 Subject: [PATCH] sync api/v1/pod/util with api/pod/util and remove DefaultContainers --- pkg/api/pod/BUILD | 1 + pkg/api/pod/util.go | 28 ++- pkg/api/pod/util_test.go | 227 +++++++++-------- pkg/api/v1/pod/BUILD | 1 + pkg/api/v1/pod/util.go | 63 +++-- pkg/api/v1/pod/util_test.go | 230 +++++++++++------- pkg/kubelet/container/helpers.go | 4 +- .../kuberuntime/kuberuntime_manager_test.go | 2 +- pkg/registry/core/pod/strategy.go | 4 +- pkg/security/apparmor/validate.go | 4 +- pkg/volume/util/nested_volumes.go | 4 +- pkg/volume/util/util.go | 2 +- plugin/pkg/admission/exec/admission.go | 2 +- test/e2e/framework/util.go | 2 +- 14 files changed, 350 insertions(+), 224 deletions(-) diff --git a/pkg/api/pod/BUILD b/pkg/api/pod/BUILD index a20e7b9cccd..f5d1531c4f3 100644 --- a/pkg/api/pod/BUILD +++ b/pkg/api/pod/BUILD @@ -47,5 +47,6 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", ], ) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 366ca87df29..0d12ea511d2 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -29,9 +29,6 @@ import ( // ContainerType signifies container type type ContainerType int -// DefaultContainers defines default behavior: Iterate containers based on feature gates -const DefaultContainers ContainerType = 0 - const ( // Containers is for normal containers Containers ContainerType = 1 << iota @@ -44,35 +41,40 @@ const ( // AllContainers specifies that all containers be visited const AllContainers ContainerType = (InitContainers | Containers | EphemeralContainers) +// AllFeatureEnabledContainers returns a ContainerType mask which includes all container +// types except for the ones guarded by feature gate. +func AllFeatureEnabledContainers() ContainerType { + containerType := AllContainers + if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) { + containerType &= ^EphemeralContainers + } + return containerType +} + // ContainerVisitor is called with each container spec, and returns true // if visiting should continue. type ContainerVisitor func(container *api.Container, containerType ContainerType) (shouldContinue bool) -// VisitContainers invokes the visitor function with a pointer to the container -// spec of every container in the given pod spec. If visitor returns false, +// VisitContainers invokes the visitor function with a pointer to every container +// spec in the given pod spec with type set in mask. If visitor returns false, // visiting is short-circuited. VisitContainers returns true if visiting completes, // false if visiting was short-circuited. -// -// With the default mask (zero value or DefaultContainers) VisitContainers will visit all containers -// enabled by current feature gates. If mask is non-zero, VisitContainers will unconditionally visit -// container types specified by mask, and no feature gate checks will be performed. func VisitContainers(podSpec *api.PodSpec, mask ContainerType, visitor ContainerVisitor) bool { - if mask == DefaultContainers || (mask&InitContainers) > 0 { + if mask&InitContainers != 0 { for i := range podSpec.InitContainers { if !visitor(&podSpec.InitContainers[i], InitContainers) { return false } } } - if mask == DefaultContainers || (mask&Containers) > 0 { + if mask&Containers != 0 { for i := range podSpec.Containers { if !visitor(&podSpec.Containers[i], Containers) { return false } } } - if (mask == DefaultContainers && utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers)) || - (mask&EphemeralContainers) > 0 { + if mask&EphemeralContainers != 0 { for i := range podSpec.EphemeralContainers { if !visitor((*api.Container)(&podSpec.EphemeralContainers[i].EphemeralContainerCommon), EphemeralContainers) { return false diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 9b4deb29211..4e103706364 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -22,6 +22,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" @@ -35,74 +36,22 @@ import ( ) func TestVisitContainers(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - testCases := []struct { - description string - haveSpec *api.PodSpec - wantNames []string - mask ContainerType + desc string + spec *api.PodSpec + wantContainers []string + mask ContainerType + ephemeralContainersEnabled bool }{ { - "empty podspec", - &api.PodSpec{}, - []string{}, - DefaultContainers, + desc: "empty podspec", + spec: &api.PodSpec{}, + wantContainers: []string{}, + mask: AllContainers, }, { - "regular containers", - &api.PodSpec{ - Containers: []api.Container{ - {Name: "c1"}, - {Name: "c2"}, - }, - }, - []string{"c1", "c2"}, - DefaultContainers, - }, - { - "init containers", - &api.PodSpec{ - InitContainers: []api.Container{ - {Name: "i1"}, - {Name: "i2"}, - }, - }, - []string{"i1", "i2"}, - DefaultContainers, - }, - { - "regular and init containers", - &api.PodSpec{ - Containers: []api.Container{ - {Name: "c1"}, - {Name: "c2"}, - }, - InitContainers: []api.Container{ - {Name: "i1"}, - {Name: "i2"}, - }, - }, - []string{"i1", "i2", "c1", "c2"}, - DefaultContainers, - }, - { - "ephemeral containers", - &api.PodSpec{ - Containers: []api.Container{ - {Name: "c1"}, - {Name: "c2"}, - }, - EphemeralContainers: []api.EphemeralContainer{ - {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e1"}}, - }, - }, - []string{"c1", "c2", "e1"}, - DefaultContainers, - }, - { - "all container types", - &api.PodSpec{ + desc: "regular containers", + spec: &api.PodSpec{ Containers: []api.Container{ {Name: "c1"}, {Name: "c2"}, @@ -116,12 +65,12 @@ func TestVisitContainers(t *testing.T) { {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2"}}, }, }, - []string{"i1", "i2", "c1", "c2", "e1", "e2"}, - DefaultContainers, + wantContainers: []string{"c1", "c2"}, + mask: Containers, }, { - "all container types with init and regular container types chosen", - &api.PodSpec{ + desc: "init containers", + spec: &api.PodSpec{ Containers: []api.Container{ {Name: "c1"}, {Name: "c2"}, @@ -135,12 +84,89 @@ func TestVisitContainers(t *testing.T) { {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2"}}, }, }, - []string{"i1", "i2", "c1", "c2"}, - Containers | InitContainers, + wantContainers: []string{"i1", "i2"}, + mask: InitContainers, }, { - "dropping fields", - &api.PodSpec{ + desc: "ephemeral containers", + spec: &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []api.EphemeralContainer{ + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"e1", "e2"}, + mask: EphemeralContainers, + }, + { + desc: "all container types", + spec: &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []api.EphemeralContainer{ + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + mask: AllContainers, + }, + { + desc: "all feature enabled container types with ephemeral containers disabled", + spec: &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []api.EphemeralContainer{ + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2", "c1", "c2"}, + mask: AllFeatureEnabledContainers(), + }, + { + desc: "all feature enabled container types with ephemeral containers enabled", + spec: &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2", SecurityContext: &api.SecurityContext{}}, + }, + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2", SecurityContext: &api.SecurityContext{}}, + }, + EphemeralContainers: []api.EphemeralContainer{ + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + mask: AllFeatureEnabledContainers(), + ephemeralContainersEnabled: true, + }, + { + desc: "dropping fields", + spec: &api.PodSpec{ Containers: []api.Container{ {Name: "c1"}, {Name: "c2", SecurityContext: &api.SecurityContext{}}, @@ -154,38 +180,45 @@ func TestVisitContainers(t *testing.T) { {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "e2", SecurityContext: &api.SecurityContext{}}}, }, }, - []string{"i1", "i2", "c1", "c2", "e1", "e2"}, - DefaultContainers, + wantContainers: []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + mask: AllContainers, }, } for _, tc := range testCases { - gotNames := []string{} - VisitContainers(tc.haveSpec, tc.mask, func(c *api.Container, containerType ContainerType) bool { - gotNames = append(gotNames, c.Name) - if c.SecurityContext != nil { - c.SecurityContext = nil + t.Run(tc.desc, func(t *testing.T) { + if tc.ephemeralContainersEnabled { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, tc.ephemeralContainersEnabled)() + tc.mask = AllFeatureEnabledContainers() + } + + gotContainers := []string{} + VisitContainers(tc.spec, tc.mask, func(c *api.Container, containerType ContainerType) bool { + gotContainers = append(gotContainers, c.Name) + if c.SecurityContext != nil { + c.SecurityContext = nil + } + return true + }) + if !cmp.Equal(gotContainers, tc.wantContainers) { + t.Errorf("VisitContainers() = %+v, want %+v", gotContainers, tc.wantContainers) + } + for _, c := range tc.spec.Containers { + if c.SecurityContext != nil { + t.Errorf("VisitContainers() did not drop SecurityContext for container %q", c.Name) + } + } + for _, c := range tc.spec.InitContainers { + if c.SecurityContext != nil { + t.Errorf("VisitContainers() did not drop SecurityContext for init container %q", c.Name) + } + } + for _, c := range tc.spec.EphemeralContainers { + if c.SecurityContext != nil { + t.Errorf("VisitContainers() did not drop SecurityContext for ephemeral container %q", c.Name) + } } - return true }) - if !reflect.DeepEqual(gotNames, tc.wantNames) { - t.Errorf("VisitContainers() for test case %q visited containers %q, wanted to visit %q", tc.description, gotNames, tc.wantNames) - } - for _, c := range tc.haveSpec.Containers { - if c.SecurityContext != nil { - t.Errorf("VisitContainers() for test case %q: got SecurityContext %#v for container %v, wanted nil", tc.description, c.SecurityContext, c.Name) - } - } - for _, c := range tc.haveSpec.InitContainers { - if c.SecurityContext != nil { - t.Errorf("VisitContainers() for test case %q: got SecurityContext %#v for init container %v, wanted nil", tc.description, c.SecurityContext, c.Name) - } - } - for _, c := range tc.haveSpec.EphemeralContainers { - if c.SecurityContext != nil { - t.Errorf("VisitContainers() for test case %q: got SecurityContext %#v for ephemeral container %v, wanted nil", tc.description, c.SecurityContext, c.Name) - } - } } } diff --git a/pkg/api/v1/pod/BUILD b/pkg/api/v1/pod/BUILD index 3402380e7eb..c5c52c6db34 100644 --- a/pkg/api/v1/pod/BUILD +++ b/pkg/api/v1/pod/BUILD @@ -32,6 +32,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/pkg/api/v1/pod/util.go b/pkg/api/v1/pod/util.go index 1457e37ed86..90c3d34f506 100644 --- a/pkg/api/v1/pod/util.go +++ b/pkg/api/v1/pod/util.go @@ -50,28 +50,60 @@ func FindPort(pod *v1.Pod, svcPort *v1.ServicePort) (int, error) { return 0, fmt.Errorf("no suitable port for manifest: %s", pod.UID) } +// ContainerType signifies container type +type ContainerType int + +const ( + // Containers is for normal containers + Containers ContainerType = 1 << iota + // InitContainers is for init containers + InitContainers + // EphemeralContainers is for ephemeral containers + EphemeralContainers +) + +// AllContainers specifies that all containers be visited +const AllContainers ContainerType = (InitContainers | Containers | EphemeralContainers) + +// AllFeatureEnabledContainers returns a ContainerType mask which includes all container +// types except for the ones guarded by feature gate. +func AllFeatureEnabledContainers() ContainerType { + containerType := AllContainers + if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) { + containerType &= ^EphemeralContainers + } + return containerType +} + // ContainerVisitor is called with each container spec, and returns true // if visiting should continue. -type ContainerVisitor func(container *v1.Container) (shouldContinue bool) +type ContainerVisitor func(container *v1.Container, containerType ContainerType) (shouldContinue bool) -// VisitContainers invokes the visitor function with a pointer to the container -// spec of every container in the given pod spec. If visitor returns false, +// Visitor is called with each object name, and returns true if visiting should continue +type Visitor func(name string) (shouldContinue bool) + +// VisitContainers invokes the visitor function with a pointer to every container +// spec in the given pod spec with type set in mask. If visitor returns false, // visiting is short-circuited. VisitContainers returns true if visiting completes, // false if visiting was short-circuited. -func VisitContainers(podSpec *v1.PodSpec, visitor ContainerVisitor) bool { - for i := range podSpec.InitContainers { - if !visitor(&podSpec.InitContainers[i]) { - return false +func VisitContainers(podSpec *v1.PodSpec, mask ContainerType, visitor ContainerVisitor) bool { + if mask&InitContainers != 0 { + for i := range podSpec.InitContainers { + if !visitor(&podSpec.InitContainers[i], InitContainers) { + return false + } } } - for i := range podSpec.Containers { - if !visitor(&podSpec.Containers[i]) { - return false + if mask&Containers != 0 { + for i := range podSpec.Containers { + if !visitor(&podSpec.Containers[i], Containers) { + return false + } } } - if utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) { + if mask&EphemeralContainers != 0 { for i := range podSpec.EphemeralContainers { - if !visitor((*v1.Container)(&podSpec.EphemeralContainers[i].EphemeralContainerCommon)) { + if !visitor((*v1.Container)(&podSpec.EphemeralContainers[i].EphemeralContainerCommon), EphemeralContainers) { return false } } @@ -79,9 +111,6 @@ func VisitContainers(podSpec *v1.PodSpec, visitor ContainerVisitor) bool { return true } -// Visitor is called with each object name, and returns true if visiting should continue -type Visitor func(name string) (shouldContinue bool) - // VisitPodSecretNames invokes the visitor function with the name of every secret // referenced by the pod spec. If visitor returns false, visiting is short-circuited. // Transitive references (e.g. pod -> pvc -> pv -> secret) are not visited. @@ -92,7 +121,7 @@ func VisitPodSecretNames(pod *v1.Pod, visitor Visitor) bool { return false } } - VisitContainers(&pod.Spec, func(c *v1.Container) bool { + VisitContainers(&pod.Spec, AllContainers, func(c *v1.Container, containerType ContainerType) bool { return visitContainerSecretNames(c, visitor) }) var source *v1.VolumeSource @@ -176,7 +205,7 @@ func visitContainerSecretNames(container *v1.Container, visitor Visitor) bool { // Transitive references (e.g. pod -> pvc -> pv -> secret) are not visited. // Returns true if visiting completed, false if visiting was short-circuited. func VisitPodConfigmapNames(pod *v1.Pod, visitor Visitor) bool { - VisitContainers(&pod.Spec, func(c *v1.Container) bool { + VisitContainers(&pod.Spec, AllContainers, func(c *v1.Container, containerType ContainerType) bool { return visitContainerConfigmapNames(c, visitor) }) var source *v1.VolumeSource diff --git a/pkg/api/v1/pod/util_test.go b/pkg/api/v1/pod/util_test.go index d3f8ebdea62..1f86e4753f7 100644 --- a/pkg/api/v1/pod/util_test.go +++ b/pkg/api/v1/pod/util_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -202,68 +203,22 @@ func TestFindPort(t *testing.T) { } func TestVisitContainers(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - testCases := []struct { - description string - haveSpec *v1.PodSpec - wantNames []string + desc string + spec *v1.PodSpec + wantContainers []string + mask ContainerType + ephemeralContainersEnabled bool }{ { - "empty podspec", - &v1.PodSpec{}, - []string{}, + desc: "empty podspec", + spec: &v1.PodSpec{}, + wantContainers: []string{}, + mask: AllContainers, }, { - "regular containers", - &v1.PodSpec{ - Containers: []v1.Container{ - {Name: "c1"}, - {Name: "c2"}, - }, - }, - []string{"c1", "c2"}, - }, - { - "init containers", - &v1.PodSpec{ - InitContainers: []v1.Container{ - {Name: "i1"}, - {Name: "i2"}, - }, - }, - []string{"i1", "i2"}, - }, - { - "regular and init containers", - &v1.PodSpec{ - Containers: []v1.Container{ - {Name: "c1"}, - {Name: "c2"}, - }, - InitContainers: []v1.Container{ - {Name: "i1"}, - {Name: "i2"}, - }, - }, - []string{"i1", "i2", "c1", "c2"}, - }, - { - "ephemeral containers", - &v1.PodSpec{ - Containers: []v1.Container{ - {Name: "c1"}, - {Name: "c2"}, - }, - EphemeralContainers: []v1.EphemeralContainer{ - {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e1"}}, - }, - }, - []string{"c1", "c2", "e1"}, - }, - { - "all container types", - &v1.PodSpec{ + desc: "regular containers", + spec: &v1.PodSpec{ Containers: []v1.Container{ {Name: "c1"}, {Name: "c2"}, @@ -277,11 +232,108 @@ func TestVisitContainers(t *testing.T) { {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2"}}, }, }, - []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + wantContainers: []string{"c1", "c2"}, + mask: Containers, }, { - "dropping fields", - &v1.PodSpec{ + desc: "init containers", + spec: &v1.PodSpec{ + Containers: []v1.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []v1.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []v1.EphemeralContainer{ + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2"}, + mask: InitContainers, + }, + { + desc: "ephemeral containers", + spec: &v1.PodSpec{ + Containers: []v1.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []v1.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []v1.EphemeralContainer{ + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"e1", "e2"}, + mask: EphemeralContainers, + }, + { + desc: "all container types", + spec: &v1.PodSpec{ + Containers: []v1.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []v1.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []v1.EphemeralContainer{ + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + mask: AllContainers, + }, + { + desc: "all feature enabled container types with ephemeral containers disabled", + spec: &v1.PodSpec{ + Containers: []v1.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []v1.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + EphemeralContainers: []v1.EphemeralContainer{ + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2", "c1", "c2"}, + mask: AllFeatureEnabledContainers(), + }, + { + desc: "all feature enabled container types with ephemeral containers enabled", + spec: &v1.PodSpec{ + Containers: []v1.Container{ + {Name: "c1"}, + {Name: "c2", SecurityContext: &v1.SecurityContext{}}, + }, + InitContainers: []v1.Container{ + {Name: "i1"}, + {Name: "i2", SecurityContext: &v1.SecurityContext{}}, + }, + EphemeralContainers: []v1.EphemeralContainer{ + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e1"}}, + {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2"}}, + }, + }, + wantContainers: []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + mask: AllFeatureEnabledContainers(), + ephemeralContainersEnabled: true, + }, + { + desc: "dropping fields", + spec: &v1.PodSpec{ Containers: []v1.Container{ {Name: "c1"}, {Name: "c2", SecurityContext: &v1.SecurityContext{}}, @@ -295,37 +347,45 @@ func TestVisitContainers(t *testing.T) { {EphemeralContainerCommon: v1.EphemeralContainerCommon{Name: "e2", SecurityContext: &v1.SecurityContext{}}}, }, }, - []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + wantContainers: []string{"i1", "i2", "c1", "c2", "e1", "e2"}, + mask: AllContainers, }, } for _, tc := range testCases { - gotNames := []string{} - VisitContainers(tc.haveSpec, func(c *v1.Container) bool { - gotNames = append(gotNames, c.Name) - if c.SecurityContext != nil { - c.SecurityContext = nil + t.Run(tc.desc, func(t *testing.T) { + if tc.ephemeralContainersEnabled { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, tc.ephemeralContainersEnabled)() + tc.mask = AllFeatureEnabledContainers() + } + + gotContainers := []string{} + VisitContainers(tc.spec, tc.mask, func(c *v1.Container, containerType ContainerType) bool { + gotContainers = append(gotContainers, c.Name) + if c.SecurityContext != nil { + c.SecurityContext = nil + } + return true + }) + if !cmp.Equal(gotContainers, tc.wantContainers) { + t.Errorf("VisitContainers() = %+v, want %+v", gotContainers, tc.wantContainers) + } + for _, c := range tc.spec.Containers { + if c.SecurityContext != nil { + t.Errorf("VisitContainers() did not drop SecurityContext for container %q", c.Name) + } + } + for _, c := range tc.spec.InitContainers { + if c.SecurityContext != nil { + t.Errorf("VisitContainers() did not drop SecurityContext for init container %q", c.Name) + } + } + for _, c := range tc.spec.EphemeralContainers { + if c.SecurityContext != nil { + t.Errorf("VisitContainers() did not drop SecurityContext for ephemeral container %q", c.Name) + } } - return true }) - if !reflect.DeepEqual(gotNames, tc.wantNames) { - t.Errorf("VisitContainers() for test case %q visited containers %q, wanted to visit %q", tc.description, gotNames, tc.wantNames) - } - for _, c := range tc.haveSpec.Containers { - if c.SecurityContext != nil { - t.Errorf("VisitContainers() for test case %q: got SecurityContext %#v for container %v, wanted nil", tc.description, c.SecurityContext, c.Name) - } - } - for _, c := range tc.haveSpec.InitContainers { - if c.SecurityContext != nil { - t.Errorf("VisitContainers() for test case %q: got SecurityContext %#v for init container %v, wanted nil", tc.description, c.SecurityContext, c.Name) - } - } - for _, c := range tc.haveSpec.EphemeralContainers { - if c.SecurityContext != nil { - t.Errorf("VisitContainers() for test case %q: got SecurityContext %#v for ephemeral container %v, wanted nil", tc.description, c.SecurityContext, c.Name) - } - } } } diff --git a/pkg/kubelet/container/helpers.go b/pkg/kubelet/container/helpers.go index b15be2d7500..4299ce90274 100644 --- a/pkg/kubelet/container/helpers.go +++ b/pkg/kubelet/container/helpers.go @@ -283,7 +283,7 @@ func FormatPod(pod *Pod) string { // GetContainerSpec gets the container spec by containerName. func GetContainerSpec(pod *v1.Pod, containerName string) *v1.Container { var containerSpec *v1.Container - podutil.VisitContainers(&pod.Spec, func(c *v1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(c *v1.Container, containerType podutil.ContainerType) bool { if containerName == c.Name { containerSpec = c return false @@ -296,7 +296,7 @@ func GetContainerSpec(pod *v1.Pod, containerName string) *v1.Container { // HasPrivilegedContainer returns true if any of the containers in the pod are privileged. func HasPrivilegedContainer(pod *v1.Pod) bool { var hasPrivileged bool - podutil.VisitContainers(&pod.Spec, func(c *v1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(c *v1.Container, containerType podutil.ContainerType) bool { if c.SecurityContext != nil && c.SecurityContext.Privileged != nil && *c.SecurityContext.Privileged { hasPrivileged = true return false diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index 16a4cc467b2..8cf4b279a19 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -101,7 +101,7 @@ func makeAndSetFakePod(t *testing.T, m *kubeGenericRuntimeManager, fakeRuntime * state: runtimeapi.ContainerState_CONTAINER_RUNNING, } } - podutil.VisitContainers(&pod.Spec, func(c *v1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(c *v1.Container, containerType podutil.ContainerType) bool { containers = append(containers, makeFakeContainer(t, m, newTemplate(c))) return true }) diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index b02bafd0e5d..22fa5a1d134 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -387,7 +387,7 @@ func LogLocation( func podHasContainerWithName(pod *api.Pod, containerName string) bool { var hasContainer bool - podutil.VisitContainers(&pod.Spec, podutil.DefaultContainers, func(c *api.Container, containerType podutil.ContainerType) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(c *api.Container, containerType podutil.ContainerType) bool { if c.Name == containerName { hasContainer = true return false @@ -554,7 +554,7 @@ func validateContainer(container string, pod *api.Pod) (string, error) { return "", errors.NewBadRequest(fmt.Sprintf("a container name must be specified for pod %s", pod.Name)) default: var containerNames []string - podutil.VisitContainers(&pod.Spec, podutil.DefaultContainers, func(c *api.Container, containerType podutil.ContainerType) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(c *api.Container, containerType podutil.ContainerType) bool { containerNames = append(containerNames, c.Name) return true }) diff --git a/pkg/security/apparmor/validate.go b/pkg/security/apparmor/validate.go index 1742b4e040d..f66ff1ce1f2 100644 --- a/pkg/security/apparmor/validate.go +++ b/pkg/security/apparmor/validate.go @@ -25,7 +25,7 @@ import ( "path" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" podutil "k8s.io/kubernetes/pkg/api/v1/pod" "k8s.io/kubernetes/pkg/features" @@ -79,7 +79,7 @@ func (v *validator) Validate(pod *v1.Pod) error { } var retErr error - podutil.VisitContainers(&pod.Spec, func(container *v1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllContainers, func(container *v1.Container, containerType podutil.ContainerType) bool { retErr = validateProfile(GetProfileName(pod, container.Name), loadedProfiles) if retErr != nil { return false diff --git a/pkg/volume/util/nested_volumes.go b/pkg/volume/util/nested_volumes.go index 5a728a8397a..51a53d79a18 100644 --- a/pkg/volume/util/nested_volumes.go +++ b/pkg/volume/util/nested_volumes.go @@ -23,7 +23,7 @@ import ( "sort" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" podutil "k8s.io/kubernetes/pkg/api/v1/pod" ) @@ -73,7 +73,7 @@ func getNestedMountpoints(name, baseDir string, pod v1.Pod) ([]string, error) { } var retErr error - podutil.VisitContainers(&pod.Spec, func(c *v1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(c *v1.Container, containerType podutil.ContainerType) bool { retErr = checkContainer(c) if retErr != nil { return false diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 2f2ce7ef0f9..e0b3458b940 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -592,7 +592,7 @@ func GetPodVolumeNames(pod *v1.Pod) (mounts sets.String, devices sets.String) { mounts = sets.NewString() devices = sets.NewString() - podutil.VisitContainers(&pod.Spec, func(container *v1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllFeatureEnabledContainers(), func(container *v1.Container, containerType podutil.ContainerType) bool { if container.VolumeMounts != nil { for _, mount := range container.VolumeMounts { mounts.Insert(mount.Name) diff --git a/plugin/pkg/admission/exec/admission.go b/plugin/pkg/admission/exec/admission.go index 1a0051dd978..b96e5f231d7 100644 --- a/plugin/pkg/admission/exec/admission.go +++ b/plugin/pkg/admission/exec/admission.go @@ -149,7 +149,7 @@ func (d *DenyExec) Validate(ctx context.Context, a admission.Attributes, o admis // isPrivileged will return true a pod has any privileged containers func isPrivileged(pod *corev1.Pod) bool { var privileged bool - podutil.VisitContainers(&pod.Spec, func(c *corev1.Container) bool { + podutil.VisitContainers(&pod.Spec, podutil.AllContainers, func(c *corev1.Container, containerType podutil.ContainerType) bool { if c.SecurityContext == nil || c.SecurityContext.Privileged == nil { return true } diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 52b3840f48e..28add2415f8 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -819,7 +819,7 @@ func (f *Framework) MatchContainerOutput( if podErr != nil { // Pod failed. Dump all logs from all containers to see what's wrong - _ = podutil.VisitContainers(&podStatus.Spec, func(c *v1.Container) bool { + _ = podutil.VisitContainers(&podStatus.Spec, podutil.AllFeatureEnabledContainers(), func(c *v1.Container, containerType podutil.ContainerType) bool { logs, err := e2epod.GetPodLogs(f.ClientSet, ns, podStatus.Name, c.Name) if err != nil { Logf("Failed to get logs from node %q pod %q container %q: %v",