Re: [kubernetes/kubernetes] introduce ratcheting validation mechanism (#64907)

[Jordan Liggitt]

rebased and updated. tagging in sigs for review of area-specific validation tightening

@kubernetes/sig-storage-pr-reviews:

• validate flex volume driver name
• validate emptyDir medium
• validate duplicate PVC volume names

@kubernetes/sig-node-pr-reviews:

• validate memory units are not fractional

@kubernetes/sig-api-machinery-pr-reviews:

• prevent duplicate envvar names (broke apply/update removals)

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[Jordan Liggitt]

cc @kubernetes/api-reviewers @kubernetes/api-approvers

this gives us a way to prevent new objects that are fatally flawed, while not causing existing persisted data to be rejected during update/delete requests required to migrate storage and clean them up

[Dr. Stefan Schimanski]

@sttts commented on this pull request.

---

In staging/src/k8s.io/apimachinery/pkg/api/validation/ratcheting.go:

> +	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+type RatchetingValidators []RatchetingValidator
+
+type RatchetingValidator func(data interface{}, fldPath *field.Path) field.ErrorList
+
+func ValidateRatchetingCreate(newData interface{}, fldPath *field.Path, validators []RatchetingValidator) field.ErrorList {
+	allErrs := field.ErrorList{}
+	for _, validator := range validators {
+		allErrs = append(allErrs, validator(newData, fldPath)...)
+	}
+	return allErrs
+}
+
+func ValidateRatchetingUpdate(newData, existingData interface{}, fldPath *field.Path, validators RatchetingValidators) field.ErrorList {

go doc.

[Dr. Stefan Schimanski]

@sttts commented on this pull request.

---

In pkg/registry/core/pod/strategy.go:

>  	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	apimachineryvalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	apimachineryv1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"

nit: metav1validation

[Dr. Stefan Schimanski]

@sttts commented on this pull request.

---

In pkg/apis/core/validation/ratcheting.go:

> +				} else {
+					names.Insert(e.Name)
+				}
+			}
+		})
+		return allErrs
+	},
+
+	// duplicate pvc volumes
+	func(data interface{}, fldPath *field.Path) field.ErrorList {
+		allErrs := field.ErrorList{}
+		spec := data.(*core.PodSpec)
+		names := sets.NewString()
+		for i, v := range spec.Volumes {
+			if v.VolumeSource.PersistentVolumeClaim != nil {
+				if names.Has(v.VolumeSource.PersistentVolumeClaim.ClaimName) {

nit: if cn := v.VolumeSource.PersistentVolumeClaim; cn != nil {

then use cn below. Easier to read.

[Dr. Stefan Schimanski]

Looks good overall. Some nits. Please squash.

[Jordan Liggitt]

Looks good overall. Some nits. Please squash.

Fixed nits. Will squash after individual validations are reviewed

[Jordan Liggitt]

/cc @msau42 @jsafrane
for storage reviews

/cc @tallclair @derekwaynecarr
for node reviews

[k8s-ci-robot]

@liggitt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	7e463d9	link	/test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[Jan Šafránek]

storage validation lgtm

[Tim Allclair (St. Clair)]

@tallclair commented on this pull request.

---

In pkg/apis/core/validation/ratcheting.go:

> +				if names.Has(pvc.ClaimName) {
+					allErrs = append(allErrs, field.Duplicate(fldPath.Child("volumes").Index(i).Child("persistentVolumeClaim").Child("claimName"), pvc.ClaimName))
+				} else {
+					names.Insert(pvc.ClaimName)
+				}
+			}
+		}
+		return allErrs
+	},
+
+	// memory units
+	func(data interface{}, fldPath *field.Path) field.ErrorList {
+		allErrs := field.ErrorList{}
+		visitContainers(data.(*core.PodSpec), fldPath, func(c *core.Container, fldPath *field.Path) {
+			for k, q := range c.Resources.Limits {
+				if k == core.ResourceMemory && q.MilliValue()%1000 != 0 {

Comment on MilliValue notes:

// MilliValue returns the value of ceil(q * 1000); this could overflow an int64;
// if that's a concern, call Value() first to verify the number is small enough.

Can you just do: _, loss := q.AsScale(0); loss ?

[Jordan Liggitt]

@liggitt commented on this pull request.

---

In pkg/apis/core/validation/ratcheting.go:

> +				if names.Has(pvc.ClaimName) {
+					allErrs = append(allErrs, field.Duplicate(fldPath.Child("volumes").Index(i).Child("persistentVolumeClaim").Child("claimName"), pvc.ClaimName))
+				} else {
+					names.Insert(pvc.ClaimName)
+				}
+			}
+		}
+		return allErrs
+	},
+
+	// memory units
+	func(data interface{}, fldPath *field.Path) field.ErrorList {
+		allErrs := field.ErrorList{}
+		visitContainers(data.(*core.PodSpec), fldPath, func(c *core.Container, fldPath *field.Path) {
+			for k, q := range c.Resources.Limits {
+				if k == core.ResourceMemory && q.MilliValue()%1000 != 0 {

done (though the loss bool was flipped)

[Jordan Liggitt]

updated and squashed

[Tim Allclair (St. Clair)]

/cc @dashpole
/approve
for resource validation

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/apis/core/OWNERS [liggitt]
• pkg/registry/OWNERS [liggitt]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [liggitt]
• test/e2e/apps/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@liggitt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command

pull-kubernetes-e2e-gce

71918f8

link

/test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

—

[Nikhita Raghunath]

/retest

[Jordan Liggitt]

I think all aspects have been reviewed/approved:

• Mechanism has lgtm from @sttts and @lavalamp
• Storage has lgtm from @jsafrane
• Resource has approval from @tallclair, can you lgtm as well?

[Tim Allclair (St. Clair)]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/apis/core/OWNERS [liggitt]
• pkg/registry/OWNERS [liggitt]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [liggitt]
• test/e2e/apps/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

—

[Brian Grant]

Sorry for not noticing this earlier.

I commented on #64841

[Jordan Liggitt]

/hold

[Brian Grant]

@liggitt Is the idea that every release the ratcheting validations would be moved to standard ones, and potentially new ratcheting validations would be introduced?

[Jordan Liggitt]

Is the idea that every release the ratcheting validations would be moved to standard ones, and potentially new ratcheting validations would be introduced?

No, they couldn't be moved to be standard validations until we could guarantee the API servers would never encounter persisted etcd data that would fail those validations.

[Tim Allclair (St. Clair)]

/lgtm cancel
See #64841 (comment)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/apis/core/OWNERS [liggitt]
• pkg/registry/OWNERS [liggitt]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [liggitt]
• test/e2e/apps/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

—

[k8s-ci-robot]

@liggitt: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

—

[Eric Tune]

Hi, sorry to be late to the party, but some here is some feedback on this approach:

• Yes, we should have CRD support for ratcheting, and it should ideally be very similar to the built-in support.
• Annotating or modifying the CRD schema to express RatchetingValidations seems to me like it scales poorly.
• Instead of having special RatchetingValidation functions, instead, one could instead introduce a new api version with stronger validation. Then one can set the desired storage version (as CRDs do) to lazily move over objects or run a "storage version upgrade script" procedure to eagerly move over objects.
• To address the complaint that we don't want to move pods to v2, we could introduce a "microversion" and the validation uses both apiversion and metadata.microversion to compose a tuple which is used to select what functions/schema is used for validation.

[Jordan Liggitt]

Instead of having special RatchetingValidation functions, instead, one could instead introduce a new api version with stronger validation. Then one can set the desired storage version (as CRDs do) to lazily move over objects or run a "storage version upgrade script" procedure to eagerly move over objects.

What would the storage migration do with existing objects that would not pass the new stricter validation?

As long as the previous API version is still usable (or data persisted via the previous API version is still able to be encountered), we have to handle round-tripping and updating that object via the new API version. Making an update of an unrelated field (like an annotation or a finalizer) fail for validation reasons is problematic.

[Brian Grant]

Quick note: We don't have versioned validation yet: #8550

[CaoShuFeng]

@CaoShuFeng commented on this pull request.

---

In pkg/registry/batch/cronjob/strategy.go:

> @@ -70,7 +72,13 @@ func (cronJobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob
 // Validate validates a new scheduled job.
 func (cronJobStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	cronJob := obj.(*batch.CronJob)
-	return validation.ValidateCronJob(cronJob)
+	allErrs := validation.ValidateCronJob(cronJob)
+	allErrs = append(allErrs, apimachineryvalidation.ValidateRatchetingCreate(
+		&cronJob.Spec.JobTemplate.Spec.Template.Spec,
+		field.NewPath("spec", "template", "spec", "template", "spec"),

nit: field.NewPath("spec", "jobTemplate", "spec", "template", "spec")

[Maria Ntalla]

@hoegaarden and I were also wondering what you thought about the following:

1. The capability of also applying preprogrammed "migrations" to invalid objects? In other words, the next step after a failed validation could be to apply some logic to automatically reconcile it/ make it compatible with the new api version.

2. The validators being extracted from the body of the api server. This is perhaps more of a design consideration, but could make it easier to

• maintain validator collections in future versions of the API server
• maybe even allow operators to define their own additional validators and configure the API server with those. Maybe admission webhooks already offer the tools for that use case though. 🤔

[Davanum Srinivas]

@liggitt should we shoot for v1.12?

[Jordan Liggitt]

should we shoot for v1.12?

This isn't a top priority for the next week, so no