From ee821e2a044bb14e7a734898f211593591548c90 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Fri, 14 Jun 2019 15:20:16 +0000 Subject: [PATCH 1/2] Create helpers for iterating containers in a pod --- pkg/api/pod/util.go | 244 ++++++++---------- pkg/api/pod/util_test.go | 86 ++++++ pkg/api/v1/pod/util.go | 48 ++-- pkg/api/v1/pod/util_test.go | 86 ++++++ pkg/apis/core/pods/helpers.go | 18 ++ pkg/apis/core/pods/helpers_test.go | 62 +++++ pkg/apis/core/validation/validation.go | 14 +- pkg/registry/core/pod/strategy.go | 16 +- pkg/security/apparmor/validate.go | 19 +- pkg/security/podsecuritypolicy/provider.go | 54 ++-- pkg/volume/util/nested_volumes.go | 20 +- .../admission/alwayspullimages/admission.go | 37 +-- plugin/pkg/admission/exec/admission.go | 20 +- plugin/pkg/admission/podpreset/admission.go | 13 +- staging/publishing/import-restrictions.yaml | 1 + 15 files changed, 467 insertions(+), 271 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 7b7dbafda84..b42eee5dc93 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -26,6 +26,28 @@ import ( "k8s.io/kubernetes/pkg/security/apparmor" ) +// ContainerVisitor is called with each container spec, and returns true +// if visiting should continue. +type ContainerVisitor func(container *api.Container) (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, +// visiting is short-circuited. VisitContainers returns true if visiting completes, +// false if visiting was short-circuited. +func VisitContainers(podSpec *api.PodSpec, visitor ContainerVisitor) bool { + for i := range podSpec.InitContainers { + if !visitor(&podSpec.InitContainers[i]) { + return false + } + } + for i := range podSpec.Containers { + if !visitor(&podSpec.Containers[i]) { + return false + } + } + return true +} + // Visitor is called with each object name, and returns true if visiting should continue type Visitor func(name string) (shouldContinue bool) @@ -39,16 +61,9 @@ func VisitPodSecretNames(pod *api.Pod, visitor Visitor) bool { return false } } - for i := range pod.Spec.InitContainers { - if !visitContainerSecretNames(&pod.Spec.InitContainers[i], visitor) { - return false - } - } - for i := range pod.Spec.Containers { - if !visitContainerSecretNames(&pod.Spec.Containers[i], visitor) { - return false - } - } + VisitContainers(&pod.Spec, func(c *api.Container) bool { + return visitContainerSecretNames(c, visitor) + }) var source *api.VolumeSource for i := range pod.Spec.Volumes { source = &pod.Spec.Volumes[i].VolumeSource @@ -129,16 +144,9 @@ func visitContainerSecretNames(container *api.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 *api.Pod, visitor Visitor) bool { - for i := range pod.Spec.InitContainers { - if !visitContainerConfigmapNames(&pod.Spec.InitContainers[i], visitor) { - return false - } - } - for i := range pod.Spec.Containers { - if !visitContainerConfigmapNames(&pod.Spec.Containers[i], visitor) { - return false - } - } + VisitContainers(&pod.Spec, func(c *api.Container) bool { + return visitContainerConfigmapNames(c, visitor) + }) var source *api.VolumeSource for i := range pod.Spec.Volumes { source = &pod.Spec.Volumes[i].VolumeSource @@ -338,30 +346,22 @@ func dropDisabledFields( if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) && !subpathInUse(oldPodSpec) { // drop subpath from the pod if the feature is disabled and the old spec did not specify subpaths - for i := range podSpec.Containers { - for j := range podSpec.Containers[i].VolumeMounts { - podSpec.Containers[i].VolumeMounts[j].SubPath = "" + VisitContainers(podSpec, func(c *api.Container) bool { + for i := range c.VolumeMounts { + c.VolumeMounts[i].SubPath = "" } - } - for i := range podSpec.InitContainers { - for j := range podSpec.InitContainers[i].VolumeMounts { - podSpec.InitContainers[i].VolumeMounts[j].SubPath = "" - } - } + return true + }) } if (!utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) || !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpathEnvExpansion)) && !subpathExprInUse(oldPodSpec) { // drop subpath env expansion from the pod if either of the subpath features is disabled and the old spec did not specify subpath env expansion - for i := range podSpec.Containers { - for j := range podSpec.Containers[i].VolumeMounts { - podSpec.Containers[i].VolumeMounts[j].SubPathExpr = "" + VisitContainers(podSpec, func(c *api.Container) bool { + for i := range c.VolumeMounts { + c.VolumeMounts[i].SubPathExpr = "" } - } - for i := range podSpec.InitContainers { - for j := range podSpec.InitContainers[i].VolumeMounts { - podSpec.InitContainers[i].VolumeMounts[j].SubPathExpr = "" - } - } + return true + }) } dropDisabledVolumeDevicesFields(podSpec, oldPodSpec) @@ -399,16 +399,12 @@ func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) { if podSpec.SecurityContext != nil { podSpec.SecurityContext.RunAsGroup = nil } - for i := range podSpec.Containers { - if podSpec.Containers[i].SecurityContext != nil { - podSpec.Containers[i].SecurityContext.RunAsGroup = nil + VisitContainers(podSpec, func(c *api.Container) bool { + if c.SecurityContext != nil { + c.SecurityContext.RunAsGroup = nil } - } - for i := range podSpec.InitContainers { - if podSpec.InitContainers[i].SecurityContext != nil { - podSpec.InitContainers[i].SecurityContext.RunAsGroup = nil - } - } + return true + }) } } @@ -450,26 +446,15 @@ func dropDisabledGMSAFieldsFromContainers(containers []api.Container) { func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) && !procMountInUse(oldPodSpec) { defaultProcMount := api.DefaultProcMount - for i := range podSpec.Containers { - if podSpec.Containers[i].SecurityContext != nil { - if podSpec.Containers[i].SecurityContext.ProcMount != nil { - // The ProcMount field was improperly forced to non-nil in 1.12. - // If the feature is disabled, and the existing object is not using any non-default values, and the ProcMount field is present in the incoming object, force to the default value. - // Note: we cannot force the field to nil when the feature is disabled because it causes a diff against previously persisted data. - podSpec.Containers[i].SecurityContext.ProcMount = &defaultProcMount - } + VisitContainers(podSpec, func(c *api.Container) bool { + if c.SecurityContext != nil && c.SecurityContext.ProcMount != nil { + // The ProcMount field was improperly forced to non-nil in 1.12. + // If the feature is disabled, and the existing object is not using any non-default values, and the ProcMount field is present in the incoming object, force to the default value. + // Note: we cannot force the field to nil when the feature is disabled because it causes a diff against previously persisted data. + c.SecurityContext.ProcMount = &defaultProcMount } - } - for i := range podSpec.InitContainers { - if podSpec.InitContainers[i].SecurityContext != nil { - if podSpec.InitContainers[i].SecurityContext.ProcMount != nil { - // The ProcMount field was improperly forced to non-nil in 1.12. - // If the feature is disabled, and the existing object is not using any non-default values, and the ProcMount field is present in the incoming object, force to the default value. - // Note: we cannot force the field to nil when the feature is disabled because it causes a diff against previously persisted data. - podSpec.InitContainers[i].SecurityContext.ProcMount = &defaultProcMount - } - } - } + return true + }) } } @@ -477,12 +462,10 @@ func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a VolumeDevice func dropDisabledVolumeDevicesFields(podSpec, oldPodSpec *api.PodSpec) { if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeDevicesInUse(oldPodSpec) { - for i := range podSpec.Containers { - podSpec.Containers[i].VolumeDevices = nil - } - for i := range podSpec.InitContainers { - podSpec.InitContainers[i].VolumeDevices = nil - } + VisitContainers(podSpec, func(c *api.Container) bool { + c.VolumeDevices = nil + return true + }) } } @@ -501,21 +484,19 @@ func subpathInUse(podSpec *api.PodSpec) bool { if podSpec == nil { return false } - for i := range podSpec.Containers { - for j := range podSpec.Containers[i].VolumeMounts { - if len(podSpec.Containers[i].VolumeMounts[j].SubPath) > 0 { - return true + + var inUse bool + VisitContainers(podSpec, func(c *api.Container) bool { + for i := range c.VolumeMounts { + if len(c.VolumeMounts[i].SubPath) > 0 { + inUse = true + return false } } - } - for i := range podSpec.InitContainers { - for j := range podSpec.InitContainers[i].VolumeMounts { - if len(podSpec.InitContainers[i].VolumeMounts[j].SubPath) > 0 { - return true - } - } - } - return false + return true + }) + + return inUse } // runtimeClassInUse returns true if the pod spec is non-nil and has a RuntimeClassName set @@ -546,25 +527,20 @@ func procMountInUse(podSpec *api.PodSpec) bool { if podSpec == nil { return false } - for i := range podSpec.Containers { - if podSpec.Containers[i].SecurityContext != nil { - if podSpec.Containers[i].SecurityContext.ProcMount != nil { - if *podSpec.Containers[i].SecurityContext.ProcMount != api.DefaultProcMount { - return true - } - } + + var inUse bool + VisitContainers(podSpec, func(c *api.Container) bool { + if c.SecurityContext == nil || c.SecurityContext.ProcMount == nil { + return true } - } - for i := range podSpec.InitContainers { - if podSpec.InitContainers[i].SecurityContext != nil { - if podSpec.InitContainers[i].SecurityContext.ProcMount != nil { - if *podSpec.InitContainers[i].SecurityContext.ProcMount != api.DefaultProcMount { - return true - } - } + if *c.SecurityContext.ProcMount != api.DefaultProcMount { + inUse = true + return false } - } - return false + return true + }) + + return inUse } // appArmorInUse returns true if the pod has apparmor related information @@ -645,17 +621,17 @@ func volumeDevicesInUse(podSpec *api.PodSpec) bool { if podSpec == nil { return false } - for i := range podSpec.Containers { - if podSpec.Containers[i].VolumeDevices != nil { - return true + + var inUse bool + VisitContainers(podSpec, func(c *api.Container) bool { + if c.VolumeDevices != nil { + inUse = true + return false } - } - for i := range podSpec.InitContainers { - if podSpec.InitContainers[i].VolumeDevices != nil { - return true - } - } - return false + return true + }) + + return inUse } // runAsGroupInUse returns true if the pod spec is non-nil and has a SecurityContext's RunAsGroup field set @@ -667,17 +643,17 @@ func runAsGroupInUse(podSpec *api.PodSpec) bool { if podSpec.SecurityContext != nil && podSpec.SecurityContext.RunAsGroup != nil { return true } - for i := range podSpec.Containers { - if podSpec.Containers[i].SecurityContext != nil && podSpec.Containers[i].SecurityContext.RunAsGroup != nil { - return true + + var inUse bool + VisitContainers(podSpec, func(c *api.Container) bool { + if c.SecurityContext != nil && c.SecurityContext.RunAsGroup != nil { + inUse = true + return false } - } - for i := range podSpec.InitContainers { - if podSpec.InitContainers[i].SecurityContext != nil && podSpec.InitContainers[i].SecurityContext.RunAsGroup != nil { - return true - } - } - return false + return true + }) + + return inUse } // gMSAFieldsInUse returns true if the pod spec is non-nil and has one of any @@ -723,21 +699,19 @@ func subpathExprInUse(podSpec *api.PodSpec) bool { if podSpec == nil { return false } - for i := range podSpec.Containers { - for j := range podSpec.Containers[i].VolumeMounts { - if len(podSpec.Containers[i].VolumeMounts[j].SubPathExpr) > 0 { - return true + + var inUse bool + VisitContainers(podSpec, func(c *api.Container) bool { + for i := range c.VolumeMounts { + if len(c.VolumeMounts[i].SubPathExpr) > 0 { + inUse = true + return false } } - } - for i := range podSpec.InitContainers { - for j := range podSpec.InitContainers[i].VolumeMounts { - if len(podSpec.InitContainers[i].VolumeMounts[j].SubPathExpr) > 0 { - return true - } - } - } - return false + return true + }) + + return inUse } // csiInUse returns true if any pod's spec include inline CSI volumes. diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 7f3f1dc98b4..5884c236e3b 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -34,6 +34,92 @@ import ( "k8s.io/kubernetes/pkg/security/apparmor" ) +func TestVisitContainers(t *testing.T) { + testCases := []struct { + description string + haveSpec *api.PodSpec + wantNames []string + }{ + { + "empty podspec", + &api.PodSpec{}, + []string{}, + }, + { + "regular containers", + &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + }, + []string{"c1", "c2"}, + }, + { + "init containers", + &api.PodSpec{ + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + }, + []string{"i1", "i2"}, + }, + { + "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"}, + }, + { + "dropping fields", + &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2", SecurityContext: &api.SecurityContext{}}, + }, + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2", SecurityContext: &api.SecurityContext{}}, + }, + }, + []string{"i1", "i2", "c1", "c2"}, + }, + } + + for _, tc := range testCases { + gotNames := []string{} + VisitContainers(tc.haveSpec, func(c *api.Container) bool { + gotNames = append(gotNames, c.Name) + if c.SecurityContext != nil { + c.SecurityContext = nil + } + 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) + } + } + } +} + func TestPodSecrets(t *testing.T) { // Stub containing all possible secret references in a pod. // The names of the referenced secrets match struct paths detected by reflection. diff --git a/pkg/api/v1/pod/util.go b/pkg/api/v1/pod/util.go index 590bca8eb05..24a3446a238 100644 --- a/pkg/api/v1/pod/util.go +++ b/pkg/api/v1/pod/util.go @@ -48,6 +48,28 @@ func FindPort(pod *v1.Pod, svcPort *v1.ServicePort) (int, error) { return 0, fmt.Errorf("no suitable port for manifest: %s", pod.UID) } +// ContainerVisitor is called with each container spec, and returns true +// if visiting should continue. +type ContainerVisitor func(container *v1.Container) (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, +// 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 + } + } + for i := range podSpec.Containers { + if !visitor(&podSpec.Containers[i]) { + return false + } + } + return true +} + // Visitor is called with each object name, and returns true if visiting should continue type Visitor func(name string) (shouldContinue bool) @@ -61,16 +83,9 @@ func VisitPodSecretNames(pod *v1.Pod, visitor Visitor) bool { return false } } - for i := range pod.Spec.InitContainers { - if !visitContainerSecretNames(&pod.Spec.InitContainers[i], visitor) { - return false - } - } - for i := range pod.Spec.Containers { - if !visitContainerSecretNames(&pod.Spec.Containers[i], visitor) { - return false - } - } + VisitContainers(&pod.Spec, func(c *v1.Container) bool { + return visitContainerSecretNames(c, visitor) + }) var source *v1.VolumeSource for i := range pod.Spec.Volumes { @@ -152,16 +167,9 @@ 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 { - for i := range pod.Spec.InitContainers { - if !visitContainerConfigmapNames(&pod.Spec.InitContainers[i], visitor) { - return false - } - } - for i := range pod.Spec.Containers { - if !visitContainerConfigmapNames(&pod.Spec.Containers[i], visitor) { - return false - } - } + VisitContainers(&pod.Spec, func(c *v1.Container) bool { + return visitContainerConfigmapNames(c, visitor) + }) var source *v1.VolumeSource for i := range pod.Spec.Volumes { source = &pod.Spec.Volumes[i].VolumeSource diff --git a/pkg/api/v1/pod/util_test.go b/pkg/api/v1/pod/util_test.go index 681d5faab55..6aabc324d76 100644 --- a/pkg/api/v1/pod/util_test.go +++ b/pkg/api/v1/pod/util_test.go @@ -198,6 +198,92 @@ func TestFindPort(t *testing.T) { } } +func TestVisitContainers(t *testing.T) { + testCases := []struct { + description string + haveSpec *v1.PodSpec + wantNames []string + }{ + { + "empty podspec", + &v1.PodSpec{}, + []string{}, + }, + { + "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"}, + }, + { + "dropping fields", + &v1.PodSpec{ + Containers: []v1.Container{ + {Name: "c1"}, + {Name: "c2", SecurityContext: &v1.SecurityContext{}}, + }, + InitContainers: []v1.Container{ + {Name: "i1"}, + {Name: "i2", SecurityContext: &v1.SecurityContext{}}, + }, + }, + []string{"i1", "i2", "c1", "c2"}, + }, + } + + 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 + } + 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) + } + } + } +} + func TestPodSecrets(t *testing.T) { // Stub containing all possible secret references in a pod. // The names of the referenced secrets match struct paths detected by reflection. diff --git a/pkg/apis/core/pods/helpers.go b/pkg/apis/core/pods/helpers.go index cf199cee737..544b3c8de7d 100644 --- a/pkg/apis/core/pods/helpers.go +++ b/pkg/apis/core/pods/helpers.go @@ -19,9 +19,27 @@ package pods import ( "fmt" + "k8s.io/apimachinery/pkg/util/validation/field" + api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/fieldpath" ) +// ContainerVisitorWithPath is called with each container and the field.Path to that container +type ContainerVisitorWithPath func(container *api.Container, path *field.Path) + +// VisitContainersWithPath invokes the visitor function with a pointer to the spec +// of every container in the given pod spec and the field.Path to that container. +func VisitContainersWithPath(podSpec *api.PodSpec, visitor ContainerVisitorWithPath) { + path := field.NewPath("spec", "initContainers") + for i := range podSpec.InitContainers { + visitor(&podSpec.InitContainers[i], path.Index(i)) + } + path = field.NewPath("spec", "containers") + for i := range podSpec.Containers { + visitor(&podSpec.Containers[i], path.Index(i)) + } +} + // ConvertDownwardAPIFieldLabel converts the specified downward API field label // and its value in the pod of the specified version to the internal version, // and returns the converted label and value. This function returns an error if diff --git a/pkg/apis/core/pods/helpers_test.go b/pkg/apis/core/pods/helpers_test.go index 9db95a014ba..2677709e968 100644 --- a/pkg/apis/core/pods/helpers_test.go +++ b/pkg/apis/core/pods/helpers_test.go @@ -17,9 +17,71 @@ limitations under the License. package pods import ( + "reflect" "testing" + + "k8s.io/apimachinery/pkg/util/validation/field" + api "k8s.io/kubernetes/pkg/apis/core" ) +func TestVisitContainersWithPath(t *testing.T) { + testCases := []struct { + description string + haveSpec *api.PodSpec + wantNames []string + }{ + { + "empty podspec", + &api.PodSpec{}, + []string{}, + }, + { + "regular containers", + &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + }, + []string{"spec.containers[0]", "spec.containers[1]"}, + }, + { + "init containers", + &api.PodSpec{ + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + }, + []string{"spec.initContainers[0]", "spec.initContainers[1]"}, + }, + { + "regular and init containers", + &api.PodSpec{ + Containers: []api.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + InitContainers: []api.Container{ + {Name: "i1"}, + {Name: "i2"}, + }, + }, + []string{"spec.initContainers[0]", "spec.initContainers[1]", "spec.containers[0]", "spec.containers[1]"}, + }, + } + + for _, tc := range testCases { + gotNames := []string{} + VisitContainersWithPath(tc.haveSpec, func(c *api.Container, p *field.Path) { + gotNames = append(gotNames, p.String()) + }) + if !reflect.DeepEqual(gotNames, tc.wantNames) { + t.Errorf("VisitContainersWithPath() for test case %q visited containers %q, wanted to visit %q", tc.description, gotNames, tc.wantNames) + } + } +} + func TestConvertDownwardAPIFieldLabel(t *testing.T) { testCases := []struct { version string diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 6bbc1d1e031..43e07b89618 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3434,17 +3434,13 @@ func ValidateAppArmorPodAnnotations(annotations map[string]string, spec *core.Po } func podSpecHasContainer(spec *core.PodSpec, containerName string) bool { - for _, c := range spec.InitContainers { + var hasContainer bool + podshelper.VisitContainersWithPath(spec, func(c *core.Container, _ *field.Path) { if c.Name == containerName { - return true + hasContainer = true } - } - for _, c := range spec.Containers { - if c.Name == containerName { - return true - } - } - return false + }) + return hasContainer } const ( diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 086b0850d51..c543ecbc240 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -361,17 +361,15 @@ func LogLocation( } func podHasContainerWithName(pod *api.Pod, containerName string) bool { - for _, c := range pod.Spec.Containers { + var hasContainer bool + podutil.VisitContainers(&pod.Spec, func(c *api.Container) bool { if c.Name == containerName { - return true + hasContainer = true + return false } - } - for _, c := range pod.Spec.InitContainers { - if c.Name == containerName { - return true - } - } - return false + return true + }) + return hasContainer } func streamParams(params url.Values, opts runtime.Object) error { diff --git a/pkg/security/apparmor/validate.go b/pkg/security/apparmor/validate.go index 1d03c7ee02d..bd721759566 100644 --- a/pkg/security/apparmor/validate.go +++ b/pkg/security/apparmor/validate.go @@ -27,6 +27,7 @@ import ( "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" kubetypes "k8s.io/kubernetes/pkg/kubelet/types" utilpath "k8s.io/utils/path" @@ -76,18 +77,16 @@ func (v *validator) Validate(pod *v1.Pod) error { return fmt.Errorf("could not read loaded profiles: %v", err) } - for _, container := range pod.Spec.InitContainers { - if err := validateProfile(GetProfileName(pod, container.Name), loadedProfiles); err != nil { - return err + var retErr error + podutil.VisitContainers(&pod.Spec, func(container *v1.Container) bool { + retErr = validateProfile(GetProfileName(pod, container.Name), loadedProfiles) + if retErr != nil { + return false } - } - for _, container := range pod.Spec.Containers { - if err := validateProfile(GetProfileName(pod, container.Name), loadedProfiles); err != nil { - return err - } - } + return true + }) - return nil + return retErr } func (v *validator) ValidateHost() error { diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go index fc33afc5dc5..22d48966969 100644 --- a/pkg/security/podsecuritypolicy/provider.go +++ b/pkg/security/podsecuritypolicy/provider.go @@ -24,7 +24,9 @@ import ( policy "k8s.io/api/policy/v1beta1" "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" + podutil "k8s.io/kubernetes/pkg/api/pod" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/apis/core/pods" "k8s.io/kubernetes/pkg/features" psputil "k8s.io/kubernetes/pkg/security/podsecuritypolicy/util" "k8s.io/kubernetes/pkg/securitycontext" @@ -108,19 +110,16 @@ func (s *simpleProvider) MutatePod(pod *api.Pod) error { pod.Spec.RuntimeClassName = s.psp.Spec.RuntimeClass.DefaultRuntimeClassName } - for i := range pod.Spec.InitContainers { - if err := s.mutateContainer(pod, &pod.Spec.InitContainers[i]); err != nil { - return err + var retErr error + podutil.VisitContainers(&pod.Spec, func(c *api.Container) bool { + retErr = s.mutateContainer(pod, c) + if retErr != nil { + return false } - } + return true + }) - for i := range pod.Spec.Containers { - if err := s.mutateContainer(pod, &pod.Spec.Containers[i]); err != nil { - return err - } - } - - return nil + return retErr } // mutateContainer sets the default values of the required but not filled fields. @@ -239,15 +238,9 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList { allErrs = append(allErrs, validateRuntimeClassName(pod.Spec.RuntimeClassName, s.psp.Spec.RuntimeClass.AllowedRuntimeClassNames)...) } - fldPath := field.NewPath("spec", "initContainers") - for i := range pod.Spec.InitContainers { - allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.InitContainers[i], fldPath.Index(i))...) - } - - fldPath = field.NewPath("spec", "containers") - for i := range pod.Spec.Containers { - allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.Containers[i], fldPath.Index(i))...) - } + pods.VisitContainersWithPath(&pod.Spec, func(c *api.Container, p *field.Path) { + allErrs = append(allErrs, s.validateContainer(pod, c, p)...) + }) return allErrs } @@ -281,26 +274,13 @@ func (s *simpleProvider) validatePodVolumes(pod *api.Pod) field.ErrorList { fmt.Sprintf("is not allowed to be used"))) } else if mustBeReadOnly { // Ensure all the VolumeMounts that use this volume are read-only - for i, c := range pod.Spec.InitContainers { - for j, cv := range c.VolumeMounts { + pods.VisitContainersWithPath(&pod.Spec, func(c *api.Container, p *field.Path) { + for i, cv := range c.VolumeMounts { if cv.Name == v.Name && !cv.ReadOnly { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec", "initContainers").Index(i).Child("volumeMounts").Index(j).Child("readOnly"), - cv.ReadOnly, "must be read-only"), - ) + allErrs = append(allErrs, field.Invalid(p.Child("volumeMounts").Index(i).Child("readOnly"), cv.ReadOnly, "must be read-only")) } } - } - for i, c := range pod.Spec.Containers { - for j, cv := range c.VolumeMounts { - if cv.Name == v.Name && !cv.ReadOnly { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec", "containers").Index(i).Child("volumeMounts").Index(j).Child("readOnly"), - cv.ReadOnly, "must be read-only"), - ) - } - } - } + }) } case policy.FlexVolume: diff --git a/pkg/volume/util/nested_volumes.go b/pkg/volume/util/nested_volumes.go index 174f5b0fac1..5a728a8397a 100644 --- a/pkg/volume/util/nested_volumes.go +++ b/pkg/volume/util/nested_volumes.go @@ -24,6 +24,7 @@ import ( "strings" "k8s.io/api/core/v1" + podutil "k8s.io/kubernetes/pkg/api/v1/pod" ) // getNestedMountpoints returns a list of mountpoint directories that should be created @@ -70,16 +71,19 @@ func getNestedMountpoints(name, baseDir string, pod v1.Pod) ([]string, error) { } return nil } - for _, container := range pod.Spec.InitContainers { - if err := checkContainer(&container); err != nil { - return nil, err - } - } - for _, container := range pod.Spec.Containers { - if err := checkContainer(&container); err != nil { - return nil, err + + var retErr error + podutil.VisitContainers(&pod.Spec, func(c *v1.Container) bool { + retErr = checkContainer(c) + if retErr != nil { + return false } + return true + }) + if retErr != nil { + return nil, retErr } + return retval, nil } diff --git a/plugin/pkg/admission/alwayspullimages/admission.go b/plugin/pkg/admission/alwayspullimages/admission.go index 9f751a48126..ebb5cb7bfdc 100644 --- a/plugin/pkg/admission/alwayspullimages/admission.go +++ b/plugin/pkg/admission/alwayspullimages/admission.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/admission" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/apis/core/pods" ) // PluginName indicates name of admission plugin. @@ -63,13 +64,9 @@ func (a *AlwaysPullImages) Admit(attributes admission.Attributes, o admission.Ob return apierrors.NewBadRequest("Resource was marked with kind Pod but was unable to be converted") } - for i := range pod.Spec.InitContainers { - pod.Spec.InitContainers[i].ImagePullPolicy = api.PullAlways - } - - for i := range pod.Spec.Containers { - pod.Spec.Containers[i].ImagePullPolicy = api.PullAlways - } + pods.VisitContainersWithPath(&pod.Spec, func(c *api.Container, _ *field.Path) { + c.ImagePullPolicy = api.PullAlways + }) return nil } @@ -85,23 +82,17 @@ func (*AlwaysPullImages) Validate(attributes admission.Attributes, o admission.O return apierrors.NewBadRequest("Resource was marked with kind Pod but was unable to be converted") } - for i := range pod.Spec.InitContainers { - if pod.Spec.InitContainers[i].ImagePullPolicy != api.PullAlways { - return admission.NewForbidden(attributes, - field.NotSupported(field.NewPath("spec", "initContainers").Index(i).Child("imagePullPolicy"), - pod.Spec.InitContainers[i].ImagePullPolicy, []string{string(api.PullAlways)}, - ), - ) - } - } - for i := range pod.Spec.Containers { - if pod.Spec.Containers[i].ImagePullPolicy != api.PullAlways { - return admission.NewForbidden(attributes, - field.NotSupported(field.NewPath("spec", "containers").Index(i).Child("imagePullPolicy"), - pod.Spec.Containers[i].ImagePullPolicy, []string{string(api.PullAlways)}, - ), - ) + var allErrs []error + pods.VisitContainersWithPath(&pod.Spec, func(c *api.Container, p *field.Path) { + if c.ImagePullPolicy != api.PullAlways { + allErrs = append(allErrs, admission.NewForbidden(attributes, + field.NotSupported(p.Child("imagePullPolicy"), c.ImagePullPolicy, []string{string(api.PullAlways)}), + )) } + }) + if len(allErrs) > 0 { + // TODO: consider using utilerrors.NewAggregate(allErrs) + return allErrs[0] } return nil diff --git a/plugin/pkg/admission/exec/admission.go b/plugin/pkg/admission/exec/admission.go index a90581a07ad..620cfd1e5c2 100644 --- a/plugin/pkg/admission/exec/admission.go +++ b/plugin/pkg/admission/exec/admission.go @@ -26,6 +26,7 @@ import ( genericadmissioninitializer "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/client-go/kubernetes" "k8s.io/klog" + podutil "k8s.io/kubernetes/pkg/api/v1/pod" ) const ( @@ -146,21 +147,16 @@ func (d *DenyExec) Validate(a admission.Attributes, o admission.ObjectInterfaces // isPrivileged will return true a pod has any privileged containers func isPrivileged(pod *corev1.Pod) bool { - for _, c := range pod.Spec.InitContainers { + var privileged bool + podutil.VisitContainers(&pod.Spec, func(c *corev1.Container) bool { if c.SecurityContext == nil || c.SecurityContext.Privileged == nil { - continue - } - if *c.SecurityContext.Privileged { return true } - } - for _, c := range pod.Spec.Containers { - if c.SecurityContext == nil || c.SecurityContext.Privileged == nil { - continue - } if *c.SecurityContext.Privileged { - return true + privileged = true + return false } - } - return false + return true + }) + return privileged } diff --git a/plugin/pkg/admission/podpreset/admission.go b/plugin/pkg/admission/podpreset/admission.go index 3a991554140..63b7fe0ced5 100644 --- a/plugin/pkg/admission/podpreset/admission.go +++ b/plugin/pkg/admission/podpreset/admission.go @@ -29,12 +29,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/admission" genericadmissioninitializer "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" settingsv1alpha1listers "k8s.io/client-go/listers/settings/v1alpha1" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/apis/core/pods" apiscorev1 "k8s.io/kubernetes/pkg/apis/core/v1" ) @@ -183,16 +185,11 @@ func safeToApplyPodPresetsOnPod(pod *api.Pod, podPresets []*settingsv1alpha1.Pod if _, err := mergeVolumes(pod.Spec.Volumes, podPresets); err != nil { errs = append(errs, err) } - for _, ctr := range pod.Spec.Containers { - if err := safeToApplyPodPresetsOnContainer(&ctr, podPresets); err != nil { + pods.VisitContainersWithPath(&pod.Spec, func(c *api.Container, _ *field.Path) { + if err := safeToApplyPodPresetsOnContainer(c, podPresets); err != nil { errs = append(errs, err) } - } - for _, iCtr := range pod.Spec.InitContainers { - if err := safeToApplyPodPresetsOnContainer(&iCtr, podPresets); err != nil { - errs = append(errs, err) - } - } + }) return utilerrors.NewAggregate(errs) } diff --git a/staging/publishing/import-restrictions.yaml b/staging/publishing/import-restrictions.yaml index b87453cc655..036f5ed4f60 100644 --- a/staging/publishing/import-restrictions.yaml +++ b/staging/publishing/import-restrictions.yaml @@ -2,6 +2,7 @@ allowedImports: - k8s.io/apimachinery - k8s.io/apiserver/pkg/util/feature + - k8s.io/component-base/featuregate/testing - k8s.io/kubernetes/pkg/apis/core - k8s.io/kubernetes/pkg/features - k8s.io/kubernetes/pkg/fieldpath From a0b57ad3db7d20319e3eaa84ec461aea2047d9cf Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Wed, 19 Jun 2019 16:00:02 +0000 Subject: [PATCH 2/2] Update BUILD files for container helper --- pkg/apis/core/pods/BUILD | 10 +++++++++- pkg/security/apparmor/BUILD | 1 + pkg/security/podsecuritypolicy/BUILD | 2 ++ pkg/volume/util/BUILD | 1 + plugin/pkg/admission/alwayspullimages/BUILD | 1 + plugin/pkg/admission/exec/BUILD | 1 + plugin/pkg/admission/podpreset/BUILD | 2 ++ 7 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/apis/core/pods/BUILD b/pkg/apis/core/pods/BUILD index dea471375dc..aaa745e6361 100644 --- a/pkg/apis/core/pods/BUILD +++ b/pkg/apis/core/pods/BUILD @@ -5,13 +5,21 @@ go_library( srcs = ["helpers.go"], importpath = "k8s.io/kubernetes/pkg/apis/core/pods", visibility = ["//visibility:public"], - deps = ["//pkg/fieldpath:go_default_library"], + deps = [ + "//pkg/apis/core:go_default_library", + "//pkg/fieldpath:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + ], ) go_test( name = "go_default_test", srcs = ["helpers_test.go"], embed = [":go_default_library"], + deps = [ + "//pkg/apis/core:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + ], ) filegroup( diff --git a/pkg/security/apparmor/BUILD b/pkg/security/apparmor/BUILD index fb29986efd4..3b46734c358 100644 --- a/pkg/security/apparmor/BUILD +++ b/pkg/security/apparmor/BUILD @@ -15,6 +15,7 @@ go_library( ], importpath = "k8s.io/kubernetes/pkg/security/apparmor", deps = [ + "//pkg/api/v1/pod:go_default_library", "//pkg/features:go_default_library", "//pkg/kubelet/types:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", diff --git a/pkg/security/podsecuritypolicy/BUILD b/pkg/security/podsecuritypolicy/BUILD index fb7715cc5a9..15a1df6789a 100644 --- a/pkg/security/podsecuritypolicy/BUILD +++ b/pkg/security/podsecuritypolicy/BUILD @@ -16,7 +16,9 @@ go_library( ], importpath = "k8s.io/kubernetes/pkg/security/podsecuritypolicy", deps = [ + "//pkg/api/pod:go_default_library", "//pkg/apis/core:go_default_library", + "//pkg/apis/core/pods:go_default_library", "//pkg/features:go_default_library", "//pkg/security/podsecuritypolicy/apparmor:go_default_library", "//pkg/security/podsecuritypolicy/capabilities:go_default_library", diff --git a/pkg/volume/util/BUILD b/pkg/volume/util/BUILD index 4b8c3f4827c..e51e73f6231 100644 --- a/pkg/volume/util/BUILD +++ b/pkg/volume/util/BUILD @@ -20,6 +20,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/api/legacyscheme:go_default_library", + "//pkg/api/v1/pod:go_default_library", "//pkg/apis/core/v1/helper:go_default_library", "//pkg/features:go_default_library", "//pkg/util/mount:go_default_library", diff --git a/plugin/pkg/admission/alwayspullimages/BUILD b/plugin/pkg/admission/alwayspullimages/BUILD index f2fc4bc9c05..34533347242 100644 --- a/plugin/pkg/admission/alwayspullimages/BUILD +++ b/plugin/pkg/admission/alwayspullimages/BUILD @@ -12,6 +12,7 @@ go_library( importpath = "k8s.io/kubernetes/plugin/pkg/admission/alwayspullimages", deps = [ "//pkg/apis/core:go_default_library", + "//pkg/apis/core/pods:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", diff --git a/plugin/pkg/admission/exec/BUILD b/plugin/pkg/admission/exec/BUILD index 2b53a313aeb..d03d66924a4 100644 --- a/plugin/pkg/admission/exec/BUILD +++ b/plugin/pkg/admission/exec/BUILD @@ -11,6 +11,7 @@ go_library( srcs = ["admission.go"], importpath = "k8s.io/kubernetes/plugin/pkg/admission/exec", deps = [ + "//pkg/api/v1/pod:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", diff --git a/plugin/pkg/admission/podpreset/BUILD b/plugin/pkg/admission/podpreset/BUILD index 95e3ac06ea5..3e8e50d48ea 100644 --- a/plugin/pkg/admission/podpreset/BUILD +++ b/plugin/pkg/admission/podpreset/BUILD @@ -34,12 +34,14 @@ go_library( importpath = "k8s.io/kubernetes/plugin/pkg/admission/podpreset", deps = [ "//pkg/apis/core:go_default_library", + "//pkg/apis/core/pods:go_default_library", "//pkg/apis/core/v1:go_default_library", "//staging/src/k8s.io/api/settings/v1alpha1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/initializer:go_default_library", "//staging/src/k8s.io/client-go/informers:go_default_library",