Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove the gate "SkipReadOnlyValidationGCE" #124210

Merged
merged 1 commit into from Apr 18, 2024
+17 −260
Merged

Remove the gate "SkipReadOnlyValidationGCE" #124210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 6 additions & 31 deletions pkg/apis/apps/validation/validation.go
View file
Open in desktop
Expand Up @@ -280,15 +280,15 @@ 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, nil, field.NewPath("spec"), opts)...)
allErrs = append(allErrs, ValidateDaemonSetSpec(&ds.Spec, field.NewPath("spec"), opts)...)
return allErrs
}

// ValidateDaemonSetUpdate tests if required fields in the DaemonSet are set.
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, &oldDS.Spec, field.NewPath("spec"), opts)...)
allErrs = append(allErrs, ValidateDaemonSetSpec(&ds.Spec, field.NewPath("spec"), opts)...)
return allErrs
}

Expand Down Expand Up @@ -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, oldSpec *apps.DaemonSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList {
func ValidateDaemonSetSpec(spec *apps.DaemonSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList {
allErrs := field.ErrorList{}
labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector}

Expand All @@ -359,12 +359,6 @@ func ValidateDaemonSetSpec(spec, oldSpec *apps.DaemonSetSpec, fldPath *field.Pat
}

allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(&spec.Template, fldPath.Child("template"), opts)...)
// 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)}))
Expand Down Expand Up @@ -566,12 +560,7 @@ func ValidateDeploymentSpec(spec, oldSpec *apps.DeploymentSpec, fldPath *field.P
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "invalid label selector"))
} else {
// 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, ValidatePodTemplateSpecForReplicaSet(&spec.Template, selector, spec.Replicas, fldPath.Child("template"), opts)...)
}

allErrs = append(allErrs, ValidateDeploymentStrategy(&spec.Strategy, fldPath.Child("strategy"))...)
Expand Down Expand Up @@ -734,18 +723,13 @@ func ValidateReplicaSetSpec(spec, oldSpec *apps.ReplicaSetSpec, fldPath *field.P
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "invalid label selector"))
} else {
// 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, ValidatePodTemplateSpecForReplicaSet(&spec.Template, 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, oldTemplate *api.PodTemplateSpec, selector labels.Selector, replicas int32, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList {
func ValidatePodTemplateSpecForReplicaSet(template *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, ""))
Expand All @@ -758,15 +742,6 @@ func ValidatePodTemplateSpecForReplicaSet(template, oldTemplate *api.PodTemplate
}
}
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, oldVols, fldPath.Child("spec", "volumes"))...)
}
// RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec().
if template.Spec.RestartPolicy != api.RestartPolicyAlways {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("spec", "restartPolicy"), template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)}))
Expand Down
94 changes: 9 additions & 85 deletions pkg/apis/apps/validation/validation_test.go
View file
Open in desktop
Expand Up @@ -1572,10 +1572,9 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
},
}
type dsUpdateTest struct {
old apps.DaemonSet
update apps.DaemonSet
expectedErrNum int
enableSkipReadOnlyValidationGCE bool
old apps.DaemonSet
update apps.DaemonSet
expectedErrNum int
}
successCases := map[string]dsUpdateTest{
"no change": {
Expand Down Expand Up @@ -1729,7 +1728,6 @@ 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{
Expand All @@ -1756,7 +1754,6 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
}
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"
Expand Down Expand Up @@ -1848,32 +1845,6 @@ 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{
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,
},
},
},
expectedErrNum: 1,
},
"invalid update strategy": {
old: apps.DaemonSet{
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
Expand Down Expand Up @@ -1977,7 +1948,6 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
}
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"
Expand Down Expand Up @@ -2645,10 +2615,9 @@ func TestValidateDeploymentUpdate(t *testing.T) {
},
}
type depUpdateTest struct {
old apps.Deployment
update apps.Deployment
expectedErrNum int
enableSkipReadOnlyValidationGCE bool
old apps.Deployment
update apps.Deployment
expectedErrNum int
}
successCases := map[string]depUpdateTest{
"positive replicas": {
Expand All @@ -2671,7 +2640,6 @@ func TestValidateDeploymentUpdate(t *testing.T) {
},
},
"Read-write volume verification": {
enableSkipReadOnlyValidationGCE: true,
old: apps.Deployment{
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
Spec: apps.DeploymentSpec{
Expand All @@ -2693,7 +2661,6 @@ func TestValidateDeploymentUpdate(t *testing.T) {
}
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"
Expand All @@ -2710,26 +2677,6 @@ func TestValidateDeploymentUpdate(t *testing.T) {
}
})
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},
Expand Down Expand Up @@ -2793,7 +2740,6 @@ func TestValidateDeploymentUpdate(t *testing.T) {
}
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"
Expand Down Expand Up @@ -3074,10 +3020,9 @@ func TestValidateReplicaSetUpdate(t *testing.T) {
},
}
type rcUpdateTest struct {
old apps.ReplicaSet
update apps.ReplicaSet
expectedErrNum int
enableSkipReadOnlyValidationGCE bool
old apps.ReplicaSet
update apps.ReplicaSet
expectedErrNum int
}
successCases := map[string]rcUpdateTest{
"positive replicas": {
Expand All @@ -3098,7 +3043,6 @@ 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{
Expand All @@ -3118,7 +3062,6 @@ func TestValidateReplicaSetUpdate(t *testing.T) {
}
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"
Expand All @@ -3136,24 +3079,6 @@ func TestValidateReplicaSetUpdate(t *testing.T) {
})
}
errorCases := map[string]rcUpdateTest{
"more than one read/write": {
old: apps.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault},
Spec: apps.ReplicaSetSpec{
Selector: &metav1.LabelSelector{MatchLabels: validLabels},
Template: validPodTemplate.Template,
},
},
update: apps.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
Spec: apps.ReplicaSetSpec{
Replicas: 2,
Selector: &metav1.LabelSelector{MatchLabels: validLabels},
Template: readWriteVolumePodTemplate.Template,
},
},
expectedErrNum: 2,
},
"invalid selector": {
old: apps.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault},
Expand Down Expand Up @@ -3211,7 +3136,6 @@ func TestValidateReplicaSetUpdate(t *testing.T) {
}
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"
Expand Down
44 changes: 2 additions & 42 deletions pkg/apis/core/validation/validation.go
View file
Open in desktop
Expand Up @@ -5910,7 +5910,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, oldTemplate *core.PodTemplateSpec, selectorMap map[string]string, replicas int32, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
func ValidatePodTemplateSpecForRC(template *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, ""))
Expand All @@ -5924,14 +5924,6 @@ func ValidatePodTemplateSpecForRC(template, oldTemplate *core.PodTemplateSpec, s
}
}
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, oldVols, fldPath.Child("spec", "volumes"))...)
}
// RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec().
if template.Spec.RestartPolicy != core.RestartPolicyAlways {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("spec", "restartPolicy"), template.Spec.RestartPolicy, []core.RestartPolicy{core.RestartPolicyAlways}))
Expand All @@ -5949,12 +5941,7 @@ func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationController
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"))...)
// 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)...)
allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, spec.Selector, spec.Replicas, fldPath.Child("template"), opts)...)
return allErrs
}

Expand All @@ -5975,33 +5962,6 @@ func ValidatePodTemplateSpec(spec *core.PodTemplateSpec, fldPath *field.Path, op
return allErrs
}

// ValidateReadOnlyPersistentDisks stick this AFTER the short-circuit checks
func ValidateReadOnlyPersistentDisks(volumes, oldVolumes []core.Volume, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

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"))
}
}
return allErrs
}

// ValidateTaintsInNodeAnnotations tests that the serialized taints in Node.Annotations has valid data
func ValidateTaintsInNodeAnnotations(annotations map[string]string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand Down