(feat): markers: add support for parsing DV tags #152

everettraven · 2025-10-17T19:51:58Z

Kube-api-linter will eventually need to support parsing declarative validation tags for core k/k integration and potentially for controller-tools changes that support DV tag usage on CRDs.

This PR imports the DV tag parsing and adds a special conditional branch to the marker extraction logic to handle DV tags specifically.

The scope of this PR is limited to enabling the parsing of current and future DV tags and does not include functional changes to how individual linters interact with the parsed markers.

For simple DV tags it should be straightforward as to how linter rules handle them.

For the future DV tags where there is conditional behavior it will not be as straightforward. Because these future DV tags are still in the alpha stage we are deferring any special handling considerations to the time in which we need to implement support for them.

k8s-ci-robot · 2025-10-17T19:52:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: everettraven
Once this PR has been reviewed and has the lgtm label, please assign joelspeed 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:

OWNERS

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

JoelSpeed

Do you think it would be possible to chain non-dv tags with DV tags? E.g. if we wanted to put a kubebuilder tag behind a k8s:ifEnabled? Would this parsing support that?

JoelSpeed · 2025-10-29T14:49:55Z

pkg/analysis/helpers/markers/analyzer.go

 }

-func extractGenDeclMarkers(typ *ast.GenDecl, results *markers) {
+func extractGenDeclMarkers(typ *ast.GenDecl, results *markers) error {


This change is breaking linting in other places, PTAL at the linting

Ack, will do.

JoelSpeed · 2025-10-29T14:50:16Z

pkg/analysis/helpers/markers/analyzer.go

-			if marker := extractMarker(comment); marker.Identifier != "" {
+			marker, err := extractMarker(comment)
+			if err != nil {
+				return err


wrap the error with context please

JoelSpeed · 2025-10-29T14:50:38Z

pkg/analysis/helpers/markers/analyzer.go

-		if marker := extractMarker(comment); marker.Identifier != "" {
+		marker, err := extractMarker(comment)
+		if err != nil {
+			return err


Wrap with context please

JoelSpeed · 2025-10-29T14:57:45Z

pkg/analysis/helpers/markers/analyzer.go

+	switch tag.ValueType {
+	case codetags.ValueTypeString, codetags.ValueTypeInt, codetags.ValueTypeBool, codetags.ValueTypeRaw:
+		// all resolvable to an exact string value
+		out.Expressions["payload"] = tag.Value


Wouldn't this normally be "" in our existing marker semantics?

Yes, but DV tags are a bit different in the sense that they potentially have named and unnamed arguments and a "payload".

Thinking about this some more, I'm actually thinking we may want to update how we parse existing kubebuilder markers to either change what we considered "expressions" to be "arguments".

I could see it being worth it to add typing to the markers to signal which marker this was parsed as and a helper utility for reconstruction.

JoelSpeed · 2025-10-29T14:58:05Z

pkg/analysis/helpers/markers/analyzer.go

+	case codetags.ValueTypeNone:
+		// nothing
+	case codetags.ValueTypeTag:
+		// TODO: Better position evaluation


What's wrong with the existing version?

Nothing inherently wrong, but any future logic that may recommend changes to a nested marker may need to take into account that the changes would be on the whole line and not just the subsection where the marker is.

My "TODO" here was with the idea that we might be able to improve a bit here, but thinking about it some more, I'm not quite sure the juice is worth the squeeze - at least not at this moment.

JoelSpeed · 2025-10-29T15:03:22Z

pkg/analysis/helpers/markers/analyzer_test.go

+			expect: func(m Marker) error {
+				expectedID := "required"
+				if m.Identifier != expectedID {
+					return fmt.Errorf("marker identifier %q does not match expected %q", m.Identifier, expectedID)
+				}
+				return nil
+			},


Might be worth writing a helper for this that takes the expected ID as an argument and using that to return the function rather than repeating this many times

JoelSpeed · 2025-10-29T15:09:16Z

pkg/analysis/helpers/markers/analyzer_test.go

+					return fmt.Errorf("identifier %q did not match expected identifier %q", m.Identifier, "k8s:unionMember")
+				}
+
+				union, ok := m.Expressions["union"]


I can't help but feel we are now mixing two different kinds of args (those in payload vs those in the args/parentheses) and that these should be expressed differently, else we can't reliably reconstruct right?

JoelSpeed · 2025-10-29T15:10:13Z

pkg/analysis/helpers/markers/analyzer_test.go

-			shouldBeMarker: true,
-			expectedID:     "k8s:someThing(one: \"a\", two: \"b\")",
+			name:    "declarative validation marker with multiple named arguments",
+			comment: &ast.Comment{Text: "// +k8s:someThing(one: \"a\", two: \"b\")=+k8s:required"},


The payload isn't checked here?

everettraven · 2025-10-30T12:20:28Z

Do you think it would be possible to chain non-dv tags with DV tags? E.g. if we wanted to put a kubebuilder tag behind a k8s:ifEnabled? Would this parsing support that?

Good question. I haven't tested whether something like +k8s:ifEnabled(...)=+kubebuilder:validation:Required would be valid, but I suspect it would.

I'm pretty certain something like +k8s:ifEnabled(...)=+kubebuilder:validation:XValidation:rule="...",message=".." would fail due to it containing a , character and maybe even because of the multiple = characters.

My initial stance is that we don't support the mixing of the two. We should actually add a new linter that flags the cases that would end up passing parsing. I'm not sure carrying the additional complexity of supporting that scenario on either the linter or generative tooling side is worth it there.

everettraven · 2025-10-31T16:59:43Z

pkg/analysis/helpers/markers/analyzer.go

+	if err != nil {
+		return nil
 	}


Instead of returning an error up the chain, I decided staying consistent with the behavior that exists for non-matching markers (no error, just return an empty marker object) is the better path here.

I think malformed dv markers can be added to the set of "typo" detection we do in #23 (either during initial implementation or later).

everettraven · 2025-10-31T17:00:55Z

pkg/analysis/helpers/markers/analyzer.go

+	}
+
+	for _, arg := range tag.Args {
+		out.Arguments[arg.Name] = arg.Value


Positional arguments, of which only one is allowed in a DV tag, the argument name will be "" which aligns with our UnnamedArgument constant.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

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

k8s-ci-robot requested a review from JoelSpeed October 17, 2025 19:52

k8s-ci-robot requested a review from sivchari October 17, 2025 19:52

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2025

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

everettraven force-pushed the feature/dv-tag-parsing branch 2 times, most recently from 4dc9e20 to 375334c Compare October 29, 2025 14:34

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

everettraven changed the title ~~wip: start exploring comprehensive dv tag parsing~~ (feat): markers: add support for parsing DV tags Oct 29, 2025

everettraven marked this pull request as ready for review October 29, 2025 14:40

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 Oct 29, 2025

k8s-ci-robot requested a review from jpbetz October 29, 2025 14:40

JoelSpeed reviewed Oct 29, 2025

View reviewed changes

everettraven force-pushed the feature/dv-tag-parsing branch from 375334c to 952a3de Compare October 31, 2025 16:40

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2025

everettraven commented Oct 31, 2025

View reviewed changes

(feat): markers: add support for parsing declarative validation tags

3e58f96

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven force-pushed the feature/dv-tag-parsing branch from 952a3de to 3e58f96 Compare October 31, 2025 17:04

(feat): markers: add support for parsing DV tags #152

Are you sure you want to change the base?

(feat): markers: add support for parsing DV tags #152

Uh oh!

Conversation

everettraven commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 17, 2025

Uh oh!

JoelSpeed 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!

Choose a reason for hiding this comment

Uh oh!

everettraven commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

everettraven commented Oct 17, 2025 •

edited

Loading

everettraven commented Oct 30, 2025 •

edited

Loading