Refactor: Use field.Required for missing field errors #132536

xiaoweim · 2025-06-25T19:15:49Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This commit refactors validation logic to use field.Required in cases
where field.Invalid was previously used to indicate that a field was
missing. According to the issue description, field.Invalid was sometimes
used with a detail message like "must be set", which is semantically
a "required" error.

This is the third and final part of the cleanup work for this issue #111698

Which issue(s) this PR is related to:

Fixes 111698

Does this PR introduce a user-facing change?

Clarified validation error messages by using Required instead of Invalid when a required field is missing

k8s-ci-robot · 2025-06-25T19:15:59Z

Hi @xiaoweim. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

xiaoweim · 2025-06-25T19:25:33Z

/priority important-longterm

xiaoweim · 2025-06-25T19:25:41Z

/cc @aaron-prindle

k8s-triage-robot · 2025-06-25T20:13:49Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

aaron-prindle · 2025-06-25T21:20:03Z

/ok-to-test
/triage accepted
/sig api-machinery

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

k8s-ci-robot · 2025-06-26T09:07:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: neolit123, xiaoweim
Once this PR has been reviewed and has the lgtm label, please assign deads2k, thockin for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

xiaoweim · 2025-06-30T17:24:19Z

/retest

thockin

I didn't review all of it super closely -- these ones need to look at what's ACTUALLY being validated.

Only API fields can be "required" and that means either the field value or pointer, but never what is pointed-to (that would be Invalid) or items in a list.

Might make sense to break this into several smaller PRs so we can merge and make progress inrementally.

thockin · 2025-07-01T06:34:23Z

pkg/apis/authorization/validation/validation.go

@@ -34,10 +34,10 @@ func ValidateSubjectAccessReviewSpec(spec authorizationapi.SubjectAccessReviewSp
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("nonResourceAttributes"), spec.NonResourceAttributes, `cannot be specified in combination with resourceAttributes`))
 	}
 	if spec.ResourceAttributes == nil && spec.NonResourceAttributes == nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceAttributes"), spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`))
+		allErrs = append(allErrs, field.Required(fldPath.Child("resourceAttributes"), `exactly one of nonResourceAttributes or resourceAttributes must be specified`))


This should stay as Invalid, but the fieldPath should indicate the parent (fldPath)

thockin · 2025-07-01T06:34:36Z

pkg/apis/authorization/validation/validation.go

 	}
 	if len(spec.User) == 0 && len(spec.Groups) == 0 {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("user"), spec.User, `at least one of user or group must be specified`))
+		allErrs = append(allErrs, field.Required(fldPath.Child("user"), `at least one of user or group must be specified`))


thockin · 2025-07-01T06:35:12Z

pkg/apis/authorization/validation/validation.go

@@ -52,7 +52,7 @@ func ValidateSelfSubjectAccessReviewSpec(spec authorizationapi.SelfSubjectAccess
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("nonResourceAttributes"), spec.NonResourceAttributes, `cannot be specified in combination with resourceAttributes`))
 	}
 	if spec.ResourceAttributes == nil && spec.NonResourceAttributes == nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceAttributes"), spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`))
+		allErrs = append(allErrs, field.Required(fldPath.Child("resourceAttributes"), `exactly one of nonResourceAttributes or resourceAttributes must be specified`))


thockin · 2025-07-01T06:35:25Z

pkg/apis/authorization/validation/validation_test.go

@@ -101,7 +101,7 @@ func TestValidateSARSpec(t *testing.T) {
 		obj: authorizationapi.SubjectAccessReviewSpec{
 			ResourceAttributes: &authorizationapi.ResourceAttributes{},
 		},
-		msg: `spec.user: Invalid value: "": at least one of user or group must be specified`,
+		msg: `spec.user: Required value: at least one of user or group must be specified`,


thockin · 2025-07-01T06:35:56Z

pkg/apis/batch/validation/validation.go

@@ -348,7 +348,7 @@ func validatePodFailurePolicyRule(spec *batch.JobSpec, rule *batch.PodFailurePol
 		allErrs = append(allErrs, field.Invalid(rulePath, field.OmitValueType{}, "specifying both OnExitCodes and OnPodConditions is not supported"))
 	}
 	if rule.OnExitCodes == nil && len(rule.OnPodConditions) == 0 {
-		allErrs = append(allErrs, field.Invalid(rulePath, field.OmitValueType{}, "specifying one of OnExitCodes and OnPodConditions is required"))
+		allErrs = append(allErrs, field.Required(rulePath, "specifying one of OnExitCodes and OnPodConditions is required"))


one of ... must be specified

thockin · 2025-07-01T06:39:05Z

pkg/apis/core/validation/validation.go

@@ -8498,7 +8498,7 @@ func validateUpgradeDowngradeIPFamilies(oldService, service *core.Service) field
 		// user *must* set IPFamilyPolicy == SingleStack
 		if len(service.Spec.IPFamilies) == 1 {
 			if service.Spec.IPFamilyPolicy == nil || *(service.Spec.IPFamilyPolicy) != core.IPFamilyPolicySingleStack {
-				allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "must be set to 'SingleStack' when releasing the secondary ipFamily"))
+				allErrs = append(allErrs, field.Required(field.NewPath("spec", "ipFamilyPolicy"), "must be set to 'SingleStack' when releasing the secondary ipFamily"))


Invalid is better here

thockin · 2025-07-01T06:40:57Z

pkg/apis/flowcontrol/validation/validation.go

@@ -542,7 +542,7 @@ func ValidatePriorityLevelConfigurationCondition(condition *flowcontrol.Priority
 //  4. Wildcard "*" should only do suffix glob matching. Note that wildcard also matches slashes.
 func ValidateNonResourceURLPath(path string, fldPath *field.Path) *field.Error {
 	if len(path) == 0 {
-		return field.Invalid(fldPath, path, "must not be empty")
+		return field.Required(fldPath, "")


This is a trap! Look at how this is called - it is validating a string in a list -- Invalid is correct. Please look out for this in other places. I may have missed some

thockin · 2025-07-01T06:41:42Z

pkg/apis/networking/validation/validation.go

@@ -534,7 +534,7 @@ func validateIngressBackend(backend *networking.IngressBackend, fldPath *field.P
 			allErrs = append(allErrs, field.Required(fldPath, "port name or number is required"))
 		}
 	default:
-		allErrs = append(allErrs, field.Invalid(fldPath, "", "resource or service backend is required"))
+		allErrs = append(allErrs, field.Required(fldPath, "resource or service backend is required"))


For "one of" errors, we attribute to the common parent and use Invalid

thockin · 2025-07-01T06:42:58Z

pkg/apis/resource/validation/validation.go

@@ -763,7 +763,7 @@ func validateDevice(device resource.Device, fldPath *field.Path, sharedCounterTo
 				setFields = append(setFields, "`nodeName`")
 				allErrs = append(allErrs, validateNodeName(*device.NodeName, fldPath.Child("nodeName"))...)
 			} else {
-				allErrs = append(allErrs, field.Invalid(fldPath.Child("nodeName"), *device.NodeName, "must not be empty"))
+				allErrs = append(allErrs, field.Required(fldPath.Child("nodeName"), ""))


This is another of the same trap -- the field (pointer) was specified but the value was "" - Invalid

thockin · 2025-07-01T06:43:57Z

pkg/kubelet/kubelet_server_journal.go

@@ -136,7 +136,7 @@ func newNodeLogQuery(query url.Values) (*nodeLogQuery, field.ErrorList) {
 	// Prevent specifying  an empty or blank space query.
 	// Example: kubectl get --raw /api/v1/nodes/$node/proxy/logs?query="   "
 	if ok && (len(nlq.Files) == 0 && len(nlq.Services) == 0) {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("query"), queries, "query cannot be empty"))
+		allErrs = append(allErrs, field.Required(field.NewPath("query"), ""))


this is another one-of

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 25, 2025

k8s-ci-robot requested review from andrewsykim and aroradaman June 25, 2025 19:16

github-project-automation bot added this to SIG Apps and SIG Auth Jun 25, 2025

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 25, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Jun 25, 2025

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jun 25, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs Jun 25, 2025

k8s-ci-robot added the wg/device-management Categorizes an issue or PR as relevant to WG Device Management. label Jun 25, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Jun 25, 2025

github-project-automation bot added this to Dynamic Resource Allocation Jun 25, 2025

github-project-automation bot moved this to 🆕 New in Dynamic Resource Allocation Jun 25, 2025

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 25, 2025

xiaoweim marked this pull request as ready for review June 25, 2025 19:24

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2025

k8s-ci-robot requested review from aojea and bart0sh June 25, 2025 19:25

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 25, 2025

k8s-ci-robot requested a review from aaron-prindle June 25, 2025 19:25

neolit123 reviewed Jun 26, 2025

View reviewed changes

cmd/kubeadm/app/apis/kubeadm/validation/validation.go Show resolved Hide resolved

enj moved this to Needs Triage in SIG Auth Jun 26, 2025

Refactor: Use field.Required for missing field errors

424bd82

xiaoweim force-pushed the validation-cleanup-required-conversion branch from 005f779 to 424bd82 Compare June 29, 2025 21:32

thockin reviewed Jul 1, 2025

View reviewed changes

thockin self-assigned this Jul 1, 2025

Refactor: Use field.Required for missing field errors #132536

Are you sure you want to change the base?

Refactor: Use field.Required for missing field errors #132536

Conversation

xiaoweim commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

xiaoweim commented Jun 25, 2025

Uh oh!

xiaoweim commented Jun 25, 2025

Uh oh!

k8s-triage-robot commented Jun 25, 2025

Uh oh!

aaron-prindle commented Jun 25, 2025

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

xiaoweim commented Jun 30, 2025

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaoweim commented Jun 25, 2025 •

edited

Loading