Skip to content

Commit f372c54

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#90628 from liggitt/pod-ip-status
Fix podIP validation
2 parents a034497 + 23e9fb1 commit f372c54

File tree

7 files changed

+115
-33
lines changed

7 files changed

+115
-33
lines changed

‎pkg/apis/core/validation/validation.go‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3113,8 +3113,9 @@ func ValidatePodSingleHugePageResources(pod *core.Pod, specPath *field.Path) fie
31133113
return allErrs
31143114
}
31153115

3116-
// ValidatePod tests if required fields in the pod are set.
3117-
func ValidatePod(pod *core.Pod, opts PodValidationOptions) field.ErrorList {
3116+
// validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set,
3117+
// and is called by ValidatePodCreate and ValidatePodUpdate.
3118+
func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field.ErrorList {
31183119
fldPath := field.NewPath("metadata")
31193120
allErrs := ValidateObjectMeta(&pod.ObjectMeta, true, ValidatePodName, fldPath)
31203121
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 {
31453146
allErrs = append(allErrs, ValidatePodSingleHugePageResources(pod, specPath)...)
31463147
}
31473148

3149+
return allErrs
3150+
}
3151+
3152+
// validatePodIPs validates IPs in pod status
3153+
func validatePodIPs(pod *core.Pod) field.ErrorList {
3154+
allErrs := field.ErrorList{}
3155+
31483156
podIPsField := field.NewPath("status", "podIPs")
31493157

31503158
// all PodIPs must be valid IPs
@@ -3705,7 +3713,7 @@ func ValidateContainerUpdates(newContainers, oldContainers []core.Container, fld
37053713

37063714
// ValidatePodCreate validates a pod in the context of its initial create
37073715
func ValidatePodCreate(pod *core.Pod, opts PodValidationOptions) field.ErrorList {
3708-
allErrs := ValidatePod(pod, opts)
3716+
allErrs := validatePodMetadataAndSpec(pod, opts)
37093717

37103718
fldPath := field.NewPath("spec")
37113719
// EphemeralContainers can only be set on update using the ephemeralcontainers subresource
@@ -3721,6 +3729,7 @@ func ValidatePodCreate(pod *core.Pod, opts PodValidationOptions) field.ErrorList
37213729
func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) field.ErrorList {
37223730
fldPath := field.NewPath("metadata")
37233731
allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath)
3732+
allErrs = append(allErrs, validatePodMetadataAndSpec(newPod, opts)...)
37243733
allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"))...)
37253734
specPath := field.NewPath("spec")
37263735

@@ -3850,6 +3859,14 @@ func ValidatePodStatusUpdate(newPod, oldPod *core.Pod) field.ErrorList {
38503859
allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.ContainerStatuses, oldPod.Status.ContainerStatuses, fldPath.Child("containerStatuses"), oldPod.Spec.RestartPolicy)...)
38513860
allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.InitContainerStatuses, oldPod.Status.InitContainerStatuses, fldPath.Child("initContainerStatuses"), oldPod.Spec.RestartPolicy)...)
38523861

3862+
if newIPErrs := validatePodIPs(newPod); len(newIPErrs) > 0 {
3863+
// Tolerate IP errors if IP errors already existed in the old pod. See http://issue.k8s.io/90625
3864+
// TODO(liggitt): Drop the check of oldPod in 1.20
3865+
if oldIPErrs := validatePodIPs(oldPod); len(oldIPErrs) == 0 {
3866+
allErrs = append(allErrs, newIPErrs...)
3867+
}
3868+
}
3869+
38533870
// For status update we ignore changes to pod spec.
38543871
newPod.Spec = oldPod.Spec
38553872

‎pkg/apis/core/validation/validation_test.go‎

Lines changed: 88 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4041,7 +4041,7 @@ func TestHugePagesIsolation(t *testing.T) {
40414041
for tcName, tc := range testCases {
40424042
t.Run(tcName, func(t *testing.T) {
40434043
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HugePageStorageMediumSize, tc.enableHugePageStorageMediumSize)()
4044-
errs := ValidatePod(tc.pod, PodValidationOptions{tc.enableHugePageStorageMediumSize})
4044+
errs := ValidatePodCreate(tc.pod, PodValidationOptions{tc.enableHugePageStorageMediumSize})
40454045
if tc.expectError && len(errs) == 0 {
40464046
t.Errorf("Unexpected success")
40474047
}
@@ -7426,7 +7426,7 @@ func TestValidatePod(t *testing.T) {
74267426
},
74277427
}
74287428
for _, pod := range successCases {
7429-
if errs := ValidatePod(&pod, PodValidationOptions{}); len(errs) != 0 {
7429+
if errs := ValidatePodCreate(&pod, PodValidationOptions{}); len(errs) != 0 {
74307430
t.Errorf("expected success: %v", errs)
74317431
}
74327432
}
@@ -8276,7 +8276,7 @@ func TestValidatePod(t *testing.T) {
82768276
},
82778277
}
82788278
for k, v := range errorCases {
8279-
if errs := ValidatePod(&v.spec, PodValidationOptions{}); len(errs) == 0 {
8279+
if errs := ValidatePodCreate(&v.spec, PodValidationOptions{}); len(errs) == 0 {
82808280
t.Errorf("expected failure for %q", k)
82818281
} else if v.expectedError == "" {
82828282
t.Errorf("missing expectedError for %q, got %q", k, errs.ToAggregate().Error())
@@ -8485,7 +8485,10 @@ func TestValidatePodUpdate(t *testing.T) {
84858485
Spec: core.PodSpec{
84868486
Containers: []core.Container{
84878487
{
8488-
Image: "foo:V1",
8488+
Name: "container",
8489+
Image: "foo:V1",
8490+
TerminationMessagePolicy: "File",
8491+
ImagePullPolicy: "Always",
84898492
},
84908493
},
84918494
},
@@ -8495,7 +8498,10 @@ func TestValidatePodUpdate(t *testing.T) {
84958498
Spec: core.PodSpec{
84968499
Containers: []core.Container{
84978500
{
8498-
Image: "foo:V2",
8501+
Name: "container",
8502+
Image: "foo:V2",
8503+
TerminationMessagePolicy: "File",
8504+
ImagePullPolicy: "Always",
84998505
},
85008506
},
85018507
},
@@ -8509,7 +8515,10 @@ func TestValidatePodUpdate(t *testing.T) {
85098515
Spec: core.PodSpec{
85108516
InitContainers: []core.Container{
85118517
{
8512-
Image: "foo:V1",
8518+
Name: "container",
8519+
Image: "foo:V1",
8520+
TerminationMessagePolicy: "File",
8521+
ImagePullPolicy: "Always",
85138522
},
85148523
},
85158524
},
@@ -8519,7 +8528,10 @@ func TestValidatePodUpdate(t *testing.T) {
85198528
Spec: core.PodSpec{
85208529
InitContainers: []core.Container{
85218530
{
8522-
Image: "foo:V2",
8531+
Name: "container",
8532+
Image: "foo:V2",
8533+
TerminationMessagePolicy: "File",
8534+
ImagePullPolicy: "Always",
85238535
},
85248536
},
85258537
},
@@ -8532,7 +8544,11 @@ func TestValidatePodUpdate(t *testing.T) {
85328544
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
85338545
Spec: core.PodSpec{
85348546
Containers: []core.Container{
8535-
{},
8547+
{
8548+
Name: "container",
8549+
TerminationMessagePolicy: "File",
8550+
ImagePullPolicy: "Always",
8551+
},
85368552
},
85378553
},
85388554
},
@@ -8541,7 +8557,10 @@ func TestValidatePodUpdate(t *testing.T) {
85418557
Spec: core.PodSpec{
85428558
Containers: []core.Container{
85438559
{
8544-
Image: "foo:V2",
8560+
Name: "container",
8561+
Image: "foo:V2",
8562+
TerminationMessagePolicy: "File",
8563+
ImagePullPolicy: "Always",
85458564
},
85468565
},
85478566
},
@@ -8554,7 +8573,11 @@ func TestValidatePodUpdate(t *testing.T) {
85548573
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
85558574
Spec: core.PodSpec{
85568575
InitContainers: []core.Container{
8557-
{},
8576+
{
8577+
Name: "container",
8578+
TerminationMessagePolicy: "File",
8579+
ImagePullPolicy: "Always",
8580+
},
85588581
},
85598582
},
85608583
},
@@ -8563,7 +8586,10 @@ func TestValidatePodUpdate(t *testing.T) {
85638586
Spec: core.PodSpec{
85648587
InitContainers: []core.Container{
85658588
{
8566-
Image: "foo:V2",
8589+
Name: "container",
8590+
Image: "foo:V2",
8591+
TerminationMessagePolicy: "File",
8592+
ImagePullPolicy: "Always",
85678593
},
85688594
},
85698595
},
@@ -8690,7 +8716,7 @@ func TestValidatePodUpdate(t *testing.T) {
86908716
ActiveDeadlineSeconds: &activeDeadlineSecondsPositive,
86918717
},
86928718
},
8693-
"",
8719+
"spec.activeDeadlineSeconds",
86948720
"activeDeadlineSeconds change to zero from positive",
86958721
},
86968722
{
@@ -8700,7 +8726,7 @@ func TestValidatePodUpdate(t *testing.T) {
87008726
},
87018727
},
87028728
core.Pod{},
8703-
"",
8729+
"spec.activeDeadlineSeconds",
87048730
"activeDeadlineSeconds change to zero from nil",
87058731
},
87068732
{
@@ -9047,6 +9073,29 @@ func TestValidatePodUpdate(t *testing.T) {
90479073
for _, test := range tests {
90489074
test.new.ObjectMeta.ResourceVersion = "1"
90499075
test.old.ObjectMeta.ResourceVersion = "1"
9076+
9077+
// set required fields if old and new match and have no opinion on the value
9078+
if test.new.Name == "" && test.old.Name == "" {
9079+
test.new.Name = "name"
9080+
test.old.Name = "name"
9081+
}
9082+
if test.new.Namespace == "" && test.old.Namespace == "" {
9083+
test.new.Namespace = "namespace"
9084+
test.old.Namespace = "namespace"
9085+
}
9086+
if test.new.Spec.Containers == nil && test.old.Spec.Containers == nil {
9087+
test.new.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}}
9088+
test.old.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}}
9089+
}
9090+
if len(test.new.Spec.DNSPolicy) == 0 && len(test.old.Spec.DNSPolicy) == 0 {
9091+
test.new.Spec.DNSPolicy = core.DNSClusterFirst
9092+
test.old.Spec.DNSPolicy = core.DNSClusterFirst
9093+
}
9094+
if len(test.new.Spec.RestartPolicy) == 0 && len(test.old.Spec.RestartPolicy) == 0 {
9095+
test.new.Spec.RestartPolicy = "Always"
9096+
test.old.Spec.RestartPolicy = "Always"
9097+
}
9098+
90509099
errs := ValidatePodUpdate(&test.new, &test.old, PodValidationOptions{})
90519100
if test.err == "" {
90529101
if len(errs) != 0 {
@@ -15031,15 +15080,32 @@ func TestPodIPsValidation(t *testing.T) {
1503115080
}
1503215081

1503315082
for _, testCase := range testCases {
15034-
errs := ValidatePod(&testCase.pod, PodValidationOptions{})
15035-
if len(errs) == 0 && testCase.expectError {
15036-
t.Errorf("expected failure for %s, but there were none", testCase.pod.Name)
15037-
return
15038-
}
15039-
if len(errs) != 0 && !testCase.expectError {
15040-
t.Errorf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs)
15041-
return
15042-
}
15083+
t.Run(testCase.pod.Name, func(t *testing.T) {
15084+
for _, oldTestCase := range testCases {
15085+
newPod := testCase.pod.DeepCopy()
15086+
newPod.ResourceVersion = "1"
15087+
15088+
oldPod := oldTestCase.pod.DeepCopy()
15089+
oldPod.ResourceVersion = "1"
15090+
oldPod.Name = newPod.Name
15091+
15092+
errs := ValidatePodStatusUpdate(newPod, oldPod)
15093+
if oldTestCase.expectError {
15094+
// The old pod was invalid, tolerate invalid IPs in the new pod as well
15095+
if len(errs) > 0 {
15096+
t.Fatalf("expected success for update to pod with already-invalid IPs, got errors: %v", errs)
15097+
}
15098+
continue
15099+
}
15100+
15101+
if len(errs) == 0 && testCase.expectError {
15102+
t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name)
15103+
}
15104+
if len(errs) != 0 && !testCase.expectError {
15105+
t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs)
15106+
}
15107+
}
15108+
})
1504315109
}
1504415110
}
1504515111

‎pkg/kubelet/config/common.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func tryDecodeSinglePod(data []byte, defaultFn defaultFunc) (parsed bool, pod *v
138138
opts := validation.PodValidationOptions{
139139
AllowMultipleHugePageResources: utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize),
140140
}
141-
if errs := validation.ValidatePod(newPod, opts); len(errs) > 0 {
141+
if errs := validation.ValidatePodCreate(newPod, opts); len(errs) > 0 {
142142
return true, pod, fmt.Errorf("invalid pod: %v", errs)
143143
}
144144
v1Pod := &v1.Pod{}
@@ -172,7 +172,7 @@ func tryDecodePodList(data []byte, defaultFn defaultFunc) (parsed bool, pods v1.
172172
if err = defaultFn(newPod); err != nil {
173173
return true, pods, err
174174
}
175-
if errs := validation.ValidatePod(newPod, opts); len(errs) > 0 {
175+
if errs := validation.ValidatePodCreate(newPod, opts); len(errs) > 0 {
176176
err = fmt.Errorf("invalid pod: %v", errs)
177177
return true, pods, err
178178
}

‎pkg/kubelet/config/common_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ func TestStaticPodNameGenerate(t *testing.T) {
251251
if c.overwrite != "" {
252252
pod.Name = c.overwrite
253253
}
254-
errs := validation.ValidatePod(pod, validation.PodValidationOptions{})
254+
errs := validation.ValidatePodCreate(pod, validation.PodValidationOptions{})
255255
if c.shouldErr {
256256
specNameErrored := false
257257
for _, err := range errs {

‎pkg/kubelet/config/file_linux_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func TestReadPodsFromFileExistAlready(t *testing.T) {
9090
if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil {
9191
t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err)
9292
}
93-
if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) > 0 {
93+
if errs := validation.ValidatePodCreate(internalPod, validation.PodValidationOptions{}); len(errs) > 0 {
9494
t.Fatalf("%s: Invalid pod %#v, %#v", testCase.desc, internalPod, errs)
9595
}
9696
}
@@ -369,7 +369,7 @@ func expectUpdate(t *testing.T, ch chan interface{}, testCase *testCase) {
369369
if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil {
370370
t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err)
371371
}
372-
if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) > 0 {
372+
if errs := validation.ValidatePodCreate(internalPod, validation.PodValidationOptions{}); len(errs) > 0 {
373373
t.Fatalf("%s: Invalid pod %#v, %#v", testCase.desc, internalPod, errs)
374374
}
375375
}

‎pkg/kubelet/config/http_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ func TestExtractPodsFromHTTP(t *testing.T) {
319319
if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil {
320320
t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err)
321321
}
322-
if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) != 0 {
322+
if errs := validation.ValidatePodCreate(internalPod, validation.PodValidationOptions{}); len(errs) != 0 {
323323
t.Errorf("%s: Expected no validation errors on %#v, Got %v", testCase.desc, pod, errs.ToAggregate())
324324
}
325325
}

‎pkg/registry/core/pod/strategy.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,7 @@ func (podStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object)
113113
// Allow multiple huge pages on pod create if feature is enabled or if the old pod already has multiple hugepages specified
114114
AllowMultipleHugePageResources: oldFailsSingleHugepagesValidation || utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize),
115115
}
116-
errorList := validation.ValidatePod(obj.(*api.Pod), opts)
117-
errorList = append(errorList, validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod), opts)...)
116+
errorList := validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod), opts)
118117
errorList = append(errorList, validation.ValidateConditionalPod(obj.(*api.Pod), old.(*api.Pod), field.NewPath(""))...)
119118
return errorList
120119
}

0 commit comments

Comments
 (0)