[feat] Add k8s-resource-fully-qualified-name format #134602

lalitc375 · 2025-10-14T21:05:41Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This PR introduces the k8s-resource-fully-qualified-name format for declarative validation. This new format is used to validate the MatchAttribute field in DeviceConstraint, ensuring that it is a valid fully qualified name.

This change also corrects the error type for an empty domain or name in a qualified name from Required to Invalid. This provides a more accurate error message to the user.

Which issue(s) this PR is related to:

KEP: kubernetes/enhancements#5073

Special notes for your reviewer:

The error type for an empty domain or name in validateQualifiedName has been changed from field.Required to field.Invalid.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/5073

k8s-ci-robot · 2025-10-14T21:05:50Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-triage-robot · 2025-10-14T21:58:46Z

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.

lalitc375 · 2025-10-21T18:39:20Z

/assign @aaron-prindle @jpbetz

aaron-prindle · 2025-10-28T17:58:05Z

/retest

staging/src/k8s.io/apimachinery/pkg/api/validate/strfmt.go

thockin · 2025-10-28T18:58:18Z

staging/src/k8s.io/apimachinery/pkg/api/validate/strfmt.go

+	var allErrs field.ErrorList
+	s := string(*value)
+	if len(s) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath, "name required"))


The last arg to Required() is almost never needed , and it's not clear that "required" is the right framing. What do I mean?

The field which invokes this is either required or optional. By the time you get here, requiredness should already be be asserted. Empty required values would be rejected and empty optional values would never call here. So if you are here it's either because there is a non-empty value or someone forgot to assert requiredness.

I think I would, instead, do the split and if the name is empty, let it fail the validateCIdentifier() or LongName() calls

This was done to closely mirror the handwritten validation code errors. I think we can rewrite hand validation code in the same way.

Yeah, lots of handwritten code is sloppy because it is used in exactly one place. This is our chance to clean up some of it

thockin · 2025-10-28T19:00:22Z

staging/src/k8s.io/apimachinery/pkg/api/validate/strfmt.go

+	}
+	var allErrs field.ErrorList
+	if len(*value) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath, "name can't be empty"))


Same comment as above.

Also it's odd how these chain -- you validate length here, then resourcesQualifiedName() validates it again

thockin · 2025-10-28T19:01:07Z

staging/src/k8s.io/apimachinery/pkg/api/validate/strfmt.go

+	}
+
+	s := string(*value)
+	allErrs = append(allErrs, resourcesQualifiedName(ctx, op, fldPath, value, nil)...)


why not pass s instead of value -- potentially save on template expansions and is more obvious.

This need to pointer, Replaced it with &s. both should have same impact?

thockin · 2025-10-28T19:03:18Z

pkg/apis/resource/validation/validation.go

 		}
+		// TODO: This validation is incomplete. It should reject qualified names
+		// that contain more than one slash. Currently, names like "a/b/c" are not
+		// handled and are implicitly accepted.


Worse - they seem to be accepted regardless of what value they hold. I know we want to avoid side-quests, but this seems like something we should fix fairly aggressively.

thockin

Any test which uses Matcher and Origin should probably NOT be embedding detail strings. I'd like to fix that error string and you're just adding places for me to go fix it.

thockin · 2025-10-28T22:58:03Z

staging/src/k8s.io/apimachinery/pkg/api/validate/strfmt_test.go

+		name:  "invalid name with dots",
+		input: "prefix.com/name.with.dots",
+		wantErrs: field.ErrorList{
+			field.Invalid(fldPath, "name.with.dots", "a valid C identifier must start with alphabetic character or '_', followed by a string of alphanumeric characters or '_' (e.g. 'my_name',  or 'MY_NAME',  or 'MyName', regex used for validation is '[A-Za-z_][A-Za-z0-9_]*')").WithOrigin("format=k8s-resource-fully-qualified-name"),


The Matcher doesn't need the detail string, just the origin. The fact that these giant error strings get copied into a million tests is a huge part of why we have origins in the first place,

Or at least pick a small substring that gives the confidence that the right thing went wrong

Or at least pick a small substring that gives the confidence that the right thing went wrong

I have reduced the string size. This helps in figuring the validation is not circuited before expected blocks.

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2025-10-29T22:47:57Z

LGTM label has been added.

Git tree hash: ca08a4dbe8b89f466fc149c06091a75ef54b0e1d

k8s-ci-robot · 2025-10-29T22:48:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, lalitc375, thockin

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/OWNERS~~ [thockin]
~~pkg/registry/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [jpbetz,thockin]
~~staging/src/k8s.io/apimachinery/pkg/util/validation/OWNERS~~ [thockin]
~~staging/src/k8s.io/code-generator/OWNERS~~ [jpbetz,thockin]

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

github-project-automation bot added this to Dynamic Resource Allocation Oct 14, 2025

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

k8s-ci-robot requested review from bart0sh and deads2k October 14, 2025 21:06

lalitc375 force-pushed the device-attirubute branch 2 times, most recently from f685293 to 1bea5c7 Compare October 14, 2025 21:13

lalitc375 changed the title ~~Device attirubute~~ [feat] Add k8s-resource-fully-qualified-name format Oct 14, 2025

lalitc375 force-pushed the device-attirubute branch from 1bea5c7 to 87dc870 Compare October 17, 2025 20:19

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2025

pohly moved this from 🆕 New to ✅ Done in Dynamic Resource Allocation Oct 21, 2025

lalitc375 force-pushed the device-attirubute branch from 87dc870 to 148436e Compare October 21, 2025 18:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2025

k8s-ci-robot assigned aaron-prindle and jpbetz Oct 21, 2025

lalitc375 force-pushed the device-attirubute branch from 148436e to 743b3c4 Compare October 21, 2025 20:24

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 27, 2025

k8s-ci-robot requested review from jpbetz and thockin October 27, 2025 17:29

lalitc375 force-pushed the device-attirubute branch 2 times, most recently from fa056fe to 185bdd8 Compare October 27, 2025 18:14

thockin reviewed Oct 28, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2025

lalitc375 force-pushed the device-attirubute branch 2 times, most recently from df039c3 to 4a7d759 Compare October 28, 2025 21:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2025

thockin reviewed Oct 28, 2025

View reviewed changes

lalitc375 force-pushed the device-attirubute branch from 4a7d759 to 1029044 Compare October 28, 2025 23:27

lalitc375 added 4 commits October 29, 2025 04:39

Add k8s-resource-fully-qualified-name format in valdiation-gen

2de0389

Use k8s-resource-fully-qualified-name format to validate MatchAttribute

ec8c224

Add new method to validate C Identifier

94101ba

Address feedbacks

37ba7bb

lalitc375 force-pushed the device-attirubute branch from bbab2e1 to 37ba7bb Compare October 29, 2025 04:43

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 29, 2025

thockin reviewed Oct 29, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2025

k8s-ci-robot merged commit de28a5b into kubernetes:master Oct 29, 2025
21 checks passed

k8s-ci-robot added this to the v1.35 milestone Oct 29, 2025

lalitc375 deleted the device-attirubute branch November 3, 2025 17:46

[feat] Add k8s-resource-fully-qualified-name format #134602

[feat] Add k8s-resource-fully-qualified-name format #134602

Uh oh!

Conversation

lalitc375 commented Oct 14, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Oct 14, 2025

Uh oh!

k8s-triage-robot commented Oct 14, 2025

Uh oh!

lalitc375 commented Oct 21, 2025

Uh oh!

aaron-prindle commented Oct 28, 2025

Uh oh!

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!

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!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 29, 2025

Uh oh!

k8s-ci-robot commented Oct 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants