From c3384191ea6410121339a48d1c68ba6f4b3ab31a Mon Sep 17 00:00:00 2001 From: mfordjody <11638005@qq.com> Date: Sun, 23 Apr 2023 01:37:47 +0800 Subject: [PATCH] remove validation GCE-ism update testing update testing update testing update core and testing update testing --- pkg/apis/apps/validation/validation.go | 50 ++- pkg/apis/apps/validation/validation_test.go | 374 +++++++++++++++++--- pkg/apis/core/validation/validation.go | 52 ++- pkg/apis/core/validation/validation_test.go | 105 ++++++ pkg/features/kube_features.go | 8 + 5 files changed, 511 insertions(+), 78 deletions(-) diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index 14c287c3963..8c10a956f13 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/pkg/apis/apps/validation/validation.go @@ -280,7 +280,7 @@ func ValidateControllerRevisionUpdate(newHistory, oldHistory *apps.ControllerRev // ValidateDaemonSet tests if required fields in the DaemonSet are set. func ValidateDaemonSet(ds *apps.DaemonSet, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&ds.ObjectMeta, true, ValidateDaemonSetName, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateDaemonSetSpec(&ds.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateDaemonSetSpec(&ds.Spec, nil, field.NewPath("spec"), opts)...) return allErrs } @@ -288,7 +288,7 @@ func ValidateDaemonSet(ds *apps.DaemonSet, opts apivalidation.PodValidationOptio func ValidateDaemonSetUpdate(ds, oldDS *apps.DaemonSet, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMetaUpdate(&ds.ObjectMeta, &oldDS.ObjectMeta, field.NewPath("metadata")) allErrs = append(allErrs, ValidateDaemonSetSpecUpdate(&ds.Spec, &oldDS.Spec, field.NewPath("spec"))...) - allErrs = append(allErrs, ValidateDaemonSetSpec(&ds.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateDaemonSetSpec(&ds.Spec, &oldDS.Spec, field.NewPath("spec"), opts)...) return allErrs } @@ -344,7 +344,7 @@ func ValidateDaemonSetStatusUpdate(ds, oldDS *apps.DaemonSet) field.ErrorList { } // ValidateDaemonSetSpec tests if required fields in the DaemonSetSpec are set. -func ValidateDaemonSetSpec(spec *apps.DaemonSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { +func ValidateDaemonSetSpec(spec, oldSpec *apps.DaemonSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector} @@ -359,8 +359,12 @@ func ValidateDaemonSetSpec(spec *apps.DaemonSetSpec, fldPath *field.Path, opts a } allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(&spec.Template, fldPath.Child("template"), opts)...) - // Daemons typically run on more than one node, so mark Read-Write persistent disks as invalid. - allErrs = append(allErrs, apivalidation.ValidateReadOnlyPersistentDisks(spec.Template.Spec.Volumes, fldPath.Child("template", "spec", "volumes"))...) + // get rid of apivalidation.ValidateReadOnlyPersistentDisks,stop passing oldSpec to this function + var oldVols []api.Volume + if oldSpec != nil { + oldVols = oldSpec.Template.Spec.Volumes // +k8s:verify-mutation:reason=clone + } + allErrs = append(allErrs, apivalidation.ValidateReadOnlyPersistentDisks(spec.Template.Spec.Volumes, oldVols, fldPath.Child("template", "spec", "volumes"))...) // RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec(). if spec.Template.Spec.RestartPolicy != api.RestartPolicyAlways { allErrs = append(allErrs, field.NotSupported(fldPath.Child("template", "spec", "restartPolicy"), spec.Template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)})) @@ -544,7 +548,7 @@ func ValidateRollback(rollback *apps.RollbackConfig, fldPath *field.Path) field. } // ValidateDeploymentSpec validates given deployment spec. -func ValidateDeploymentSpec(spec *apps.DeploymentSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { +func ValidateDeploymentSpec(spec, oldSpec *apps.DeploymentSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...) @@ -562,7 +566,12 @@ func ValidateDeploymentSpec(spec *apps.DeploymentSpec, fldPath *field.Path, opts if err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "invalid label selector")) } else { - allErrs = append(allErrs, ValidatePodTemplateSpecForReplicaSet(&spec.Template, selector, spec.Replicas, fldPath.Child("template"), opts)...) + // oldSpec is not empty, pass oldSpec.template + var oldTemplate *api.PodTemplateSpec + if oldSpec != nil { + oldTemplate = &oldSpec.Template // +k8s:verify-mutation:reason=clone + } + allErrs = append(allErrs, ValidatePodTemplateSpecForReplicaSet(&spec.Template, oldTemplate, selector, spec.Replicas, fldPath.Child("template"), opts)...) } allErrs = append(allErrs, ValidateDeploymentStrategy(&spec.Strategy, fldPath.Child("strategy"))...) @@ -614,7 +623,7 @@ func ValidateDeploymentStatus(status *apps.DeploymentStatus, fldPath *field.Path // ValidateDeploymentUpdate tests if an update to a Deployment is valid. func ValidateDeploymentUpdate(update, old *apps.Deployment, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateDeploymentSpec(&update.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateDeploymentSpec(&update.Spec, &old.Spec, field.NewPath("spec"), opts)...) return allErrs } @@ -637,7 +646,7 @@ func ValidateDeploymentStatusUpdate(update, old *apps.Deployment) field.ErrorLis // ValidateDeployment validates a given Deployment. func ValidateDeployment(obj *apps.Deployment, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&obj.ObjectMeta, true, ValidateDeploymentName, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateDeploymentSpec(&obj.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateDeploymentSpec(&obj.Spec, nil, field.NewPath("spec"), opts)...) return allErrs } @@ -660,7 +669,7 @@ var ValidateReplicaSetName = apimachineryvalidation.NameIsDNSSubdomain // ValidateReplicaSet tests if required fields in the ReplicaSet are set. func ValidateReplicaSet(rs *apps.ReplicaSet, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&rs.ObjectMeta, true, ValidateReplicaSetName, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateReplicaSetSpec(&rs.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateReplicaSetSpec(&rs.Spec, nil, field.NewPath("spec"), opts)...) return allErrs } @@ -668,7 +677,7 @@ func ValidateReplicaSet(rs *apps.ReplicaSet, opts apivalidation.PodValidationOpt func ValidateReplicaSetUpdate(rs, oldRs *apps.ReplicaSet, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&rs.ObjectMeta, &oldRs.ObjectMeta, field.NewPath("metadata"))...) - allErrs = append(allErrs, ValidateReplicaSetSpec(&rs.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateReplicaSetSpec(&rs.Spec, &oldRs.Spec, field.NewPath("spec"), opts)...) return allErrs } @@ -705,7 +714,7 @@ func ValidateReplicaSetStatus(status apps.ReplicaSetStatus, fldPath *field.Path) } // ValidateReplicaSetSpec tests if required fields in the ReplicaSet spec are set. -func ValidateReplicaSetSpec(spec *apps.ReplicaSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { +func ValidateReplicaSetSpec(spec, oldSpec *apps.ReplicaSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...) @@ -725,13 +734,18 @@ func ValidateReplicaSetSpec(spec *apps.ReplicaSetSpec, fldPath *field.Path, opts if err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "invalid label selector")) } else { - allErrs = append(allErrs, ValidatePodTemplateSpecForReplicaSet(&spec.Template, selector, spec.Replicas, fldPath.Child("template"), opts)...) + // oldSpec is not empty, pass oldSpec.template. + var oldTemplate *api.PodTemplateSpec + if oldSpec != nil { + oldTemplate = &oldSpec.Template // +k8s:verify-mutation:reason=clone + } + allErrs = append(allErrs, ValidatePodTemplateSpecForReplicaSet(&spec.Template, oldTemplate, selector, spec.Replicas, fldPath.Child("template"), opts)...) } return allErrs } // ValidatePodTemplateSpecForReplicaSet validates the given template and ensures that it is in accordance with the desired selector and replicas. -func ValidatePodTemplateSpecForReplicaSet(template *api.PodTemplateSpec, selector labels.Selector, replicas int32, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { +func ValidatePodTemplateSpecForReplicaSet(template, oldTemplate *api.PodTemplateSpec, selector labels.Selector, replicas int32, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if template == nil { allErrs = append(allErrs, field.Required(fldPath, "")) @@ -744,8 +758,14 @@ func ValidatePodTemplateSpecForReplicaSet(template *api.PodTemplateSpec, selecto } } allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(template, fldPath, opts)...) + // Daemons run on more than one node, Cancel verification of read and write volumes. + // get rid of apivalidation.ValidateReadOnlyPersistentDisks,stop passing oldTemplate to this function + var oldVols []api.Volume + if oldTemplate != nil { + oldVols = oldTemplate.Spec.Volumes // +k8s:verify-mutation:reason=clone + } if replicas > 1 { - allErrs = append(allErrs, apivalidation.ValidateReadOnlyPersistentDisks(template.Spec.Volumes, fldPath.Child("spec", "volumes"))...) + allErrs = append(allErrs, apivalidation.ValidateReadOnlyPersistentDisks(template.Spec.Volumes, oldVols, fldPath.Child("spec", "volumes"))...) } // RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec(). if template.Spec.RestartPolicy != api.RestartPolicyAlways { diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go index 0cbb1349686..f0069416981 100644 --- a/pkg/apis/apps/validation/validation_test.go +++ b/pkg/apis/apps/validation/validation_test.go @@ -1618,11 +1618,11 @@ func TestValidateDaemonSetUpdate(t *testing.T) { Spec: validPodSpecVolume, }, } - type dsUpdateTest struct { - old apps.DaemonSet - update apps.DaemonSet - expectedErrNum int + old apps.DaemonSet + update apps.DaemonSet + expectedErrNum int + enableSkipReadOnlyValidationGCE bool } successCases := map[string]dsUpdateTest{ "no change": { @@ -1775,21 +1775,49 @@ func TestValidateDaemonSetUpdate(t *testing.T) { }, }, }, + "Read-write volume verification": { + enableSkipReadOnlyValidationGCE: true, + old: apps.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DaemonSetSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validSelector}, + TemplateGeneration: 1, + Template: validPodTemplateAbc.Template, + UpdateStrategy: apps.DaemonSetUpdateStrategy{ + Type: apps.OnDeleteDaemonSetStrategyType, + }, + }, + }, + update: apps.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DaemonSetSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validSelector}, + TemplateGeneration: 2, + Template: readWriteVolumePodTemplate.Template, + UpdateStrategy: apps.DaemonSetUpdateStrategy{ + Type: apps.OnDeleteDaemonSetStrategyType, + }, + }, + }, + }, } for testName, successCase := range successCases { - // ResourceVersion is required for updates. - successCase.old.ObjectMeta.ResourceVersion = "1" - successCase.update.ObjectMeta.ResourceVersion = "2" - // Check test setup - if successCase.expectedErrNum > 0 { - t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected no error", testName, successCase.expectedErrNum) - } - if len(successCase.old.ObjectMeta.ResourceVersion) == 0 || len(successCase.update.ObjectMeta.ResourceVersion) == 0 { - t.Errorf("%q has incorrect test setup with no resource version set", testName) - } - if errs := ValidateDaemonSetUpdate(&successCase.update, &successCase.old, corevalidation.PodValidationOptions{}); len(errs) != 0 { - t.Errorf("%q expected no error, but got: %v", testName, errs) - } + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, successCase.enableSkipReadOnlyValidationGCE)() + // ResourceVersion is required for updates. + successCase.old.ObjectMeta.ResourceVersion = "1" + successCase.update.ObjectMeta.ResourceVersion = "2" + // Check test setup + if successCase.expectedErrNum > 0 { + t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected no error", testName, successCase.expectedErrNum) + } + if len(successCase.old.ObjectMeta.ResourceVersion) == 0 || len(successCase.update.ObjectMeta.ResourceVersion) == 0 { + t.Errorf("%q has incorrect test setup with no resource version set", testName) + } + if errs := ValidateDaemonSetUpdate(&successCase.update, &successCase.old, corevalidation.PodValidationOptions{}); len(errs) != 0 { + t.Errorf("%q expected no error, but got: %v", testName, errs) + } + }) } errorCases := map[string]dsUpdateTest{ "change daemon name": { @@ -1868,6 +1896,7 @@ func TestValidateDaemonSetUpdate(t *testing.T) { expectedErrNum: 1, }, "invalid read-write volume": { + enableSkipReadOnlyValidationGCE: false, old: apps.DaemonSet{ ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, Spec: apps.DaemonSetSpec{ @@ -1994,22 +2023,25 @@ func TestValidateDaemonSetUpdate(t *testing.T) { }, } for testName, errorCase := range errorCases { - // ResourceVersion is required for updates. - errorCase.old.ObjectMeta.ResourceVersion = "1" - errorCase.update.ObjectMeta.ResourceVersion = "2" - // Check test setup - if errorCase.expectedErrNum <= 0 { - t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected at least one error", testName, errorCase.expectedErrNum) - } - if len(errorCase.old.ObjectMeta.ResourceVersion) == 0 || len(errorCase.update.ObjectMeta.ResourceVersion) == 0 { - t.Errorf("%q has incorrect test setup with no resource version set", testName) - } - // Run the tests - if errs := ValidateDaemonSetUpdate(&errorCase.update, &errorCase.old, corevalidation.PodValidationOptions{}); len(errs) != errorCase.expectedErrNum { - t.Errorf("%q expected %d errors, but got %d error: %v", testName, errorCase.expectedErrNum, len(errs), errs) - } else { - t.Logf("(PASS) %q got errors %v", testName, errs) - } + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, errorCase.enableSkipReadOnlyValidationGCE)() + // ResourceVersion is required for updates. + errorCase.old.ObjectMeta.ResourceVersion = "1" + errorCase.update.ObjectMeta.ResourceVersion = "2" + // Check test setup + if errorCase.expectedErrNum <= 0 { + t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected at least one error", testName, errorCase.expectedErrNum) + } + if len(errorCase.old.ObjectMeta.ResourceVersion) == 0 || len(errorCase.update.ObjectMeta.ResourceVersion) == 0 { + t.Errorf("%q has incorrect test setup with no resource version set", testName) + } + // Run the tests + if errs := ValidateDaemonSetUpdate(&errorCase.update, &errorCase.old, corevalidation.PodValidationOptions{}); len(errs) != errorCase.expectedErrNum { + t.Errorf("%q expected %d errors, but got %d error: %v", testName, errorCase.expectedErrNum, len(errs), errs) + } else { + t.Logf("(PASS) %q got errors %v", testName, errs) + } + }) } } @@ -2590,6 +2622,217 @@ func validDeploymentRollback() *apps.DeploymentRollback { } } +func TestValidateDeploymentUpdate(t *testing.T) { + validLabels := map[string]string{"a": "b"} + validPodTemplate := api.PodTemplate{ + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: validLabels, + }, + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyAlways, + DNSPolicy: api.DNSClusterFirst, + Containers: []api.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: api.TerminationMessageReadFile}}, + }, + }, + } + readWriteVolumePodTemplate := api.PodTemplate{ + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: validLabels, + }, + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyAlways, + DNSPolicy: api.DNSClusterFirst, + Containers: []api.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: api.TerminationMessageReadFile}}, + Volumes: []api.Volume{{Name: "gcepd", VolumeSource: api.VolumeSource{GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{PDName: "my-PD", FSType: "ext4", Partition: 1, ReadOnly: false}}}}, + }, + }, + } + invalidLabels := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} + invalidPodTemplate := api.PodTemplate{ + Template: api.PodTemplateSpec{ + Spec: api.PodSpec{ + // no containers specified + RestartPolicy: api.RestartPolicyAlways, + DNSPolicy: api.DNSClusterFirst, + }, + ObjectMeta: metav1.ObjectMeta{ + Labels: invalidLabels, + }, + }, + } + type depUpdateTest struct { + old apps.Deployment + update apps.Deployment + expectedErrNum int + enableSkipReadOnlyValidationGCE bool + } + successCases := map[string]depUpdateTest{ + "positive replicas": { + old: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + update: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Replicas: 1, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: readWriteVolumePodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + }, + "Read-write volume verification": { + enableSkipReadOnlyValidationGCE: true, + old: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + update: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Replicas: 2, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: readWriteVolumePodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + }, + } + for testName, successCase := range successCases { + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, successCase.enableSkipReadOnlyValidationGCE)() + // ResourceVersion is required for updates. + successCase.old.ObjectMeta.ResourceVersion = "1" + successCase.update.ObjectMeta.ResourceVersion = "2" + // Check test setup + if successCase.expectedErrNum > 0 { + t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected no error", testName, successCase.expectedErrNum) + } + if len(successCase.old.ObjectMeta.ResourceVersion) == 0 || len(successCase.update.ObjectMeta.ResourceVersion) == 0 { + t.Errorf("%q has incorrect test setup with no resource version set", testName) + } + // Run the tests + if errs := ValidateDeploymentUpdate(&successCase.update, &successCase.old, corevalidation.PodValidationOptions{}); len(errs) != 0 { + t.Errorf("%q expected no error, but got: %v", testName, errs) + } + }) + errorCases := map[string]depUpdateTest{ + "more than one read/write": { + old: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + update: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Replicas: 2, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: readWriteVolumePodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + expectedErrNum: 2, + }, + "invalid selector": { + old: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + update: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Replicas: 2, + Selector: &metav1.LabelSelector{MatchLabels: invalidLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + expectedErrNum: 3, + }, + "invalid pod": { + old: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + update: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Replicas: 2, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: invalidPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + expectedErrNum: 4, + }, + "negative replicas": { + old: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: validPodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + update: apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.DeploymentSpec{ + Replicas: -1, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + Template: readWriteVolumePodTemplate.Template, + Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}, + }, + }, + expectedErrNum: 1, + }, + } + for testName, errorCase := range errorCases { + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, errorCase.enableSkipReadOnlyValidationGCE)() + // ResourceVersion is required for updates. + errorCase.old.ObjectMeta.ResourceVersion = "1" + errorCase.update.ObjectMeta.ResourceVersion = "2" + // Check test setup + if errorCase.expectedErrNum <= 0 { + t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected at least one error", testName, errorCase.expectedErrNum) + } + if len(errorCase.old.ObjectMeta.ResourceVersion) == 0 || len(errorCase.update.ObjectMeta.ResourceVersion) == 0 { + t.Errorf("%q has incorrect test setup with no resource version set", testName) + } + // Run the tests + if errs := ValidateDeploymentUpdate(&errorCase.update, &errorCase.old, corevalidation.PodValidationOptions{}); len(errs) != errorCase.expectedErrNum { + t.Errorf("%q expected %d errors, but got %d error: %v", testName, errorCase.expectedErrNum, len(errs), errs) + } else { + t.Logf("(PASS) %q got errors %v", testName, errs) + } + }) + } + } +} + func TestValidateDeploymentRollback(t *testing.T) { noAnnotation := validDeploymentRollback() noAnnotation.UpdatedAnnotations = nil @@ -2860,11 +3103,13 @@ func TestValidateReplicaSetUpdate(t *testing.T) { }, } type rcUpdateTest struct { - old apps.ReplicaSet - update apps.ReplicaSet + old apps.ReplicaSet + update apps.ReplicaSet + expectedErrNum int + enableSkipReadOnlyValidationGCE bool } - successCases := []rcUpdateTest{ - { + successCases := map[string]rcUpdateTest{ + "positive replicas": { old: apps.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, Spec: apps.ReplicaSetSpec{ @@ -2881,7 +3126,8 @@ func TestValidateReplicaSetUpdate(t *testing.T) { }, }, }, - { + "Read-write volume verification": { + enableSkipReadOnlyValidationGCE: true, old: apps.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, Spec: apps.ReplicaSetSpec{ @@ -2892,19 +3138,31 @@ func TestValidateReplicaSetUpdate(t *testing.T) { update: apps.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, Spec: apps.ReplicaSetSpec{ - Replicas: 1, + Replicas: 3, Selector: &metav1.LabelSelector{MatchLabels: validLabels}, Template: readWriteVolumePodTemplate.Template, }, }, }, } - for _, successCase := range successCases { - successCase.old.ObjectMeta.ResourceVersion = "1" - successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateReplicaSetUpdate(&successCase.update, &successCase.old, corevalidation.PodValidationOptions{}); len(errs) != 0 { - t.Errorf("expected success: %v", errs) - } + for testName, successCase := range successCases { + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, successCase.enableSkipReadOnlyValidationGCE)() + // ResourceVersion is required for updates. + successCase.old.ObjectMeta.ResourceVersion = "1" + successCase.update.ObjectMeta.ResourceVersion = "2" + // Check test setup + if successCase.expectedErrNum > 0 { + t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected no error", testName, successCase.expectedErrNum) + } + if len(successCase.old.ObjectMeta.ResourceVersion) == 0 || len(successCase.update.ObjectMeta.ResourceVersion) == 0 { + t.Errorf("%q has incorrect test setup with no resource version set", testName) + } + // Run the tests + if errs := ValidateReplicaSetUpdate(&successCase.update, &successCase.old, corevalidation.PodValidationOptions{}); len(errs) != 0 { + t.Errorf("%q expected no error, but got: %v", testName, errs) + } + }) } errorCases := map[string]rcUpdateTest{ "more than one read/write": { @@ -2923,6 +3181,7 @@ func TestValidateReplicaSetUpdate(t *testing.T) { Template: readWriteVolumePodTemplate.Template, }, }, + expectedErrNum: 2, }, "invalid selector": { old: apps.ReplicaSet{ @@ -2940,6 +3199,7 @@ func TestValidateReplicaSetUpdate(t *testing.T) { Template: validPodTemplate.Template, }, }, + expectedErrNum: 3, }, "invalid pod": { old: apps.ReplicaSet{ @@ -2957,6 +3217,7 @@ func TestValidateReplicaSetUpdate(t *testing.T) { Template: invalidPodTemplate.Template, }, }, + expectedErrNum: 4, }, "negative replicas": { old: apps.ReplicaSet{ @@ -2974,12 +3235,29 @@ func TestValidateReplicaSetUpdate(t *testing.T) { Template: validPodTemplate.Template, }, }, + expectedErrNum: 1, }, } for testName, errorCase := range errorCases { - if errs := ValidateReplicaSetUpdate(&errorCase.update, &errorCase.old, corevalidation.PodValidationOptions{}); len(errs) == 0 { - t.Errorf("expected failure: %s", testName) - } + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, errorCase.enableSkipReadOnlyValidationGCE)() + // ResourceVersion is required for updates. + errorCase.old.ObjectMeta.ResourceVersion = "1" + errorCase.update.ObjectMeta.ResourceVersion = "2" + // Check test setup + if errorCase.expectedErrNum <= 0 { + t.Errorf("%q has incorrect test setup with expectedErrNum %d, expected at least one error", testName, errorCase.expectedErrNum) + } + if len(errorCase.old.ObjectMeta.ResourceVersion) == 0 || len(errorCase.update.ObjectMeta.ResourceVersion) == 0 { + t.Errorf("%q has incorrect test setup with no resource version set", testName) + } + // Run the tests + if errs := ValidateReplicaSetUpdate(&errorCase.update, &errorCase.old, corevalidation.PodValidationOptions{}); len(errs) != errorCase.expectedErrNum { + t.Errorf("%q expected %d errors, but got %d error: %v", testName, errorCase.expectedErrNum, len(errs), errs) + } else { + t.Logf("(PASS) %q got errors %v", testName, errs) + } + }) } } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 465c92380a9..a4c88ed415a 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5238,14 +5238,14 @@ func ValidateServiceStatusUpdate(service, oldService *core.Service) field.ErrorL // ValidateReplicationController tests if required fields in the replication controller are set. func ValidateReplicationController(controller *core.ReplicationController, opts PodValidationOptions) field.ErrorList { allErrs := ValidateObjectMeta(&controller.ObjectMeta, true, ValidateReplicationControllerName, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec, nil, field.NewPath("spec"), opts)...) return allErrs } // ValidateReplicationControllerUpdate tests if required fields in the replication controller are set. func ValidateReplicationControllerUpdate(controller, oldController *core.ReplicationController, opts PodValidationOptions) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&controller.ObjectMeta, &oldController.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec, field.NewPath("spec"), opts)...) + allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec, &oldController.Spec, field.NewPath("spec"), opts)...) return allErrs } @@ -5290,7 +5290,7 @@ func ValidateNonEmptySelector(selectorMap map[string]string, fldPath *field.Path } // Validates the given template and ensures that it is in accordance with the desired selector and replicas. -func ValidatePodTemplateSpecForRC(template *core.PodTemplateSpec, selectorMap map[string]string, replicas int32, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func ValidatePodTemplateSpecForRC(template, oldTemplate *core.PodTemplateSpec, selectorMap map[string]string, replicas int32, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if template == nil { allErrs = append(allErrs, field.Required(fldPath, "")) @@ -5304,8 +5304,13 @@ func ValidatePodTemplateSpecForRC(template *core.PodTemplateSpec, selectorMap ma } } allErrs = append(allErrs, ValidatePodTemplateSpec(template, fldPath, opts)...) + // get rid of apivalidation.ValidateReadOnlyPersistentDisks,stop passing oldTemplate to this function + var oldVols []core.Volume + if oldTemplate != nil { + oldVols = oldTemplate.Spec.Volumes // +k8s:verify-mutation:reason=clone + } if replicas > 1 { - allErrs = append(allErrs, ValidateReadOnlyPersistentDisks(template.Spec.Volumes, fldPath.Child("spec", "volumes"))...) + allErrs = append(allErrs, ValidateReadOnlyPersistentDisks(template.Spec.Volumes, oldVols, fldPath.Child("spec", "volumes"))...) } // RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec(). if template.Spec.RestartPolicy != core.RestartPolicyAlways { @@ -5319,12 +5324,17 @@ func ValidatePodTemplateSpecForRC(template *core.PodTemplateSpec, selectorMap ma } // ValidateReplicationControllerSpec tests if required fields in the replication controller spec are set. -func ValidateReplicationControllerSpec(spec *core.ReplicationControllerSpec, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationControllerSpec, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...) allErrs = append(allErrs, ValidateNonEmptySelector(spec.Selector, fldPath.Child("selector"))...) allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...) - allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, spec.Selector, spec.Replicas, fldPath.Child("template"), opts)...) + // oldSpec is not empty, pass oldSpec.template. + var oldTemplate *core.PodTemplateSpec + if oldSpec != nil { + oldTemplate = oldSpec.Template // +k8s:verify-mutation:reason=clone + } + allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, oldTemplate, spec.Selector, spec.Replicas, fldPath.Child("template"), opts)...) return allErrs } @@ -5344,17 +5354,29 @@ func ValidatePodTemplateSpec(spec *core.PodTemplateSpec, fldPath *field.Path, op return allErrs } -func ValidateReadOnlyPersistentDisks(volumes []core.Volume, fldPath *field.Path) field.ErrorList { +// ValidateReadOnlyPersistentDisks stick this AFTER the short-circuit checks +func ValidateReadOnlyPersistentDisks(volumes, oldVolumes []core.Volume, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - for i := range volumes { - vol := &volumes[i] - idxPath := fldPath.Index(i) - if vol.GCEPersistentDisk != nil { - if !vol.GCEPersistentDisk.ReadOnly { - allErrs = append(allErrs, field.Invalid(idxPath.Child("gcePersistentDisk", "readOnly"), false, "must be true for replicated pods > 1; GCE PD can only be mounted on multiple machines if it is read-only")) - } + + if utilfeature.DefaultFeatureGate.Enabled(features.SkipReadOnlyValidationGCE) { + return field.ErrorList{} + } + + isWriteablePD := func(vol *core.Volume) bool { + return vol.GCEPersistentDisk != nil && !vol.GCEPersistentDisk.ReadOnly + } + + for i := range oldVolumes { + if isWriteablePD(&oldVolumes[i]) { + return field.ErrorList{} + } + } + + for i := range volumes { + idxPath := fldPath.Index(i) + if isWriteablePD(&volumes[i]) { + allErrs = append(allErrs, field.Invalid(idxPath.Child("gcePersistentDisk", "readOnly"), false, "must be true for replicated pods > 1; GCE PD can only be mounted on multiple machines if it is read-only")) } - // TODO: What to do for AWS? It doesn't support replicas } return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index f210b336880..cb911e661b1 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -5310,6 +5310,111 @@ func TestValidateVolumes(t *testing.T) { } +func TestValidateReadOnlyPersistentDisks(t *testing.T) { + cases := []struct { + name string + volumes []core.Volume + oldVolume []core.Volume + gateValue bool + expectError bool + }{ + { + name: "gate on, read-only disk, nil old", + gateValue: true, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume(nil), + expectError: false, + }, + { + name: "gate off, read-only disk, nil old", + gateValue: false, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume(nil), + expectError: false, + }, + { + name: "gate on, read-write, nil old", + gateValue: true, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + oldVolume: []core.Volume(nil), + expectError: false, + }, + { + name: "gate off, read-write, nil old", + gateValue: false, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + oldVolume: []core.Volume(nil), + expectError: true, + }, + { + name: "gate on, new read-only and old read-write", + gateValue: true, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + expectError: false, + }, + { + name: "gate off, new read-only and old read-write", + gateValue: false, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + expectError: false, + }, + { + name: "gate on, new read-write and old read-write", + gateValue: true, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + expectError: false, + }, + { + name: "gate off, new read-write and old read-write", + gateValue: false, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + expectError: false, + }, + { + name: "gate on, new read-only and old read-only", + gateValue: true, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + expectError: false, + }, + { + name: "gate off, new read-only and old read-only", + gateValue: false, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + expectError: false, + }, + { + name: "gate on, new read-write and old read-only", + gateValue: true, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + expectError: false, + }, + { + name: "gate off, new read-write and old read-only", + gateValue: false, + volumes: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: false}}}}, + oldVolume: []core.Volume{{VolumeSource: core.VolumeSource{GCEPersistentDisk: &core.GCEPersistentDiskVolumeSource{ReadOnly: true}}}}, + expectError: true, + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + fidPath := field.NewPath("testField") + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SkipReadOnlyValidationGCE, testCase.gateValue)() + errs := ValidateReadOnlyPersistentDisks(testCase.volumes, testCase.oldVolume, fidPath) + if !testCase.expectError && len(errs) != 0 { + t.Errorf("expected success, got:%v", errs) + } + }) + } +} + func TestHugePagesIsolation(t *testing.T) { testCases := map[string]struct { pod *core.Pod diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 866d9328f90..42b97a038b0 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -137,6 +137,12 @@ const ( // Enables the GCE PD in-tree driver to GCE CSI Driver migration feature. CSIMigrationGCE featuregate.Feature = "CSIMigrationGCE" + // owner: @mfordjody + // alpha: v1.26 + // + // Skip validation Enable in next version + SkipReadOnlyValidationGCE featuregate.Feature = "SkipReadOnlyValidationGCE" + // owner: @trierra // alpha: v1.23 // @@ -951,6 +957,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS CSIVolumeHealth: {Default: false, PreRelease: featuregate.Alpha}, + SkipReadOnlyValidationGCE: {Default: false, PreRelease: featuregate.Alpha}, + CloudControllerManagerWebhook: {Default: false, PreRelease: featuregate.Alpha}, ContainerCheckpoint: {Default: false, PreRelease: featuregate.Alpha},