linters: add forbiddenmarkers and nonullable linters #111

everettraven · 2025-06-26T13:48:35Z

Adds a disabled by default forbiddenmarkers linter that has configuration options to specify marker identifiers that should never exist on a type/field. Suggests fixes to remove the forbidden markers.

Adds an enabled by default nonullable linter that wraps the forbiddenmarkers linter and configures the nullable marker to be forbidden.

Fixes #100

k8s-ci-robot · 2025-06-26T13:48:47Z

[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 jpbetz 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

everettraven · 2025-06-26T14:05:56Z

I'll need to add the linter documentation to the new location as well :P

/hold

JoelSpeed · 2025-06-27T09:38:58Z

pkg/analysis/forbiddenmarkers/analyzer.go

+const name = "forbiddenmarkers"
+
+type analyzer struct {
+	forbiddenMarkers []string


What if I wanted to forbid a marker only if it had a certain parameter? Or a certain value?

E.g. I want to prevent someone from using a particular kubebuilder:validation:Format, would this allow me to do that?

Or I wanted to say, you can't use the optionalOldSelf property on an XValidation?

Good questions. This would not allow that for now. I think those are reasonable use cases and make sense to consider.

I can add some configuration options to allow for this type of customization.

JoelSpeed · 2025-06-27T09:43:39Z

pkg/analysis/nonullable/analyzer.go

+	analyzer.Name = name
+	analyzer.Doc = "Check that nullable marker is not present on any types or fields."


Maybe create a constructor on the forbidden markers side that allows you to pass options for Name and Doc, rather than overriding them here? Then this just becomes

return NewAnalyzerWithOpts(forbidden.Opts{ Name: ... Doc: ... Markers: []string{...} })

Or even, you could just call that inside the initializer.

JoelSpeed · 2025-06-27T09:44:57Z

pkg/analysis/nonullable/doc.go

+* The `nonullable` linter ensures that types and fields do not have the `nullable` marker.
+*
+* Fixes are suggested to remove the `nullable` marker.
+*


Why the asterisks at the beginning of each line?

Copied format from another linter doc.go - but also I think my editor does this by default. I can strip the asterisks at the beginning of each line if necessary

Doesn't matter to me, but normally with /* */ syntax I didn't think it was needed

JoelSpeed · 2025-06-27T09:45:25Z

pkg/analysis/nonullable/initializer.go

+// Initializer returns the AnalyzerInitializer for this
+// Analyzer so that it can be added to the registry.
+func Initializer() initializer {
+	return initializer{}
+}
+
+// intializer implements the AnalyzerInitializer interface.
+type initializer struct{}
+
+// Name returns the name of the Analyzer.
+func (initializer) Name() string {
+	return name
+}
+
+// Init returns the intialized Analyzer.
+func (initializer) Init(_ config.LintersConfig) (*analysis.Analyzer, error) {
+	return newAnalyzer(), nil
+}
+
+// Default determines whether this Analyzer is on by default, or not.
+func (initializer) Default() bool {
+	return true
+}


I'm starting to wonder if we should create a util to generate the initializer from a func 🤔

I like that idea, but to keep the scope of this PR tightened to these two linters I'd suggest that be a follow up.

This is now implemented, PR will need a rebase

everettraven · 2025-06-27T19:45:36Z

Working on allowing configuration of forbidden attributes and values.

/hold

Signed-off-by: Bryce Palmer <[email protected]>

everettraven · 2025-07-25T19:56:58Z

This is mostly ready to go, just have to fix linting issues and squash commits.

JoelSpeed · 2025-07-29T14:32:55Z

pkg/analysis/forbiddenmarkers/initializer.go

+		return field.ErrorList{field.Required(fldPath, "configuration is required for the forbiddenmarkers linter when it is enabled")}
+	}
+
+	fieldErrors := field.ErrorList{}


Implementation of validation missing?

JoelSpeed · 2025-07-29T14:34:00Z

pkg/analysis/forbiddenmarkers/initializer.go

+}
+
+func validateMarkers(fldPath *field.Path, markers ...Marker) field.ErrorList {
+	childPath := fldPath.Child("markers")


Normally the child element is called by the parent, rather than here

JoelSpeed · 2025-07-29T14:41:30Z

pkg/analysis/forbiddenmarkers/initializer.go

+
+	for i, marker := range markers {
+		indexPath := childPath.Index(i)
+		if knownMarkers.Has(marker.Identifier) {


What if I wanted to say identifier foo is allow, unless it has attribute A and B, or A and C? Are we sure the identifiers must be unique? Or should they be unique only when the attributes match?

For example, what if I wanted to forbid Blue Apples, or Green Oranges, but allow Red Apples and Orange Oranges? Would that be configurable today?

Good question. I'd have to think through potential scenarios a bit further,~~but giving this a quick analysis~~.

In this example we essentially want the markers +custom:marker:fruit=apple,color=blue and +custom:marker:fruit=orange,color=green to be invalid. I think boils down to a configuration of:

markers: - identifier: "custom:marker" attributes: # entries join as an AND operation - name: fruit values: # OR operation - apple - orange - name: color values: # OR operation - blue - green

This basically becomes a boolean conditional chain of (fruit == apple || fruit == orange) && (color == blue || color == green) which means that it would successfully work for the exact constraints of your example, but would also forbid Green Apples and Blue Oranges.

I could imagine a case where you may want to forbid something like Blue Apples and Green Oranges but still want to allow something like Green Apples as valid.

Maybe doing something like a "rule set" that does an OR (basically a configured set of the existing ANDed attribute rules) would allow for that behavior while keeping all configuration under a unique identifier.

For example, to handle the case of forbidding Blue Apples and Green Oranges, but allowing Green Apples you might be able to do something like:

markers: - identifier: "custom:marker" ruleSets: # OR operation - attributes: # entries join as an AND operation - name: fruit values: # OR operation - apple - name: color values: # OR operation - blue - attributes: # entries join as an AND operation - name: fruit values: # OR operation - orange - name: color values: # OR operation - green

Which would essentially become a conditional of (fruit == apple && color == blue) || (fruit == orange && color == green) which would work.

Yeah I think the ruleset idea could work, shall we give that a go?

Yeah, I'll move this in that direction. I probably won't have time to do this until later this week though

k8s-ci-robot requested a review from JoelSpeed June 26, 2025 13:48

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2025

k8s-ci-robot requested a review from jpbetz June 26, 2025 13:48

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 26, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025

JoelSpeed reviewed Jun 27, 2025

View reviewed changes

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 Jun 27, 2025

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

JoelSpeed mentioned this pull request Jul 17, 2025

feat: add Conflictingmarkers #126

Merged

everettraven force-pushed the linter/forbiddenmarkers branch from 36bf9d5 to 5d01549 Compare July 25, 2025 15:40

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

everettraven added 4 commits July 25, 2025 11:41

linters: add forbiddenmarkers and nonullable linters

e60d9c6

Signed-off-by: Bryce Palmer <[email protected]>

fixup! linter issues

c2cd2ba

Signed-off-by: Bryce Palmer <[email protected]>

wip: support for attribute and value constraints

1d4846e

Signed-off-by: Bryce Palmer <[email protected]>

fixup! finalize implementation

4652204

Signed-off-by: Bryce Palmer <[email protected]>

everettraven force-pushed the linter/forbiddenmarkers branch from 5d01549 to 4652204 Compare July 25, 2025 19:55

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 25, 2025

JoelSpeed reviewed Jul 29, 2025

View reviewed changes

		analyzer.Name = name
		analyzer.Doc = "Check that nullable marker is not present on any types or fields."

linters: add forbiddenmarkers and nonullable linters #111

Are you sure you want to change the base?

linters: add forbiddenmarkers and nonullable linters #111

Conversation

everettraven commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

everettraven commented Jun 26, 2025

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 Jun 27, 2025

Uh oh!

everettraven commented Jul 25, 2025

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 Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

everettraven Jul 29, 2025 •

edited

Loading