From 4a7fd2a61410ed410d8870bdf9b5d0d46295f821 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Tue, 26 Jul 2022 06:31:35 +0200 Subject: [PATCH 1/4] Use structs for container validation test This introduces no changes to unit tests other than to switch from map-based to struct-based tables in TestValidateContainers and TestValidateInitContainers in order to make diffs for later commits easier to read. --- pkg/apis/core/validation/validation_test.go | 541 ++++++++++++-------- 1 file changed, 317 insertions(+), 224 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index ff7bc068445..76858c1832c 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -7169,245 +7169,332 @@ func TestValidateContainers(t *testing.T) { capabilities.SetForTests(capabilities.Capabilities{ AllowPrivileged: false, }) - errorCases := map[string][]core.Container{ - "zero-length name": {{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - "zero-length-image": {{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - "name > 63 characters": {{Name: strings.Repeat("a", 64), Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - "name not a DNS label": {{Name: "a.b.c", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - "name not unique": { - {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, - {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + errorCases := []struct { + title string + containers []core.Container + }{ + { + "zero-length name", + []core.Container{{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - "zero-length image": {{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - "host port not unique": { - {Name: "abc", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 80, HostPort: 80, Protocol: "TCP"}}, - ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, - {Name: "def", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 81, HostPort: 80, Protocol: "TCP"}}, - ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + { + "zero-length-image", + []core.Container{{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - "invalid env var name": { - {Name: "abc", Image: "image", Env: []core.EnvVar{{Name: "ev!1"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + { + "name > 63 characters", + []core.Container{{Name: strings.Repeat("a", 64), Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - "unknown volume name": { - {Name: "abc", Image: "image", VolumeMounts: []core.VolumeMount{{Name: "anything", MountPath: "/foo"}}, - ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + { + "name not a DNS label", + []core.Container{{Name: "a.b.c", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - "invalid lifecycle, no exec command.": { - { - Name: "life-123", - Image: "image", - Lifecycle: &core.Lifecycle{ - PreStop: &core.LifecycleHandler{ - Exec: &core.ExecAction{}, - }, - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", + { + "name not unique", + []core.Container{ + {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, }, - "invalid lifecycle, no http path.": { - { - Name: "life-123", - Image: "image", - Lifecycle: &core.Lifecycle{ - PreStop: &core.LifecycleHandler{ - HTTPGet: &core.HTTPGetAction{}, - }, - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", + { + "zero-length image", + []core.Container{{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + }, + { + "host port not unique", + []core.Container{ + {Name: "abc", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 80, HostPort: 80, Protocol: "TCP"}}, + ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + {Name: "def", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 81, HostPort: 80, Protocol: "TCP"}}, + ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, }, - "invalid lifecycle, no tcp socket port.": { - { - Name: "life-123", - Image: "image", - Lifecycle: &core.Lifecycle{ - PreStop: &core.LifecycleHandler{ - TCPSocket: &core.TCPSocketAction{}, - }, - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", + { + "invalid env var name", + []core.Container{ + {Name: "abc", Image: "image", Env: []core.EnvVar{{Name: "ev!1"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, }, - "invalid lifecycle, zero tcp socket port.": { - { - Name: "life-123", - Image: "image", - Lifecycle: &core.Lifecycle{ - PreStop: &core.LifecycleHandler{ - TCPSocket: &core.TCPSocketAction{ - Port: intstr.FromInt(0), + { + "unknown volume name", + []core.Container{ + {Name: "abc", Image: "image", VolumeMounts: []core.VolumeMount{{Name: "anything", MountPath: "/foo"}}, + ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + }, + }, + { + "invalid lifecycle, no exec command.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + Exec: &core.ExecAction{}, }, }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, - "invalid lifecycle, no action.": { - { - Name: "life-123", - Image: "image", - Lifecycle: &core.Lifecycle{ - PreStop: &core.LifecycleHandler{}, - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - "invalid readiness probe, terminationGracePeriodSeconds set.": { - { - Name: "life-123", - Image: "image", - ReadinessProbe: &core.Probe{ - ProbeHandler: core.ProbeHandler{ - TCPSocket: &core.TCPSocketAction{}, + { + "invalid lifecycle, no http path.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + HTTPGet: &core.HTTPGetAction{}, + }, }, - TerminationGracePeriodSeconds: utilpointer.Int64Ptr(10), + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, - "invalid liveness probe, no tcp socket port.": { - { - Name: "life-123", - Image: "image", - LivenessProbe: &core.Probe{ - ProbeHandler: core.ProbeHandler{ - TCPSocket: &core.TCPSocketAction{}, + { + "invalid lifecycle, no tcp socket port.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + TCPSocket: &core.TCPSocketAction{}, + }, }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, - "invalid liveness probe, no action.": { - { - Name: "life-123", - Image: "image", - LivenessProbe: &core.Probe{ - ProbeHandler: core.ProbeHandler{}, - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - "invalid message termination policy": { - { - Name: "life-123", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "Unknown", - }, - }, - "empty message termination policy": { - { - Name: "life-123", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "", - }, - }, - "privilege disabled": { - {Name: "abc", Image: "image", SecurityContext: fakeValidSecurityContext(true)}, - }, - "invalid compute resource": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Limits: core.ResourceList{ - "disk": resource.MustParse("10G"), + { + "invalid lifecycle, zero tcp socket port.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(0), + }, + }, }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, - "Resource CPU invalid": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Limits: getResourceLimits("-10", "0"), - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - "Resource Requests CPU invalid": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Requests: getResourceLimits("-10", "0"), - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - "Resource Memory invalid": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Limits: getResourceLimits("0", "-10"), - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - "Request limit simple invalid": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Limits: getResourceLimits("5", "3"), - Requests: getResourceLimits("6", "3"), - }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - "Invalid storage limit request": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Limits: core.ResourceList{ - core.ResourceName("attachable-volumes-aws-ebs"): *resource.NewQuantity(10, resource.DecimalSI), + { + "invalid lifecycle, no action.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{}, }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, - "Request limit multiple invalid": { - { - Name: "abc-123", - Image: "image", - Resources: core.ResourceRequirements{ - Limits: getResourceLimits("5", "3"), - Requests: getResourceLimits("6", "4"), + { + "invalid readiness probe, terminationGracePeriodSeconds set.", + []core.Container{ + { + Name: "life-123", + Image: "image", + ReadinessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{}, + }, + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(10), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, - "Invalid env from": { - { - Name: "env-from-source", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - EnvFrom: []core.EnvFromSource{ - { - ConfigMapRef: &core.ConfigMapEnvSource{ - LocalObjectReference: core.LocalObjectReference{ - Name: "$%^&*#", + { + "invalid liveness probe, no tcp socket port.", + []core.Container{ + { + Name: "life-123", + Image: "image", + LivenessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{}, + }, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "invalid liveness probe, no action.", + []core.Container{ + { + Name: "life-123", + Image: "image", + LivenessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{}, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "invalid message termination policy", + []core.Container{ + { + Name: "life-123", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "Unknown", + }, + }, + }, + { + "empty message termination policy", + []core.Container{ + { + Name: "life-123", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "", + }, + }, + }, + { + "privilege disabled", + []core.Container{ + {Name: "abc", Image: "image", SecurityContext: fakeValidSecurityContext(true)}, + }, + }, + { + "invalid compute resource", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: core.ResourceList{ + "disk": resource.MustParse("10G"), + }, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Resource CPU invalid", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: getResourceLimits("-10", "0"), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Resource Requests CPU invalid", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Requests: getResourceLimits("-10", "0"), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Resource Memory invalid", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: getResourceLimits("0", "-10"), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Request limit simple invalid", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: getResourceLimits("5", "3"), + Requests: getResourceLimits("6", "3"), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Invalid storage limit request", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: core.ResourceList{ + core.ResourceName("attachable-volumes-aws-ebs"): *resource.NewQuantity(10, resource.DecimalSI), + }, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Request limit multiple invalid", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: getResourceLimits("5", "3"), + Requests: getResourceLimits("6", "4"), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + }, + { + "Invalid env from", + []core.Container{ + { + Name: "env-from-source", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + EnvFrom: []core.EnvFromSource{ + { + ConfigMapRef: &core.ConfigMapEnvSource{ + LocalObjectReference: core.LocalObjectReference{ + Name: "$%^&*#", + }, }, }, }, @@ -7415,9 +7502,9 @@ func TestValidateContainers(t *testing.T) { }, }, } - for k, v := range errorCases { - if errs := validateContainers(v, false, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { - t.Errorf("expected failure for %s", k) + for _, tc := range errorCases { + if errs := validateContainers(tc.containers, false, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { + t.Errorf("expected failure for %s", tc.title) } } } @@ -7457,27 +7544,33 @@ func TestValidateInitContainers(t *testing.T) { capabilities.SetForTests(capabilities.Capabilities{ AllowPrivileged: false, }) - errorCases := map[string][]core.Container{ - "duplicate ports": { - { - Name: "abc", - Image: "image", - Ports: []core.ContainerPort{ - { - ContainerPort: 8080, HostPort: 8080, Protocol: "TCP", - }, - { - ContainerPort: 8080, HostPort: 8080, Protocol: "TCP", + errorCases := []struct { + title string + initContainers []core.Container + }{ + { + "duplicate ports", + []core.Container{ + { + Name: "abc", + Image: "image", + Ports: []core.ContainerPort{ + { + ContainerPort: 8080, HostPort: 8080, Protocol: "TCP", + }, + { + ContainerPort: 8080, HostPort: 8080, Protocol: "TCP", + }, }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", }, }, } - for k, v := range errorCases { - if errs := validateContainers(v, true, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { - t.Errorf("expected failure for %s", k) + for _, tc := range errorCases { + if errs := validateContainers(tc.initContainers, true, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { + t.Errorf("expected failure for %s", tc.title) } } } From dbbbf8502efb364479e8466b29c9027d388b7bbf Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Sat, 23 Jul 2022 22:16:59 +0200 Subject: [PATCH 2/4] Improve container validation test coverage Adds missing tests based on KUBE_COVER and checks that errors returned by validation are of the type and for the field expected. Fixes tests that had multiple errors so later failures aren't masked if there's a regression in only one of the errors. --- pkg/apis/core/validation/validation_test.go | 409 ++++++++++++++++++-- 1 file changed, 382 insertions(+), 27 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 76858c1832c..9504a15caba 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6718,7 +6718,6 @@ func TestValidateEphemeralContainers(t *testing.T) { ephemeralContainers []core.EphemeralContainer expectedError field.Error }{ - { "Name Collision with Container.Containers", []core.EphemeralContainer{ @@ -6764,6 +6763,13 @@ func TestValidateEphemeralContainers(t *testing.T) { }, field.Error{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0][0].image"}, }, + { + "invalid image pull policy", + []core.EphemeralContainer{ + {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "PullThreeTimes", TerminationMessagePolicy: "File"}}, + }, + field.Error{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0][0].imagePullPolicy"}, + }, { "TargetContainerName doesn't exist", []core.EphemeralContainer{ @@ -6849,6 +6855,26 @@ func TestValidateEphemeralContainers(t *testing.T) { }, field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].readinessProbe"}, }, + { + "Container uses disallowed field: StartupProbe", + []core.EphemeralContainer{ + { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + StartupProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{Port: intstr.FromInt(80)}, + }, + SuccessThreshold: 1, + }, + }, + }, + }, + field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].startupProbe"}, + }, { "Container uses disallowed field: Resources", []core.EphemeralContainer{ @@ -6907,20 +6933,22 @@ func TestValidateEphemeralContainers(t *testing.T) { } for _, tc := range tcs { - errs := validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, field.NewPath("ephemeralContainers"), PodValidationOptions{}) + t.Run(tc.title, func(t *testing.T) { + errs := validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, field.NewPath("ephemeralContainers"), PodValidationOptions{}) + if len(errs) == 0 { + t.Fatal("expected error but received none") + } - if len(errs) == 0 { - t.Errorf("for test %q, expected error but received none", tc.title) - } else if len(errs) > 1 { - t.Errorf("for test %q, expected 1 error but received %d: %q", tc.title, len(errs), errs) - } else { + if len(errs) > 1 { + t.Errorf("expected 1 error but received %d: %q", len(errs), errs) + } if errs[0].Type != tc.expectedError.Type { - t.Errorf("for test %q, expected error type %q but received %q: %q", tc.title, string(tc.expectedError.Type), string(errs[0].Type), errs) + t.Errorf("expected error type %q but received %q: %q", string(tc.expectedError.Type), string(errs[0].Type), errs) } if errs[0].Field != tc.expectedError.Field { - t.Errorf("for test %q, expected error for field %q but received error for field %q: %q", tc.title, tc.expectedError.Field, errs[0].Field, errs) + t.Errorf("expected error for field %q but received error for field %q: %q", tc.expectedError.Field, errs[0].Field, errs) } - } + }) } } @@ -7161,6 +7189,34 @@ func TestValidateContainers(t *testing.T) { }, }, {Name: "abc-1234", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", SecurityContext: fakeValidSecurityContext(true)}, + { + Name: "live-123", + Image: "image", + LivenessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + SuccessThreshold: 1, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + { + Name: "startup-123", + Image: "image", + StartupProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + SuccessThreshold: 1, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, } if errs := validateContainers(successCase, false, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) @@ -7170,24 +7226,29 @@ func TestValidateContainers(t *testing.T) { AllowPrivileged: false, }) errorCases := []struct { - title string - containers []core.Container + title string + containers []core.Container + expectedErrors []field.Error }{ { "zero-length name", []core.Container{{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].name"}}, }, { "zero-length-image", []core.Container{{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].image"}}, }, { "name > 63 characters", []core.Container{{Name: strings.Repeat("a", 64), Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].name"}}, }, { "name not a DNS label", []core.Container{{Name: "a.b.c", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].name"}}, }, { "name not unique", @@ -7195,10 +7256,12 @@ func TestValidateContainers(t *testing.T) { {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, + []field.Error{{Type: field.ErrorTypeDuplicate, Field: "containers[1].name"}}, }, { "zero-length image", []core.Container{{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].image"}}, }, { "host port not unique", @@ -7208,12 +7271,14 @@ func TestValidateContainers(t *testing.T) { {Name: "def", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 81, HostPort: 80, Protocol: "TCP"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, + []field.Error{{Type: field.ErrorTypeDuplicate, Field: "containers[1].ports[0].hostPort"}}, }, { "invalid env var name", []core.Container{ {Name: "abc", Image: "image", Env: []core.EnvVar{{Name: "ev!1"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].env[0].name"}}, }, { "unknown volume name", @@ -7221,6 +7286,7 @@ func TestValidateContainers(t *testing.T) { {Name: "abc", Image: "image", VolumeMounts: []core.VolumeMount{{Name: "anything", MountPath: "/foo"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, + []field.Error{{Type: field.ErrorTypeNotFound, Field: "containers[0].volumeMounts[0].name"}}, }, { "invalid lifecycle, no exec command.", @@ -7237,6 +7303,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop.exec.command"}}, }, { "invalid lifecycle, no http path.", @@ -7246,13 +7313,57 @@ func TestValidateContainers(t *testing.T) { Image: "image", Lifecycle: &core.Lifecycle{ PreStop: &core.LifecycleHandler{ - HTTPGet: &core.HTTPGetAction{}, + HTTPGet: &core.HTTPGetAction{ + Port: intstr.FromInt(80), + Scheme: "HTTP", + }, }, }, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop.httpGet.path"}}, + }, + { + "invalid lifecycle, no http port.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + HTTPGet: &core.HTTPGetAction{ + Path: "/", + Scheme: "HTTP", + }, + }, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.httpGet.port"}}, + }, + { + "invalid lifecycle, no http scheme.", + []core.Container{ + { + Name: "life-123", + Image: "image", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + HTTPGet: &core.HTTPGetAction{ + Path: "/", + Port: intstr.FromInt(80), + }, + }, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + []field.Error{{Type: field.ErrorTypeNotSupported, Field: "containers[0].lifecycle.preStop.httpGet.scheme"}}, }, { "invalid lifecycle, no tcp socket port.", @@ -7269,6 +7380,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.tcpSocket.port"}}, }, { "invalid lifecycle, zero tcp socket port.", @@ -7287,6 +7399,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.tcpSocket.port"}}, }, { "invalid lifecycle, no action.", @@ -7301,6 +7414,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop"}}, }, { "invalid readiness probe, terminationGracePeriodSeconds set.", @@ -7310,7 +7424,9 @@ func TestValidateContainers(t *testing.T) { Image: "image", ReadinessProbe: &core.Probe{ ProbeHandler: core.ProbeHandler{ - TCPSocket: &core.TCPSocketAction{}, + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, }, TerminationGracePeriodSeconds: utilpointer.Int64Ptr(10), }, @@ -7318,36 +7434,81 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.terminationGracePeriodSeconds"}}, }, { "invalid liveness probe, no tcp socket port.", []core.Container{ { - Name: "life-123", + Name: "live-123", Image: "image", LivenessProbe: &core.Probe{ ProbeHandler: core.ProbeHandler{ TCPSocket: &core.TCPSocketAction{}, }, + SuccessThreshold: 1, }, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.tcpSocket.port"}}, }, { "invalid liveness probe, no action.", []core.Container{ { - Name: "life-123", + Name: "live-123", Image: "image", LivenessProbe: &core.Probe{ - ProbeHandler: core.ProbeHandler{}, + ProbeHandler: core.ProbeHandler{}, + SuccessThreshold: 1, }, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].livenessProbe"}}, + }, + { + "invalid liveness probe, successThreshold != 1", + []core.Container{ + { + Name: "live-123", + Image: "image", + LivenessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + SuccessThreshold: 2, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.successThreshold"}}, + }, + { + "invalid startup probe, successThreshold != 1", + []core.Container{ + { + Name: "startup-123", + Image: "image", + StartupProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + SuccessThreshold: 2, + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.successThreshold"}}, }, { "invalid message termination policy", @@ -7359,6 +7520,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "Unknown", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].terminationMessagePolicy"}}, }, { "empty message termination policy", @@ -7370,12 +7532,20 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "", }, }, + []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].terminationMessagePolicy"}}, }, { "privilege disabled", []core.Container{ - {Name: "abc", Image: "image", SecurityContext: fakeValidSecurityContext(true)}, + { + Name: "abc", + Image: "image", + SecurityContext: fakeValidSecurityContext(true), + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, }, + []field.Error{{Type: field.ErrorTypeForbidden, Field: "containers[0].securityContext.privileged"}}, }, { "invalid compute resource", @@ -7392,6 +7562,10 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{ + {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[disk]"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[disk]"}, + }, }, { "Resource CPU invalid", @@ -7406,6 +7580,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[cpu]"}}, }, { "Resource Requests CPU invalid", @@ -7420,6 +7595,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests[cpu]"}}, }, { "Resource Memory invalid", @@ -7434,6 +7610,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[memory]"}}, }, { "Request limit simple invalid", @@ -7449,6 +7626,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, }, { "Invalid storage limit request", @@ -7465,21 +7643,42 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + []field.Error{ + {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[attachable-volumes-aws-ebs]"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[attachable-volumes-aws-ebs]"}, + }, }, { - "Request limit multiple invalid", + "CPU request limit multiple invalid", []core.Container{ { Name: "abc-123", Image: "image", Resources: core.ResourceRequirements{ Limits: getResourceLimits("5", "3"), - Requests: getResourceLimits("6", "4"), + Requests: getResourceLimits("6", "3"), }, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, + }, + { + "Memory request limit multiple invalid", + []core.Container{ + { + Name: "abc-123", + Image: "image", + Resources: core.ResourceRequirements{ + Limits: getResourceLimits("5", "3"), + Requests: getResourceLimits("5", "4"), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, }, { "Invalid env from", @@ -7500,12 +7699,29 @@ func TestValidateContainers(t *testing.T) { }, }, }, + []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].envFrom[0].configMapRef.name"}}, }, } for _, tc := range errorCases { - if errs := validateContainers(tc.containers, false, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { - t.Errorf("expected failure for %s", tc.title) - } + t.Run(tc.title, func(t *testing.T) { + errs := validateContainers(tc.containers, false, volumeDevices, field.NewPath("containers"), PodValidationOptions{}) + if len(errs) == 0 { + t.Fatalf("expected error but received none") + } + + if len(errs) != len(tc.expectedErrors) { + t.Fatalf("unexpected number of errors (got %d, want %d) in validation result %q", len(errs), len(tc.expectedErrors), errs) + } + + for i, err := range errs { + if err.Type != tc.expectedErrors[i].Type { + t.Errorf("returned error %d of %d: got error type %q but wanted %q: %q", i+1, len(errs), string(err.Type), string(tc.expectedErrors[i].Type), err) + } + if errs[i].Field != tc.expectedErrors[i].Field { + t.Errorf("returned error %d of %d: got error for field %q but wanted error for field %q: %q", i+1, len(errs), err.Field, tc.expectedErrors[i].Field, err) + } + } + }) } } @@ -7515,6 +7731,15 @@ func TestValidateInitContainers(t *testing.T) { AllowPrivileged: true, }) + containers := []core.Container{ + { + Name: "app", + Image: "nginx", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + } + successCase := []core.Container{ { Name: "container-1-same-host-port-different-protocol", @@ -7537,7 +7762,7 @@ func TestValidateInitContainers(t *testing.T) { TerminationMessagePolicy: "File", }, } - if errs := validateContainers(successCase, true, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { + if errs := validateInitContainers(successCase, containers, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) } @@ -7547,7 +7772,53 @@ func TestValidateInitContainers(t *testing.T) { errorCases := []struct { title string initContainers []core.Container + expectedError field.Error }{ + { + "name collision with regular container", + []core.Container{ + { + Name: "app", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[0].name"}, + }, + { + "invalid termination message policy", + []core.Container{ + { + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "INVALID_POLICY_NAME", + }, + }, + field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].terminationMessagePolicy"}, + }, + /* + TODO: Validation incorrectly returns duplicate errors for duplicate names. + { + "duplicate names", + []core.Container{ + { + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + { + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name"}, + }, + */ { "duplicate ports", []core.Container{ @@ -7566,12 +7837,96 @@ func TestValidateInitContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, + field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[0].ports[1].hostPort"}, + }, + { + "uses disallowed field: Lifecycle", + []core.Container{ + { + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + Exec: &core.ExecAction{Command: []string{"ls", "-l"}}, + }, + }, + }, + }, + field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].lifecycle"}, + }, + { + "uses disallowed field: LivenessProbe", + []core.Container{ + { + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + LivenessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{Port: intstr.FromInt(80)}, + }, + SuccessThreshold: 1, + }, + }, + }, + field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].livenessProbe"}, + }, + { + "uses disallowed field: ReadinessProbe", + []core.Container{ + { + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + ReadinessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{Port: intstr.FromInt(80)}, + }, + }, + }, + }, + field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].readinessProbe"}, + }, + { + "Container uses disallowed field: StartupProbe", + []core.Container{ + { + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + StartupProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{Port: intstr.FromInt(80)}, + }, + SuccessThreshold: 1, + }, + }, + }, + field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].startupProbe"}, }, } for _, tc := range errorCases { - if errs := validateContainers(tc.initContainers, true, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { - t.Errorf("expected failure for %s", tc.title) - } + t.Run(tc.title, func(t *testing.T) { + errs := validateInitContainers(tc.initContainers, containers, volumeDevices, field.NewPath("initContainers"), PodValidationOptions{}) + if len(errs) == 0 { + t.Fatal("expected error but received none") + } + + if len(errs) > 1 { + t.Errorf("expected 1 error but received %d: %q", len(errs), errs) + } + if errs[0].Type != tc.expectedError.Type { + t.Errorf("expected error type %q but received %q: %q", string(tc.expectedError.Type), string(errs[0].Type), errs) + } + if errs[0].Field != tc.expectedError.Field { + t.Errorf("expected error for field %q but received error for field %q: %q", tc.expectedError.Field, errs[0].Field, errs) + } + }) } } From 1dc040082c3bead76c8731c7f94eba4a0265ca10 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Sun, 24 Jul 2022 15:52:03 +0200 Subject: [PATCH 3/4] Refactor container validation Refactor common validation into methods that validate a single container and call these methods when iterating the three types of container lists. Move initContainer-specific validation from validateContainers to validateInitContainers. This resolves issues where init and ephemeral containers would return duplicate or incorrectly formatted errors for problems detected by validateContainers. --- pkg/apis/core/validation/validation.go | 166 ++++++++++---------- pkg/apis/core/validation/validation_test.go | 47 +++--- 2 files changed, 109 insertions(+), 104 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index b0ad047073f..ffb91a497b0 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2930,18 +2930,14 @@ func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, } if ec.Name == "" { - allErrs = append(allErrs, field.Required(idxPath, "ephemeralContainer requires a name")) + allErrs = append(allErrs, field.Required(idxPath.Child("name"), "ephemeralContainer requires a name")) continue } - // Using validateContainers() here isn't ideal because it adds an index to the error message that - // doesn't really exist for EphemeralContainers (i.e. ephemeralContainers[0].spec[0].name instead - // of ephemeralContainers[0].spec.name) - // TODO(verb): factor a validateContainer() out of validateContainers() to be used here - c := core.Container(ec.EphemeralContainerCommon) - allErrs = append(allErrs, validateContainers([]core.Container{c}, false, volumes, idxPath, opts)...) + c := (*core.Container)(&ec.EphemeralContainerCommon) + allErrs = append(allErrs, validateContainer(c, volumes, idxPath, opts)...) // EphemeralContainers don't require the backwards-compatibility distinction between pod/podTemplate validation - allErrs = append(allErrs, validateContainersOnlyForPod([]core.Container{c}, idxPath)...) + allErrs = append(allErrs, validateContainerOnlyForPod(c, idxPath)...) if allNames.Has(ec.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ec.Name)) @@ -2993,11 +2989,8 @@ func validateFieldAllowList(value interface{}, allowedFields map[string]bool, er return allErrs } -func validateInitContainers(containers []core.Container, otherContainers []core.Container, deviceVolumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateInitContainers(containers []core.Container, otherContainers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { var allErrs field.ErrorList - if len(containers) > 0 { - allErrs = append(allErrs, validateContainers(containers, true, deviceVolumes, fldPath, opts)...) - } allNames := sets.String{} for _, ctr := range otherContainers { @@ -3007,10 +3000,15 @@ func validateInitContainers(containers []core.Container, otherContainers []core. idxPath := fldPath.Index(i) if allNames.Has(ctr.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) - } - if len(ctr.Name) > 0 { + } else if len(ctr.Name) > 0 { allNames.Insert(ctr.Name) } + + allErrs = append(allErrs, validateContainer(&ctr, volumes, idxPath, opts)...) + + // Check for port conflicts in init containers individually since init containers run one-by-one. + allErrs = append(allErrs, checkHostPortConflicts([]core.Container{ctr}, fldPath)...) + if ctr.Lifecycle != nil { allErrs = append(allErrs, field.Invalid(idxPath.Child("lifecycle"), ctr.Lifecycle, "must not be set for init containers")) } @@ -3024,10 +3022,70 @@ func validateInitContainers(containers []core.Container, otherContainers []core. allErrs = append(allErrs, field.Invalid(idxPath.Child("startupProbe"), ctr.StartupProbe, "must not be set for init containers")) } } + return allErrs } -func validateContainers(containers []core.Container, isInitContainers bool, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateContainer(ctr *core.Container, volumes map[string]core.VolumeSource, path *field.Path, opts PodValidationOptions) field.ErrorList { + allErrs := field.ErrorList{} + + namePath := path.Child("name") + if len(ctr.Name) == 0 { + allErrs = append(allErrs, field.Required(namePath, "")) + } else { + allErrs = append(allErrs, ValidateDNS1123Label(ctr.Name, namePath)...) + } + + // TODO: do not validate leading and trailing whitespace to preserve backward compatibility. + // for example: https://github.com/openshift/origin/issues/14659 image = " " is special token in pod template + // others may have done similar + if len(ctr.Image) == 0 { + allErrs = append(allErrs, field.Required(path.Child("image"), "")) + } + if ctr.Lifecycle != nil { + allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, path.Child("lifecycle"))...) + } + + allErrs = append(allErrs, validateProbe(ctr.LivenessProbe, path.Child("livenessProbe"))...) + // Readiness-specific validation + if ctr.ReadinessProbe != nil && ctr.ReadinessProbe.TerminationGracePeriodSeconds != nil { + allErrs = append(allErrs, field.Invalid(path.Child("readinessProbe", "terminationGracePeriodSeconds"), ctr.ReadinessProbe.TerminationGracePeriodSeconds, "must not be set for readinessProbes")) + } + allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) + // Liveness-specific validation + if ctr.LivenessProbe != nil && ctr.LivenessProbe.SuccessThreshold != 1 { + allErrs = append(allErrs, field.Invalid(path.Child("livenessProbe", "successThreshold"), ctr.LivenessProbe.SuccessThreshold, "must be 1")) + } + allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) + // Startup-specific validation + if ctr.StartupProbe != nil && ctr.StartupProbe.SuccessThreshold != 1 { + allErrs = append(allErrs, field.Invalid(path.Child("startupProbe", "successThreshold"), ctr.StartupProbe.SuccessThreshold, "must be 1")) + } + + switch ctr.TerminationMessagePolicy { + case core.TerminationMessageReadFile, core.TerminationMessageFallbackToLogsOnError: + case "": + allErrs = append(allErrs, field.Required(path.Child("terminationMessagePolicy"), "must be 'File' or 'FallbackToLogsOnError'")) + default: + allErrs = append(allErrs, field.Invalid(path.Child("terminationMessagePolicy"), ctr.TerminationMessagePolicy, "must be 'File' or 'FallbackToLogsOnError'")) + } + + volMounts := GetVolumeMountMap(ctr.VolumeMounts) + volDevices := GetVolumeDeviceMap(ctr.VolumeDevices) + allErrs = append(allErrs, validateProbe(ctr.ReadinessProbe, path.Child("readinessProbe"))...) + allErrs = append(allErrs, validateContainerPorts(ctr.Ports, path.Child("ports"))...) + allErrs = append(allErrs, ValidateEnv(ctr.Env, path.Child("env"), opts)...) + allErrs = append(allErrs, ValidateEnvFrom(ctr.EnvFrom, path.Child("envFrom"))...) + allErrs = append(allErrs, ValidateVolumeMounts(ctr.VolumeMounts, volDevices, volumes, ctr, path.Child("volumeMounts"))...) + allErrs = append(allErrs, ValidateVolumeDevices(ctr.VolumeDevices, volMounts, volumes, path.Child("volumeDevices"))...) + allErrs = append(allErrs, validatePullPolicy(ctr.ImagePullPolicy, path.Child("imagePullPolicy"))...) + allErrs = append(allErrs, ValidateResourceRequirements(&ctr.Resources, path.Child("resources"), opts)...) + allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, path.Child("securityContext"))...) + + return allErrs +} + +func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if len(containers) == 0 { @@ -3037,73 +3095,18 @@ func validateContainers(containers []core.Container, isInitContainers bool, volu allNames := sets.String{} for i, ctr := range containers { idxPath := fldPath.Index(i) - namePath := idxPath.Child("name") - volMounts := GetVolumeMountMap(ctr.VolumeMounts) - volDevices := GetVolumeDeviceMap(ctr.VolumeDevices) - if len(ctr.Name) == 0 { - allErrs = append(allErrs, field.Required(namePath, "")) - } else { - allErrs = append(allErrs, ValidateDNS1123Label(ctr.Name, namePath)...) - } if allNames.Has(ctr.Name) { - allErrs = append(allErrs, field.Duplicate(namePath, ctr.Name)) + allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) } else { allNames.Insert(ctr.Name) } - // TODO: do not validate leading and trailing whitespace to preserve backward compatibility. - // for example: https://github.com/openshift/origin/issues/14659 image = " " is special token in pod template - // others may have done similar - if len(ctr.Image) == 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("image"), "")) - } - if ctr.Lifecycle != nil { - allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, idxPath.Child("lifecycle"))...) - } - allErrs = append(allErrs, validateProbe(ctr.LivenessProbe, idxPath.Child("livenessProbe"))...) - // Readiness-specific validation - if ctr.ReadinessProbe != nil && ctr.ReadinessProbe.TerminationGracePeriodSeconds != nil { - allErrs = append(allErrs, field.Invalid(idxPath.Child("readinessProbe", "terminationGracePeriodSeconds"), ctr.ReadinessProbe.TerminationGracePeriodSeconds, "must not be set for readinessProbes")) - } - allErrs = append(allErrs, validateProbe(ctr.StartupProbe, idxPath.Child("startupProbe"))...) - // Liveness-specific validation - if ctr.LivenessProbe != nil && ctr.LivenessProbe.SuccessThreshold != 1 { - allErrs = append(allErrs, field.Invalid(idxPath.Child("livenessProbe", "successThreshold"), ctr.LivenessProbe.SuccessThreshold, "must be 1")) - } - allErrs = append(allErrs, validateProbe(ctr.StartupProbe, idxPath.Child("startupProbe"))...) - // Startup-specific validation - if ctr.StartupProbe != nil && ctr.StartupProbe.SuccessThreshold != 1 { - allErrs = append(allErrs, field.Invalid(idxPath.Child("startupProbe", "successThreshold"), ctr.StartupProbe.SuccessThreshold, "must be 1")) - } - switch ctr.TerminationMessagePolicy { - case core.TerminationMessageReadFile, core.TerminationMessageFallbackToLogsOnError: - case "": - allErrs = append(allErrs, field.Required(idxPath.Child("terminationMessagePolicy"), "must be 'File' or 'FallbackToLogsOnError'")) - default: - allErrs = append(allErrs, field.Invalid(idxPath.Child("terminationMessagePolicy"), ctr.TerminationMessagePolicy, "must be 'File' or 'FallbackToLogsOnError'")) - } - - allErrs = append(allErrs, validateProbe(ctr.ReadinessProbe, idxPath.Child("readinessProbe"))...) - allErrs = append(allErrs, validateContainerPorts(ctr.Ports, idxPath.Child("ports"))...) - allErrs = append(allErrs, ValidateEnv(ctr.Env, idxPath.Child("env"), opts)...) - allErrs = append(allErrs, ValidateEnvFrom(ctr.EnvFrom, idxPath.Child("envFrom"))...) - allErrs = append(allErrs, ValidateVolumeMounts(ctr.VolumeMounts, volDevices, volumes, &ctr, idxPath.Child("volumeMounts"))...) - allErrs = append(allErrs, ValidateVolumeDevices(ctr.VolumeDevices, volMounts, volumes, idxPath.Child("volumeDevices"))...) - allErrs = append(allErrs, validatePullPolicy(ctr.ImagePullPolicy, idxPath.Child("imagePullPolicy"))...) - allErrs = append(allErrs, ValidateResourceRequirements(&ctr.Resources, idxPath.Child("resources"), opts)...) - allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, idxPath.Child("securityContext"))...) + allErrs = append(allErrs, validateContainer(&ctr, volumes, idxPath, opts)...) } - if isInitContainers { - // check initContainers one by one since they are running in sequential order. - for _, initContainer := range containers { - allErrs = append(allErrs, checkHostPortConflicts([]core.Container{initContainer}, fldPath)...) - } - } else { - // Check for colliding ports across all containers. - allErrs = append(allErrs, checkHostPortConflicts(containers, fldPath)...) - } + // Check for colliding ports across all containers. + allErrs = append(allErrs, checkHostPortConflicts(containers, fldPath)...) return allErrs } @@ -3393,10 +3396,15 @@ func ValidateTolerations(tolerations []core.Toleration, fldPath *field.Path) fie func validateContainersOnlyForPod(containers []core.Container, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, ctr := range containers { - idxPath := fldPath.Index(i) - if len(ctr.Image) != len(strings.TrimSpace(ctr.Image)) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("image"), ctr.Image, "must not have leading or trailing whitespace")) - } + allErrs = append(allErrs, validateContainerOnlyForPod(&ctr, fldPath.Index(i))...) + } + return allErrs +} + +func validateContainerOnlyForPod(ctr *core.Container, path *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if len(ctr.Image) != len(strings.TrimSpace(ctr.Image)) { + allErrs = append(allErrs, field.Invalid(path.Child("image"), ctr.Image, "must not have leading or trailing whitespace")) } return allErrs } @@ -3503,7 +3511,7 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi vols, vErrs := ValidateVolumes(spec.Volumes, podMeta, fldPath.Child("volumes"), opts) allErrs = append(allErrs, vErrs...) - allErrs = append(allErrs, validateContainers(spec.Containers, false, vols, fldPath.Child("containers"), opts)...) + allErrs = append(allErrs, validateContainers(spec.Containers, vols, fldPath.Child("containers"), opts)...) allErrs = append(allErrs, validateInitContainers(spec.InitContainers, spec.Containers, vols, fldPath.Child("initContainers"), opts)...) allErrs = append(allErrs, validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, fldPath.Child("ephemeralContainers"), opts)...) allErrs = append(allErrs, validateRestartPolicy(&spec.RestartPolicy, fldPath.Child("restartPolicy"))...) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 9504a15caba..00ea5fbcbb3 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6747,28 +6747,28 @@ func TestValidateEphemeralContainers(t *testing.T) { []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{}}, }, - field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0]"}, + field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, }, { "empty Container Name", []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0]"}, + field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, }, { "whitespace padded image name", []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: " image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0][0].image"}, + field.Error{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0].image"}, }, { "invalid image pull policy", []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "PullThreeTimes", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0][0].imagePullPolicy"}, + field.Error{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0].imagePullPolicy"}, }, { "TargetContainerName doesn't exist", @@ -7218,7 +7218,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, } - if errs := validateContainers(successCase, false, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { + if errs := validateContainers(successCase, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) } @@ -7704,7 +7704,7 @@ func TestValidateContainers(t *testing.T) { } for _, tc := range errorCases { t.Run(tc.title, func(t *testing.T) { - errs := validateContainers(tc.containers, false, volumeDevices, field.NewPath("containers"), PodValidationOptions{}) + errs := validateContainers(tc.containers, volumeDevices, field.NewPath("containers"), PodValidationOptions{}) if len(errs) == 0 { t.Fatalf("expected error but received none") } @@ -7798,27 +7798,24 @@ func TestValidateInitContainers(t *testing.T) { }, field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].terminationMessagePolicy"}, }, - /* - TODO: Validation incorrectly returns duplicate errors for duplicate names. + { + "duplicate names", + []core.Container{ { - "duplicate names", - []core.Container{ - { - Name: "init", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - { - Name: "init", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name"}, + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - */ + { + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name"}, + }, { "duplicate ports", []core.Container{ From 537e73601d6d5ddb7d5c4e5395cba67ec6b9b384 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Thu, 28 Jul 2022 19:38:59 +0200 Subject: [PATCH 4/4] Further cleanup of container validation --- pkg/apis/core/validation/validation.go | 121 +++--- pkg/apis/core/validation/validation_test.go | 433 +++++++++++++++----- 2 files changed, 408 insertions(+), 146 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index ffb91a497b0..880a3cd4983 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2907,6 +2907,8 @@ func validatePullPolicy(policy core.PullPolicy, fldPath *field.Path) field.Error return allErrors } +// validateEphemeralContainers is called by pod spec and template validation to validate the list of ephemeral containers. +// Note that this is called for pod template even though ephemeral containers aren't allowed in pod templates. func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, containers, initContainers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} @@ -2914,40 +2916,40 @@ func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, return allErrs } - allNames := sets.String{} + otherNames, allNames := sets.String{}, sets.String{} for _, c := range containers { + otherNames.Insert(c.Name) allNames.Insert(c.Name) } for _, c := range initContainers { + otherNames.Insert(c.Name) allNames.Insert(c.Name) } for i, ec := range ephemeralContainers { idxPath := fldPath.Index(i) - if ec.TargetContainerName != "" && !allNames.Has(ec.TargetContainerName) { - allErrs = append(allErrs, field.NotFound(idxPath.Child("targetContainerName"), ec.TargetContainerName)) - } - - if ec.Name == "" { - allErrs = append(allErrs, field.Required(idxPath.Child("name"), "ephemeralContainer requires a name")) - continue - } - c := (*core.Container)(&ec.EphemeralContainerCommon) - allErrs = append(allErrs, validateContainer(c, volumes, idxPath, opts)...) - // EphemeralContainers don't require the backwards-compatibility distinction between pod/podTemplate validation + allErrs = append(allErrs, validateContainerCommon(c, volumes, idxPath, opts)...) + // Ephemeral containers don't need looser constraints for pod templates, so it's convenient to apply both validations + // here where we've already converted EphemeralContainerCommon to Container. allErrs = append(allErrs, validateContainerOnlyForPod(c, idxPath)...) + // Ephemeral containers must have a name unique across all container types. if allNames.Has(ec.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ec.Name)) } else { allNames.Insert(ec.Name) } - // Ephemeral Containers should not be relied upon for fundamental pod services, so fields such as + // The target container name must exist and be non-ephemeral. + if ec.TargetContainerName != "" && !otherNames.Has(ec.TargetContainerName) { + allErrs = append(allErrs, field.NotFound(idxPath.Child("targetContainerName"), ec.TargetContainerName)) + } + + // Ephemeral containers should not be relied upon for fundamental pod services, so fields such as // Lifecycle, probes, resources and ports should be disallowed. This is implemented as a list - // of allowed fields so that new fields will be given consideration prior to inclusion in Ephemeral Containers. + // of allowed fields so that new fields will be given consideration prior to inclusion in ephemeral containers. allErrs = append(allErrs, validateFieldAllowList(ec.EphemeralContainerCommon, allowedEphemeralContainerFields, "cannot be set for an Ephemeral Container", idxPath)...) // VolumeMount subpaths have the potential to leak resources since they're implemented with bind mounts @@ -2989,44 +2991,52 @@ func validateFieldAllowList(value interface{}, allowedFields map[string]bool, er return allErrs } -func validateInitContainers(containers []core.Container, otherContainers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +// validateInitContainers is called by pod spec and template validation to validate the list of init containers +func validateInitContainers(containers []core.Container, regularContainers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { var allErrs field.ErrorList allNames := sets.String{} - for _, ctr := range otherContainers { + for _, ctr := range regularContainers { allNames.Insert(ctr.Name) } for i, ctr := range containers { idxPath := fldPath.Index(i) + + // Apply the validation common to all container types + allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, idxPath, opts)...) + + // Names must be unique within regular and init containers. Collisions with ephemeral containers + // will be detected by validateEphemeralContainers(). if allNames.Has(ctr.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) } else if len(ctr.Name) > 0 { allNames.Insert(ctr.Name) } - allErrs = append(allErrs, validateContainer(&ctr, volumes, idxPath, opts)...) - // Check for port conflicts in init containers individually since init containers run one-by-one. allErrs = append(allErrs, checkHostPortConflicts([]core.Container{ctr}, fldPath)...) + // These fields are disallowed for init containers. if ctr.Lifecycle != nil { - allErrs = append(allErrs, field.Invalid(idxPath.Child("lifecycle"), ctr.Lifecycle, "must not be set for init containers")) + allErrs = append(allErrs, field.Forbidden(idxPath.Child("lifecycle"), "may not be set for init containers")) } if ctr.LivenessProbe != nil { - allErrs = append(allErrs, field.Invalid(idxPath.Child("livenessProbe"), ctr.LivenessProbe, "must not be set for init containers")) + allErrs = append(allErrs, field.Forbidden(idxPath.Child("livenessProbe"), "may not be set for init containers")) } if ctr.ReadinessProbe != nil { - allErrs = append(allErrs, field.Invalid(idxPath.Child("readinessProbe"), ctr.ReadinessProbe, "must not be set for init containers")) + allErrs = append(allErrs, field.Forbidden(idxPath.Child("readinessProbe"), "may not be set for init containers")) } if ctr.StartupProbe != nil { - allErrs = append(allErrs, field.Invalid(idxPath.Child("startupProbe"), ctr.StartupProbe, "must not be set for init containers")) + allErrs = append(allErrs, field.Forbidden(idxPath.Child("startupProbe"), "may not be set for init containers")) } } return allErrs } -func validateContainer(ctr *core.Container, volumes map[string]core.VolumeSource, path *field.Path, opts PodValidationOptions) field.ErrorList { +// validateContainerCommon applies validation common to all container types. It's called by regular, init, and ephemeral +// container list validation to require a properly formatted name, image, etc. +func validateContainerCommon(ctr *core.Container, volumes map[string]core.VolumeSource, path *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} namePath := path.Child("name") @@ -3042,37 +3052,21 @@ func validateContainer(ctr *core.Container, volumes map[string]core.VolumeSource if len(ctr.Image) == 0 { allErrs = append(allErrs, field.Required(path.Child("image"), "")) } - if ctr.Lifecycle != nil { - allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, path.Child("lifecycle"))...) - } - - allErrs = append(allErrs, validateProbe(ctr.LivenessProbe, path.Child("livenessProbe"))...) - // Readiness-specific validation - if ctr.ReadinessProbe != nil && ctr.ReadinessProbe.TerminationGracePeriodSeconds != nil { - allErrs = append(allErrs, field.Invalid(path.Child("readinessProbe", "terminationGracePeriodSeconds"), ctr.ReadinessProbe.TerminationGracePeriodSeconds, "must not be set for readinessProbes")) - } - allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) - // Liveness-specific validation - if ctr.LivenessProbe != nil && ctr.LivenessProbe.SuccessThreshold != 1 { - allErrs = append(allErrs, field.Invalid(path.Child("livenessProbe", "successThreshold"), ctr.LivenessProbe.SuccessThreshold, "must be 1")) - } - allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) - // Startup-specific validation - if ctr.StartupProbe != nil && ctr.StartupProbe.SuccessThreshold != 1 { - allErrs = append(allErrs, field.Invalid(path.Child("startupProbe", "successThreshold"), ctr.StartupProbe.SuccessThreshold, "must be 1")) - } switch ctr.TerminationMessagePolicy { case core.TerminationMessageReadFile, core.TerminationMessageFallbackToLogsOnError: case "": - allErrs = append(allErrs, field.Required(path.Child("terminationMessagePolicy"), "must be 'File' or 'FallbackToLogsOnError'")) + allErrs = append(allErrs, field.Required(path.Child("terminationMessagePolicy"), "")) default: - allErrs = append(allErrs, field.Invalid(path.Child("terminationMessagePolicy"), ctr.TerminationMessagePolicy, "must be 'File' or 'FallbackToLogsOnError'")) + supported := []string{ + string(core.TerminationMessageReadFile), + string(core.TerminationMessageFallbackToLogsOnError), + } + allErrs = append(allErrs, field.NotSupported(path.Child("terminationMessagePolicy"), ctr.TerminationMessagePolicy, supported)) } volMounts := GetVolumeMountMap(ctr.VolumeMounts) volDevices := GetVolumeDeviceMap(ctr.VolumeDevices) - allErrs = append(allErrs, validateProbe(ctr.ReadinessProbe, path.Child("readinessProbe"))...) allErrs = append(allErrs, validateContainerPorts(ctr.Ports, path.Child("ports"))...) allErrs = append(allErrs, ValidateEnv(ctr.Env, path.Child("env"), opts)...) allErrs = append(allErrs, ValidateEnvFrom(ctr.EnvFrom, path.Child("envFrom"))...) @@ -3085,6 +3079,7 @@ func validateContainer(ctr *core.Container, volumes map[string]core.VolumeSource return allErrs } +// validateContainers is called by pod spec and template validation to validate the list of regular containers. func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} @@ -3094,18 +3089,40 @@ func validateContainers(containers []core.Container, volumes map[string]core.Vol allNames := sets.String{} for i, ctr := range containers { - idxPath := fldPath.Index(i) + path := fldPath.Index(i) + // Apply validation common to all containers + allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, path, opts)...) + + // Container names must be unique within the list of regular containers. + // Collisions with init or ephemeral container names will be detected by the init or ephemeral + // container validation to prevent duplicate error messages. if allNames.Has(ctr.Name) { - allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) + allErrs = append(allErrs, field.Duplicate(path.Child("name"), ctr.Name)) } else { allNames.Insert(ctr.Name) } - allErrs = append(allErrs, validateContainer(&ctr, volumes, idxPath, opts)...) + // These fields are only allowed for regular containers, so only check supported values here. + // Init and ephemeral container validation will return field.Forbidden() for these paths. + if ctr.Lifecycle != nil { + allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, path.Child("lifecycle"))...) + } + allErrs = append(allErrs, validateProbe(ctr.LivenessProbe, path.Child("livenessProbe"))...) + if ctr.LivenessProbe != nil && ctr.LivenessProbe.SuccessThreshold != 1 { + allErrs = append(allErrs, field.Invalid(path.Child("livenessProbe", "successThreshold"), ctr.LivenessProbe.SuccessThreshold, "must be 1")) + } + allErrs = append(allErrs, validateProbe(ctr.ReadinessProbe, path.Child("readinessProbe"))...) + if ctr.ReadinessProbe != nil && ctr.ReadinessProbe.TerminationGracePeriodSeconds != nil { + allErrs = append(allErrs, field.Invalid(path.Child("readinessProbe", "terminationGracePeriodSeconds"), ctr.ReadinessProbe.TerminationGracePeriodSeconds, "must not be set for readinessProbes")) + } + allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) + if ctr.StartupProbe != nil && ctr.StartupProbe.SuccessThreshold != 1 { + allErrs = append(allErrs, field.Invalid(path.Child("startupProbe", "successThreshold"), ctr.StartupProbe.SuccessThreshold, "must be 1")) + } } - // Check for colliding ports across all containers. + // Port conflicts are checked across all containers allErrs = append(allErrs, checkHostPortConflicts(containers, fldPath)...) return allErrs @@ -3392,7 +3409,8 @@ func ValidateTolerations(tolerations []core.Toleration, fldPath *field.Path) fie } // validateContainersOnlyForPod does additional validation for containers on a pod versus a pod template -// it only does additive validation of fields not covered in validateContainers +// it only does additive validation of fields not covered in validateContainers and is not called for +// ephemeral containers which require a conversion to core.Container. func validateContainersOnlyForPod(containers []core.Container, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, ctr := range containers { @@ -3401,6 +3419,8 @@ func validateContainersOnlyForPod(containers []core.Container, fldPath *field.Pa return allErrs } +// validateContainerOnlyForPod does pod-only (i.e. not pod template) validation for a single container. +// This is called by validateContainersOnlyForPod and validateEphemeralContainers directly. func validateContainerOnlyForPod(ctr *core.Container, path *field.Path) field.ErrorList { allErrs := field.ErrorList{} if len(ctr.Image) != len(strings.TrimSpace(ctr.Image)) { @@ -3451,6 +3471,7 @@ func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field. allErrs = append(allErrs, validateContainersOnlyForPod(pod.Spec.Containers, specPath.Child("containers"))...) allErrs = append(allErrs, validateContainersOnlyForPod(pod.Spec.InitContainers, specPath.Child("initContainers"))...) + // validateContainersOnlyForPod() is checked for ephemeral containers by validateEphemeralContainers() return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 00ea5fbcbb3..6e827b7a591 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -21,11 +21,13 @@ import ( "fmt" "math" "reflect" + "runtime" "strings" "testing" "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" @@ -51,6 +53,25 @@ const ( envVarNameErrMsg = "a valid environment variable name must consist of" ) +func line() string { + _, _, line, ok := runtime.Caller(1) + var s string + if ok { + s = fmt.Sprintf("%d", line) + } else { + s = "" + } + return s +} + +func prettyErrorList(errs field.ErrorList) string { + var s string + for _, e := range errs { + s += fmt.Sprintf("\t%s\n", e) + } + return s +} + func newHostPathType(pathType string) *core.HostPathType { hostPathType := new(core.HostPathType) *hostPathType = core.HostPathType(pathType) @@ -6714,74 +6735,102 @@ func TestValidateEphemeralContainers(t *testing.T) { // Failure Cases tcs := []struct { - title string + title, line string ephemeralContainers []core.EphemeralContainer - expectedError field.Error + expectedErrors field.ErrorList }{ { "Name Collision with Container.Containers", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug1", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "ephemeralContainers[0].name"}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "ephemeralContainers[0].name"}}, }, { "Name Collision with Container.InitContainers", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "ictr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug1", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "ephemeralContainers[0].name"}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "ephemeralContainers[0].name"}}, }, { "Name Collision with EphemeralContainers", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug1", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug1", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "ephemeralContainers[1].name"}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "ephemeralContainers[1].name"}}, }, { - "empty Container Container", + "empty Container", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{}}, }, - field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, + field.ErrorList{ + {Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, + {Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].image"}, + {Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].terminationMessagePolicy"}, + {Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].imagePullPolicy"}, + }, }, { "empty Container Name", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}}, }, { "whitespace padded image name", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: " image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0].image"}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0].image"}}, }, { "invalid image pull policy", + line(), []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "PullThreeTimes", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0].imagePullPolicy"}, + field.ErrorList{{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0].imagePullPolicy"}}, }, { "TargetContainerName doesn't exist", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, TargetContainerName: "bogus", }, }, - field.Error{Type: field.ErrorTypeNotFound, Field: "ephemeralContainers[0].targetContainerName"}, + field.ErrorList{{Type: field.ErrorTypeNotFound, Field: "ephemeralContainers[0].targetContainerName"}}, + }, + { + "Targets an ephemeral container", + line(), + []core.EphemeralContainer{ + { + EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + }, + { + EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debugception", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + TargetContainerName: "debug", + }, + }, + field.ErrorList{{Type: field.ErrorTypeNotFound, Field: "ephemeralContainers[1].targetContainerName"}}, }, { "Container uses disallowed field: Lifecycle", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6797,10 +6846,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].lifecycle"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].lifecycle"}}, }, { "Container uses disallowed field: LivenessProbe", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6817,10 +6867,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].livenessProbe"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].livenessProbe"}}, }, { "Container uses disallowed field: Ports", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6834,10 +6885,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].ports"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].ports"}}, }, { "Container uses disallowed field: ReadinessProbe", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6853,10 +6905,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].readinessProbe"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].readinessProbe"}}, }, { "Container uses disallowed field: StartupProbe", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6873,10 +6926,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].startupProbe"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].startupProbe"}}, }, { "Container uses disallowed field: Resources", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6892,10 +6946,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].resources"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].resources"}}, }, { "Container uses disallowed field: VolumeMount.SubPath", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6910,10 +6965,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].volumeMounts[1].subPath"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].volumeMounts[1].subPath"}}, }, { "Container uses disallowed field: VolumeMount.SubPathExpr", + line(), []core.EphemeralContainer{ { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -6928,25 +6984,40 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].volumeMounts[1].subPathExpr"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].volumeMounts[1].subPathExpr"}}, + }, + { + "Disallowed field with other errors should only return a single Forbidden", + line(), + []core.EphemeralContainer{ + { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + Lifecycle: &core.Lifecycle{ + PreStop: &core.LifecycleHandler{ + Exec: &core.ExecAction{Command: []string{}}, + }, + }, + }, + }, + }, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].lifecycle"}}, }, } for _, tc := range tcs { - t.Run(tc.title, func(t *testing.T) { + t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { errs := validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, field.NewPath("ephemeralContainers"), PodValidationOptions{}) if len(errs) == 0 { t.Fatal("expected error but received none") } - if len(errs) > 1 { - t.Errorf("expected 1 error but received %d: %q", len(errs), errs) - } - if errs[0].Type != tc.expectedError.Type { - t.Errorf("expected error type %q but received %q: %q", string(tc.expectedError.Type), string(errs[0].Type), errs) - } - if errs[0].Field != tc.expectedError.Field { - t.Errorf("expected error for field %q but received error for field %q: %q", tc.expectedError.Field, errs[0].Field, errs) + if diff := cmp.Diff(tc.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")); diff != "" { + t.Errorf("unexpected diff in errors (-want, +got):\n%s", diff) + t.Errorf("INFO: all errors:\n%s", prettyErrorList(errs)) } }) } @@ -7226,70 +7297,80 @@ func TestValidateContainers(t *testing.T) { AllowPrivileged: false, }) errorCases := []struct { - title string + title, line string containers []core.Container - expectedErrors []field.Error + expectedErrors field.ErrorList }{ { "zero-length name", + line(), []core.Container{{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].name"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].name"}}, }, { "zero-length-image", + line(), []core.Container{{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].image"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].image"}}, }, { "name > 63 characters", + line(), []core.Container{{Name: strings.Repeat("a", 64), Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].name"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].name"}}, }, { "name not a DNS label", + line(), []core.Container{{Name: "a.b.c", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].name"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].name"}}, }, { "name not unique", + line(), []core.Container{ {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, {Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, - []field.Error{{Type: field.ErrorTypeDuplicate, Field: "containers[1].name"}}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "containers[1].name"}}, }, { "zero-length image", + line(), []core.Container{{Name: "abc", Image: "", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].image"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].image"}}, }, { "host port not unique", + line(), []core.Container{ {Name: "abc", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 80, HostPort: 80, Protocol: "TCP"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, {Name: "def", Image: "image", Ports: []core.ContainerPort{{ContainerPort: 81, HostPort: 80, Protocol: "TCP"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, - []field.Error{{Type: field.ErrorTypeDuplicate, Field: "containers[1].ports[0].hostPort"}}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "containers[1].ports[0].hostPort"}}, }, { "invalid env var name", + line(), []core.Container{ {Name: "abc", Image: "image", Env: []core.EnvVar{{Name: "ev!1"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].env[0].name"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].env[0].name"}}, }, { "unknown volume name", + line(), []core.Container{ {Name: "abc", Image: "image", VolumeMounts: []core.VolumeMount{{Name: "anything", MountPath: "/foo"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, - []field.Error{{Type: field.ErrorTypeNotFound, Field: "containers[0].volumeMounts[0].name"}}, + field.ErrorList{{Type: field.ErrorTypeNotFound, Field: "containers[0].volumeMounts[0].name"}}, }, { "invalid lifecycle, no exec command.", + line(), []core.Container{ { Name: "life-123", @@ -7303,10 +7384,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop.exec.command"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop.exec.command"}}, }, { "invalid lifecycle, no http path.", + line(), []core.Container{ { Name: "life-123", @@ -7323,10 +7405,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop.httpGet.path"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop.httpGet.path"}}, }, { "invalid lifecycle, no http port.", + line(), []core.Container{ { Name: "life-123", @@ -7343,10 +7426,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.httpGet.port"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.httpGet.port"}}, }, { "invalid lifecycle, no http scheme.", + line(), []core.Container{ { Name: "life-123", @@ -7363,10 +7447,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeNotSupported, Field: "containers[0].lifecycle.preStop.httpGet.scheme"}}, + field.ErrorList{{Type: field.ErrorTypeNotSupported, Field: "containers[0].lifecycle.preStop.httpGet.scheme"}}, }, { "invalid lifecycle, no tcp socket port.", + line(), []core.Container{ { Name: "life-123", @@ -7380,10 +7465,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.tcpSocket.port"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.tcpSocket.port"}}, }, { "invalid lifecycle, zero tcp socket port.", + line(), []core.Container{ { Name: "life-123", @@ -7399,10 +7485,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.tcpSocket.port"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].lifecycle.preStop.tcpSocket.port"}}, }, { "invalid lifecycle, no action.", + line(), []core.Container{ { Name: "life-123", @@ -7414,10 +7501,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].lifecycle.preStop"}}, }, { "invalid readiness probe, terminationGracePeriodSeconds set.", + line(), []core.Container{ { Name: "life-123", @@ -7434,10 +7522,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.terminationGracePeriodSeconds"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.terminationGracePeriodSeconds"}}, }, { "invalid liveness probe, no tcp socket port.", + line(), []core.Container{ { Name: "live-123", @@ -7452,10 +7541,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.tcpSocket.port"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.tcpSocket.port"}}, }, { "invalid liveness probe, no action.", + line(), []core.Container{ { Name: "live-123", @@ -7468,10 +7558,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].livenessProbe"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].livenessProbe"}}, }, { "invalid liveness probe, successThreshold != 1", + line(), []core.Container{ { Name: "live-123", @@ -7488,10 +7579,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.successThreshold"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.successThreshold"}}, }, { "invalid startup probe, successThreshold != 1", + line(), []core.Container{ { Name: "startup-123", @@ -7508,10 +7600,116 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.successThreshold"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.successThreshold"}}, + }, + { + "invalid liveness probe, negative numbers", + line(), + []core.Container{ + { + Name: "live-123", + Image: "image", + LivenessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + InitialDelaySeconds: -1, + TimeoutSeconds: -1, + PeriodSeconds: -1, + SuccessThreshold: -1, + FailureThreshold: -1, + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.initialDelaySeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.timeoutSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.periodSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.successThreshold"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.failureThreshold"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.terminationGracePeriodSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].livenessProbe.successThreshold"}, + }, + }, + { + "invalid readiness probe, negative numbers", + line(), + []core.Container{ + { + Name: "ready-123", + Image: "image", + ReadinessProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + InitialDelaySeconds: -1, + TimeoutSeconds: -1, + PeriodSeconds: -1, + SuccessThreshold: -1, + FailureThreshold: -1, + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.initialDelaySeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.timeoutSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.periodSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.successThreshold"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.failureThreshold"}, + // terminationGracePeriodSeconds returns multiple validation errors here: + // containers[0].readinessProbe.terminationGracePeriodSeconds: Invalid value: -1: must be greater than 0 + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.terminationGracePeriodSeconds"}, + // containers[0].readinessProbe.terminationGracePeriodSeconds: Invalid value: -1: must not be set for readinessProbes + {Type: field.ErrorTypeInvalid, Field: "containers[0].readinessProbe.terminationGracePeriodSeconds"}, + }, + }, + { + "invalid startup probe, negative numbers", + line(), + []core.Container{ + { + Name: "startup-123", + Image: "image", + StartupProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{ + Port: intstr.FromInt(80), + }, + }, + InitialDelaySeconds: -1, + TimeoutSeconds: -1, + PeriodSeconds: -1, + SuccessThreshold: -1, + FailureThreshold: -1, + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1), + }, + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.initialDelaySeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.timeoutSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.periodSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.successThreshold"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.failureThreshold"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.terminationGracePeriodSeconds"}, + {Type: field.ErrorTypeInvalid, Field: "containers[0].startupProbe.successThreshold"}, + }, }, { "invalid message termination policy", + line(), []core.Container{ { Name: "life-123", @@ -7520,10 +7718,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "Unknown", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].terminationMessagePolicy"}}, + field.ErrorList{{Type: field.ErrorTypeNotSupported, Field: "containers[0].terminationMessagePolicy"}}, }, { "empty message termination policy", + line(), []core.Container{ { Name: "life-123", @@ -7532,10 +7731,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "", }, }, - []field.Error{{Type: field.ErrorTypeRequired, Field: "containers[0].terminationMessagePolicy"}}, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "containers[0].terminationMessagePolicy"}}, }, { "privilege disabled", + line(), []core.Container{ { Name: "abc", @@ -7545,10 +7745,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeForbidden, Field: "containers[0].securityContext.privileged"}}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "containers[0].securityContext.privileged"}}, }, { "invalid compute resource", + line(), []core.Container{ { Name: "abc-123", @@ -7562,13 +7763,14 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{ + field.ErrorList{ {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[disk]"}, {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[disk]"}, }, }, { "Resource CPU invalid", + line(), []core.Container{ { Name: "abc-123", @@ -7580,10 +7782,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[cpu]"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[cpu]"}}, }, { "Resource Requests CPU invalid", + line(), []core.Container{ { Name: "abc-123", @@ -7595,10 +7798,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests[cpu]"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests[cpu]"}}, }, { "Resource Memory invalid", + line(), []core.Container{ { Name: "abc-123", @@ -7610,10 +7814,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[memory]"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[memory]"}}, }, { "Request limit simple invalid", + line(), []core.Container{ { Name: "abc-123", @@ -7626,10 +7831,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, }, { "Invalid storage limit request", + line(), []core.Container{ { Name: "abc-123", @@ -7643,13 +7849,14 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{ + field.ErrorList{ {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[attachable-volumes-aws-ebs]"}, {Type: field.ErrorTypeInvalid, Field: "containers[0].resources.limits[attachable-volumes-aws-ebs]"}, }, }, { "CPU request limit multiple invalid", + line(), []core.Container{ { Name: "abc-123", @@ -7662,10 +7869,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, }, { "Memory request limit multiple invalid", + line(), []core.Container{ { Name: "abc-123", @@ -7678,10 +7886,11 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].resources.requests"}}, }, { "Invalid env from", + line(), []core.Container{ { Name: "env-from-source", @@ -7699,27 +7908,19 @@ func TestValidateContainers(t *testing.T) { }, }, }, - []field.Error{{Type: field.ErrorTypeInvalid, Field: "containers[0].envFrom[0].configMapRef.name"}}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "containers[0].envFrom[0].configMapRef.name"}}, }, } for _, tc := range errorCases { - t.Run(tc.title, func(t *testing.T) { + t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { errs := validateContainers(tc.containers, volumeDevices, field.NewPath("containers"), PodValidationOptions{}) if len(errs) == 0 { - t.Fatalf("expected error but received none") + t.Fatal("expected error but received none") } - if len(errs) != len(tc.expectedErrors) { - t.Fatalf("unexpected number of errors (got %d, want %d) in validation result %q", len(errs), len(tc.expectedErrors), errs) - } - - for i, err := range errs { - if err.Type != tc.expectedErrors[i].Type { - t.Errorf("returned error %d of %d: got error type %q but wanted %q: %q", i+1, len(errs), string(err.Type), string(tc.expectedErrors[i].Type), err) - } - if errs[i].Field != tc.expectedErrors[i].Field { - t.Errorf("returned error %d of %d: got error for field %q but wanted error for field %q: %q", i+1, len(errs), err.Field, tc.expectedErrors[i].Field, err) - } + if diff := cmp.Diff(tc.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")); diff != "" { + t.Errorf("unexpected diff in errors (-want, +got):\n%s", diff) + t.Errorf("INFO: all errors:\n%s", prettyErrorList(errs)) } }) } @@ -7770,12 +7971,26 @@ func TestValidateInitContainers(t *testing.T) { AllowPrivileged: false, }) errorCases := []struct { - title string + title, line string initContainers []core.Container - expectedError field.Error + expectedErrors field.ErrorList }{ + { + "empty name", + line(), + []core.Container{ + { + Name: "", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.ErrorList{{Type: field.ErrorTypeRequired, Field: "initContainers[0].name", BadValue: ""}}, + }, { "name collision with regular container", + line(), []core.Container{ { Name: "app", @@ -7784,22 +7999,24 @@ func TestValidateInitContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[0].name"}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "initContainers[0].name", BadValue: "app"}}, }, { "invalid termination message policy", + line(), []core.Container{ { Name: "init", Image: "image", ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "INVALID_POLICY_NAME", + TerminationMessagePolicy: "Unknown", }, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].terminationMessagePolicy"}, + field.ErrorList{{Type: field.ErrorTypeNotSupported, Field: "initContainers[0].terminationMessagePolicy", BadValue: core.TerminationMessagePolicy("Unknown")}}, }, { "duplicate names", + line(), []core.Container{ { Name: "init", @@ -7814,10 +8031,11 @@ func TestValidateInitContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name"}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name", BadValue: "init"}}, }, { "duplicate ports", + line(), []core.Container{ { Name: "abc", @@ -7834,10 +8052,11 @@ func TestValidateInitContainers(t *testing.T) { TerminationMessagePolicy: "File", }, }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[0].ports[1].hostPort"}, + field.ErrorList{{Type: field.ErrorTypeDuplicate, Field: "initContainers[0].ports[1].hostPort", BadValue: "TCP//8080"}}, }, { "uses disallowed field: Lifecycle", + line(), []core.Container{ { Name: "debug", @@ -7851,10 +8070,11 @@ func TestValidateInitContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].lifecycle"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "initContainers[0].lifecycle", BadValue: ""}}, }, { "uses disallowed field: LivenessProbe", + line(), []core.Container{ { Name: "debug", @@ -7869,10 +8089,11 @@ func TestValidateInitContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].livenessProbe"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "initContainers[0].livenessProbe", BadValue: ""}}, }, { "uses disallowed field: ReadinessProbe", + line(), []core.Container{ { Name: "debug", @@ -7886,10 +8107,11 @@ func TestValidateInitContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].readinessProbe"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "initContainers[0].readinessProbe", BadValue: ""}}, }, { "Container uses disallowed field: StartupProbe", + line(), []core.Container{ { Name: "debug", @@ -7904,24 +8126,43 @@ func TestValidateInitContainers(t *testing.T) { }, }, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].startupProbe"}, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "initContainers[0].startupProbe", BadValue: ""}}, + }, + { + "Disallowed field with other errors should only return a single Forbidden", + line(), + []core.Container{ + { + Name: "debug", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + StartupProbe: &core.Probe{ + ProbeHandler: core.ProbeHandler{ + TCPSocket: &core.TCPSocketAction{Port: intstr.FromInt(80)}, + }, + InitialDelaySeconds: -1, + TimeoutSeconds: -1, + PeriodSeconds: -1, + SuccessThreshold: -1, + FailureThreshold: -1, + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1), + }, + }, + }, + field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "initContainers[0].startupProbe", BadValue: ""}}, }, } for _, tc := range errorCases { - t.Run(tc.title, func(t *testing.T) { + t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { errs := validateInitContainers(tc.initContainers, containers, volumeDevices, field.NewPath("initContainers"), PodValidationOptions{}) if len(errs) == 0 { t.Fatal("expected error but received none") } - if len(errs) > 1 { - t.Errorf("expected 1 error but received %d: %q", len(errs), errs) - } - if errs[0].Type != tc.expectedError.Type { - t.Errorf("expected error type %q but received %q: %q", string(tc.expectedError.Type), string(errs[0].Type), errs) - } - if errs[0].Field != tc.expectedError.Field { - t.Errorf("expected error for field %q but received error for field %q: %q", tc.expectedError.Field, errs[0].Field, errs) + if diff := cmp.Diff(tc.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "Detail")); diff != "" { + t.Errorf("unexpected diff in errors (-want, +got):\n%s", diff) + t.Errorf("INFO: all errors:\n%s", prettyErrorList(errs)) } }) }