From 23e9fb1bb552bd0098770ad26bb3d5f60f8f2c06 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 29 Apr 2020 22:25:03 -0400 Subject: [PATCH] Fix podIP validation --- pkg/apis/core/validation/validation.go | 23 +++- pkg/apis/core/validation/validation_test.go | 110 ++++++++++++++++---- pkg/kubelet/config/common.go | 4 +- pkg/kubelet/config/common_test.go | 2 +- pkg/kubelet/config/file_linux_test.go | 4 +- pkg/kubelet/config/http_test.go | 2 +- pkg/registry/core/pod/strategy.go | 3 +- 7 files changed, 115 insertions(+), 33 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 41909b4d0b5..109a2220a2f 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3113,8 +3113,9 @@ func ValidatePodSingleHugePageResources(pod *core.Pod, specPath *field.Path) fie return allErrs } -// ValidatePod tests if required fields in the pod are set. -func ValidatePod(pod *core.Pod, opts PodValidationOptions) field.ErrorList { +// validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set, +// and is called by ValidatePodCreate and ValidatePodUpdate. +func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field.ErrorList { fldPath := field.NewPath("metadata") allErrs := ValidateObjectMeta(&pod.ObjectMeta, true, ValidatePodName, fldPath) allErrs = append(allErrs, ValidatePodSpecificAnnotations(pod.ObjectMeta.Annotations, &pod.Spec, fldPath.Child("annotations"))...) @@ -3145,6 +3146,13 @@ func ValidatePod(pod *core.Pod, opts PodValidationOptions) field.ErrorList { allErrs = append(allErrs, ValidatePodSingleHugePageResources(pod, specPath)...) } + return allErrs +} + +// validatePodIPs validates IPs in pod status +func validatePodIPs(pod *core.Pod) field.ErrorList { + allErrs := field.ErrorList{} + podIPsField := field.NewPath("status", "podIPs") // all PodIPs must be valid IPs @@ -3705,7 +3713,7 @@ func ValidateContainerUpdates(newContainers, oldContainers []core.Container, fld // ValidatePodCreate validates a pod in the context of its initial create func ValidatePodCreate(pod *core.Pod, opts PodValidationOptions) field.ErrorList { - allErrs := ValidatePod(pod, opts) + allErrs := validatePodMetadataAndSpec(pod, opts) fldPath := field.NewPath("spec") // EphemeralContainers can only be set on update using the ephemeralcontainers subresource @@ -3721,6 +3729,7 @@ func ValidatePodCreate(pod *core.Pod, opts PodValidationOptions) field.ErrorList func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) field.ErrorList { fldPath := field.NewPath("metadata") allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath) + allErrs = append(allErrs, validatePodMetadataAndSpec(newPod, opts)...) allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"))...) specPath := field.NewPath("spec") @@ -3850,6 +3859,14 @@ func ValidatePodStatusUpdate(newPod, oldPod *core.Pod) field.ErrorList { allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.ContainerStatuses, oldPod.Status.ContainerStatuses, fldPath.Child("containerStatuses"), oldPod.Spec.RestartPolicy)...) allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.InitContainerStatuses, oldPod.Status.InitContainerStatuses, fldPath.Child("initContainerStatuses"), oldPod.Spec.RestartPolicy)...) + if newIPErrs := validatePodIPs(newPod); len(newIPErrs) > 0 { + // Tolerate IP errors if IP errors already existed in the old pod. See http://issue.k8s.io/90625 + // TODO(liggitt): Drop the check of oldPod in 1.20 + if oldIPErrs := validatePodIPs(oldPod); len(oldIPErrs) == 0 { + allErrs = append(allErrs, newIPErrs...) + } + } + // For status update we ignore changes to pod spec. newPod.Spec = oldPod.Spec diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 731a0d27f12..ce4ffa345a7 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -4041,7 +4041,7 @@ func TestHugePagesIsolation(t *testing.T) { for tcName, tc := range testCases { t.Run(tcName, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HugePageStorageMediumSize, tc.enableHugePageStorageMediumSize)() - errs := ValidatePod(tc.pod, PodValidationOptions{tc.enableHugePageStorageMediumSize}) + errs := ValidatePodCreate(tc.pod, PodValidationOptions{tc.enableHugePageStorageMediumSize}) if tc.expectError && len(errs) == 0 { t.Errorf("Unexpected success") } @@ -7426,7 +7426,7 @@ func TestValidatePod(t *testing.T) { }, } for _, pod := range successCases { - if errs := ValidatePod(&pod, PodValidationOptions{}); len(errs) != 0 { + if errs := ValidatePodCreate(&pod, PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) } } @@ -8276,7 +8276,7 @@ func TestValidatePod(t *testing.T) { }, } for k, v := range errorCases { - if errs := ValidatePod(&v.spec, PodValidationOptions{}); len(errs) == 0 { + if errs := ValidatePodCreate(&v.spec, PodValidationOptions{}); len(errs) == 0 { t.Errorf("expected failure for %q", k) } else if v.expectedError == "" { t.Errorf("missing expectedError for %q, got %q", k, errs.ToAggregate().Error()) @@ -8485,7 +8485,10 @@ func TestValidatePodUpdate(t *testing.T) { Spec: core.PodSpec{ Containers: []core.Container{ { - Image: "foo:V1", + Name: "container", + Image: "foo:V1", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", }, }, }, @@ -8495,7 +8498,10 @@ func TestValidatePodUpdate(t *testing.T) { Spec: core.PodSpec{ Containers: []core.Container{ { - Image: "foo:V2", + Name: "container", + Image: "foo:V2", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", }, }, }, @@ -8509,7 +8515,10 @@ func TestValidatePodUpdate(t *testing.T) { Spec: core.PodSpec{ InitContainers: []core.Container{ { - Image: "foo:V1", + Name: "container", + Image: "foo:V1", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", }, }, }, @@ -8519,7 +8528,10 @@ func TestValidatePodUpdate(t *testing.T) { Spec: core.PodSpec{ InitContainers: []core.Container{ { - Image: "foo:V2", + Name: "container", + Image: "foo:V2", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", }, }, }, @@ -8532,7 +8544,11 @@ func TestValidatePodUpdate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: core.PodSpec{ Containers: []core.Container{ - {}, + { + Name: "container", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", + }, }, }, }, @@ -8541,7 +8557,10 @@ func TestValidatePodUpdate(t *testing.T) { Spec: core.PodSpec{ Containers: []core.Container{ { - Image: "foo:V2", + Name: "container", + Image: "foo:V2", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", }, }, }, @@ -8554,7 +8573,11 @@ func TestValidatePodUpdate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: core.PodSpec{ InitContainers: []core.Container{ - {}, + { + Name: "container", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", + }, }, }, }, @@ -8563,7 +8586,10 @@ func TestValidatePodUpdate(t *testing.T) { Spec: core.PodSpec{ InitContainers: []core.Container{ { - Image: "foo:V2", + Name: "container", + Image: "foo:V2", + TerminationMessagePolicy: "File", + ImagePullPolicy: "Always", }, }, }, @@ -8690,7 +8716,7 @@ func TestValidatePodUpdate(t *testing.T) { ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, }, }, - "", + "spec.activeDeadlineSeconds", "activeDeadlineSeconds change to zero from positive", }, { @@ -8700,7 +8726,7 @@ func TestValidatePodUpdate(t *testing.T) { }, }, core.Pod{}, - "", + "spec.activeDeadlineSeconds", "activeDeadlineSeconds change to zero from nil", }, { @@ -9047,6 +9073,29 @@ func TestValidatePodUpdate(t *testing.T) { for _, test := range tests { test.new.ObjectMeta.ResourceVersion = "1" test.old.ObjectMeta.ResourceVersion = "1" + + // set required fields if old and new match and have no opinion on the value + if test.new.Name == "" && test.old.Name == "" { + test.new.Name = "name" + test.old.Name = "name" + } + if test.new.Namespace == "" && test.old.Namespace == "" { + test.new.Namespace = "namespace" + test.old.Namespace = "namespace" + } + if test.new.Spec.Containers == nil && test.old.Spec.Containers == nil { + test.new.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}} + test.old.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}} + } + if len(test.new.Spec.DNSPolicy) == 0 && len(test.old.Spec.DNSPolicy) == 0 { + test.new.Spec.DNSPolicy = core.DNSClusterFirst + test.old.Spec.DNSPolicy = core.DNSClusterFirst + } + if len(test.new.Spec.RestartPolicy) == 0 && len(test.old.Spec.RestartPolicy) == 0 { + test.new.Spec.RestartPolicy = "Always" + test.old.Spec.RestartPolicy = "Always" + } + errs := ValidatePodUpdate(&test.new, &test.old, PodValidationOptions{}) if test.err == "" { if len(errs) != 0 { @@ -15031,15 +15080,32 @@ func TestPodIPsValidation(t *testing.T) { } for _, testCase := range testCases { - errs := ValidatePod(&testCase.pod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { - t.Errorf("expected failure for %s, but there were none", testCase.pod.Name) - return - } - if len(errs) != 0 && !testCase.expectError { - t.Errorf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) - return - } + t.Run(testCase.pod.Name, func(t *testing.T) { + for _, oldTestCase := range testCases { + newPod := testCase.pod.DeepCopy() + newPod.ResourceVersion = "1" + + oldPod := oldTestCase.pod.DeepCopy() + oldPod.ResourceVersion = "1" + oldPod.Name = newPod.Name + + errs := ValidatePodStatusUpdate(newPod, oldPod) + if oldTestCase.expectError { + // The old pod was invalid, tolerate invalid IPs in the new pod as well + if len(errs) > 0 { + t.Fatalf("expected success for update to pod with already-invalid IPs, got errors: %v", errs) + } + continue + } + + if len(errs) == 0 && testCase.expectError { + t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + } + if len(errs) != 0 && !testCase.expectError { + t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + } + } + }) } } diff --git a/pkg/kubelet/config/common.go b/pkg/kubelet/config/common.go index 6ddf219b4f1..5e71f977ddf 100644 --- a/pkg/kubelet/config/common.go +++ b/pkg/kubelet/config/common.go @@ -138,7 +138,7 @@ func tryDecodeSinglePod(data []byte, defaultFn defaultFunc) (parsed bool, pod *v opts := validation.PodValidationOptions{ AllowMultipleHugePageResources: utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize), } - if errs := validation.ValidatePod(newPod, opts); len(errs) > 0 { + if errs := validation.ValidatePodCreate(newPod, opts); len(errs) > 0 { return true, pod, fmt.Errorf("invalid pod: %v", errs) } v1Pod := &v1.Pod{} @@ -172,7 +172,7 @@ func tryDecodePodList(data []byte, defaultFn defaultFunc) (parsed bool, pods v1. if err = defaultFn(newPod); err != nil { return true, pods, err } - if errs := validation.ValidatePod(newPod, opts); len(errs) > 0 { + if errs := validation.ValidatePodCreate(newPod, opts); len(errs) > 0 { err = fmt.Errorf("invalid pod: %v", errs) return true, pods, err } diff --git a/pkg/kubelet/config/common_test.go b/pkg/kubelet/config/common_test.go index b29b83d080f..a3bf689e786 100644 --- a/pkg/kubelet/config/common_test.go +++ b/pkg/kubelet/config/common_test.go @@ -251,7 +251,7 @@ func TestStaticPodNameGenerate(t *testing.T) { if c.overwrite != "" { pod.Name = c.overwrite } - errs := validation.ValidatePod(pod, validation.PodValidationOptions{}) + errs := validation.ValidatePodCreate(pod, validation.PodValidationOptions{}) if c.shouldErr { specNameErrored := false for _, err := range errs { diff --git a/pkg/kubelet/config/file_linux_test.go b/pkg/kubelet/config/file_linux_test.go index 4c7a7ba8f34..e23988c46b9 100644 --- a/pkg/kubelet/config/file_linux_test.go +++ b/pkg/kubelet/config/file_linux_test.go @@ -90,7 +90,7 @@ func TestReadPodsFromFileExistAlready(t *testing.T) { if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil { t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err) } - if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) > 0 { + if errs := validation.ValidatePodCreate(internalPod, validation.PodValidationOptions{}); len(errs) > 0 { t.Fatalf("%s: Invalid pod %#v, %#v", testCase.desc, internalPod, errs) } } @@ -369,7 +369,7 @@ func expectUpdate(t *testing.T, ch chan interface{}, testCase *testCase) { if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil { t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err) } - if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) > 0 { + if errs := validation.ValidatePodCreate(internalPod, validation.PodValidationOptions{}); len(errs) > 0 { t.Fatalf("%s: Invalid pod %#v, %#v", testCase.desc, internalPod, errs) } } diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go index 2778bfc535b..d6c3eec86a8 100644 --- a/pkg/kubelet/config/http_test.go +++ b/pkg/kubelet/config/http_test.go @@ -319,7 +319,7 @@ func TestExtractPodsFromHTTP(t *testing.T) { if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil { t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err) } - if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) != 0 { + if errs := validation.ValidatePodCreate(internalPod, validation.PodValidationOptions{}); len(errs) != 0 { t.Errorf("%s: Expected no validation errors on %#v, Got %v", testCase.desc, pod, errs.ToAggregate()) } } diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 22fa5a1d134..b7c144a6fbf 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -113,8 +113,7 @@ func (podStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) // Allow multiple huge pages on pod create if feature is enabled or if the old pod already has multiple hugepages specified AllowMultipleHugePageResources: oldFailsSingleHugepagesValidation || utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize), } - errorList := validation.ValidatePod(obj.(*api.Pod), opts) - errorList = append(errorList, validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod), opts)...) + errorList := validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod), opts) errorList = append(errorList, validation.ValidateConditionalPod(obj.(*api.Pod), old.(*api.Pod), field.NewPath(""))...) return errorList }