feat: add Conflictingmarkers #126

yongruilin · 2025-07-16T05:50:44Z

Ref: #97

Adds a new configurable linter that detects mutually exclusive markers. The linter allows users to define custom conflict sets and is not enabled by default.

k8s-ci-robot · 2025-07-16T05:50:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yongruilin
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

JoelSpeed

Thanks for raising this, looking mostly there but found a few things I think we can improve, mainly around documentation and how we output the messages

JoelSpeed · 2025-07-16T07:58:27Z

docs/linters.md

+   - Set A: `+default`, `+kubebuilder:default`
+   - Set B: `+required`, `+kubebuilder:validation:Required`


Is there a k8s:default marker confirmed yet? Or even, are we expecting to create one?

Not for now, but we do expect to create one. No harm to add it here.

JoelSpeed · 2025-07-16T07:59:35Z

docs/linters.md

+The `conflictingmarkers` linter detects and reports when mutually exclusive markers are used on the same field.
+This prevents common configuration errors and unexpected behavior in Kubernetes API types.
+
+The linter includes built-in checks for the following conflicts:


Are there any other marker combinations that we want to include in this initial phase?

Not that I'm aware of.

JoelSpeed · 2025-07-16T08:00:34Z

docs/linters.md

+### Configuration
+
+The linter is configurable and allows users to define their own custom sets of conflicting markers.
+Each custom conflict set must specify a name, two sets of markers, and a description of why they conflict.


I'm guessing the description will be used in the linter output when it detects these conflicts?

Yes. It is being used in the report.

JoelSpeed · 2025-07-16T08:08:21Z

pkg/analysis/conflictingmarkers/analyzer.go

+func defaultConflictSets() []ConflictSet {
+	return []ConflictSet{
+		{
+			Name:        "optional_vs_required",


Is this used at all? I can't see it being used, should it be in the message, perhaps before the value lists

I think description should provide sufficient info? The name is used for linter author to track the sets easier.

I don't actually see the name being used anywhere apart from it being checked that it is unique?

It is used when validating the config but not used when reporting.

JoelSpeed · 2025-07-16T08:10:07Z

pkg/analysis/conflictingmarkers/analyzer_test.go

+	config := &conflictingmarkers.ConflictingMarkersConfig{
+		CustomConflicts: []conflictingmarkers.ConflictSet{
+			{
+				Name:        "custom_conflict",


I wonder if there is ever a case for a conflicting marker with attributes? For example XValidation in Kubebuilder has multiple properties, could you create conflicts with something like that? Are there any markers in the declarative validation project that have attributes like XValidation does?

Hmmm, declarative validation parses tags differently using "arguments" . I think keeping conflicting set focus on marker id is simpler.

pkg/analysis/conflictingmarkers/config.go

JoelSpeed · 2025-07-16T08:11:25Z

pkg/analysis/conflictingmarkers/config.go

+type ConflictSet struct {
+	// Name is a human-readable name for this conflict set.
+	Name string `json:"name"`
+	// SetA contains the first set of conflicting markers.


Need to make this description obvious that it is A conflicts with B and not conflicts within setA

Updated the comment.

pkg/analysis/conflictingmarkers/doc.go

pkg/analysis/conflictingmarkers/initializer.go

JoelSpeed · 2025-07-16T08:17:41Z

pkg/analysis/conflictingmarkers/testdata/src/b/b.go

@@ -0,0 +1,30 @@
+package b
+
+type CustomConflictStruct struct {


Include a case from the other tests to make sure that enabling custom markers doesn't disable the built in markers

Added one more test case.

JoelSpeed · 2025-07-17T15:53:18Z

pkg/analysis/conflictingmarkers/initializer_test.go

+			Entry("With missing name", testCase{
+				config: conflictingmarkers.ConflictingMarkersConfig{
+					CustomConflicts: []conflictingmarkers.ConflictSet{
+						{
+							Name:        "",
+							SetA:        []string{"marker1"},
+							SetB:        []string{"marker2"},
+							Description: "Test conflict",
+						},
+					},
+				},
+				expectedErr: "conflictingmarkers.customConflicts[0].name: Required value: name is required",
+			}),


Please add a test for missing description as well

Is the one on line91 you looking for?

Yes, I can't read 😅

pkg/analysis/conflictingmarkers/analyzer.go

JoelSpeed · 2025-07-17T16:49:09Z

Had a chat with @everettraven about this linter and a pattern we were looking at in #111. I think we should look to implement the same pattern here.

The pattern would be to create a generic base linter that has no configuration. Users could then configure this with their own config of conflicting markers should they so desire.

Then, for the rules we know we want, we create instances of the base marker with pre-defined configuration.

This means users would see defaultorrequired as an option as a linter, but under the hood, it's just this linter

I think in terms of implementation, the defaultorrequired linter would need to just init the current linter.

func Initializer() initializer.AnalyzerInitializer {
  return initializer.NewInitializer(
    "defaultorrequired",
    func(_ any) (*analysis.Analyzer, error) {
      return conflictingmarkers.Initializer().Init(&conflictingmarkers.ConflictingMarkersConfig{
        // Insert the correct config here
      })
    },
    true,
  )
}

This means that, should a user desire to have their own custom conflicting markers without the default set of conflicts we've identified, they can do so, and they can choose explicitly which of the conflicts they do and don't care about.

yongruilin · 2025-07-17T18:49:44Z

This means that, should a user desire to have their own custom conflicting markers without the default set of conflicts we've identified, they can do so, and they can choose explicitly which of the conflicts they do and don't care about.

It is a good idea. Then I think it is make sense to remove a built-in conflict sets to make it fully flexible?

everettraven · 2025-07-17T19:52:07Z

Then I think it is make sense to remove a built-in conflict sets to make it fully flexible?

I might not have enough of the context to fully grok this question, so feel free to ignore if I've misinterpretted.

My understanding is that we do still want to have some opinionated built-in conflict identification, but these should be distinct linters that can be enabled/disabled without also having to enable/disable the custom conflict rules.

I would expect any conflicts that we identify from reading the Kube API conventions, declarative validation, and controller-gen markers we have a linter (or multiple) for so users don't have to connect those dots themselves in a set of custom conflict rules.

yongruilin · 2025-07-17T21:35:01Z

I pushed a new commit to add an option to disable the built-in conflicts.

JoelSpeed · 2025-07-18T13:09:20Z

@yongruilin I took your branch and pushed a couple of commits on a branch on my fork, this is kind of what I was imagining in my comment, WDYT of https://github.com/kubernetes-sigs/kube-api-linter/compare/main...JoelSpeed:kube-api-linter:pr/yongruilin/126?expand=1

@everettraven please also check this aligns with what you were expecting

yongruilin · 2025-07-18T21:35:10Z

@JoelSpeed thanks for composing this.

Basically we have 2 ways to proceed on conflictingmarkers

No built-in, conflictSets can only be configured and disabled by default. (Your commits)
Built-in with customized conflict sets and enabled by default. (Current implementation)

For 1, is it worth the effort to introduce new linters? Keeping common conflict cases as built-in looks simpler to me.

everettraven · 2025-07-22T14:10:49Z

@yongruilin I took your branch and pushed a couple of commits on a branch on my fork, this is kind of what I was imagining in my comment, WDYT of https://github.com/kubernetes-sigs/kube-api-linter/compare/main...JoelSpeed:kube-api-linter:pr/yongruilin/126?expand=1

@everettraven please also check this aligns with what you were expecting

This does align with what I was expecting.

Basically we have 2 ways to proceed on conflictingmarkers

No built-in, conflictSets can only be configured and disabled by default. (Your commits)

Built-in with customized conflict sets and enabled by default. (Current implementation)

For 1, is it worth the effort to introduce new linters? Keeping common conflict cases as built-in looks simpler to me.

I would agree that keeping common conflict cases would be simpler in implementation and maintenance burden on maintainers, but I think it causes UX limitations that are not ideal.

Adding new conflicting sets for option 2 would also likely be considered a breaking change as it could be more restrictive. We would have to solve for how we add new built-in conflict rules as opt-in. I don't think option 1 has this same problem because we can always add new non-default linters that users can opt-in to use - we don't have to reinvent the wheel to solve that issue.

For example, consider the following use cases:

I want to enable my own custom conflicting cases and do not want to use any of the defaults
- Using option 1, they can enable the conflictingmarkers linter with their custom configuration and intuitively disable all of the opinionated default linters we have that builds on the conflictingmarkers implementation.
- Using option 2, they cannot disable the built-ins without a new configuration option to do so (and IMO this isn't as intuitive as disabling separate linter(s)). Without the additional configuration option they end up having to disable all of their custom conflict rules as well.
I want to enable my own custom conflicting cases and only want to use a subset of the defaults
- Option 1 facilitates this well
- Option 2, the configuration option has now gotten even more complex and more unintuitive.

IMO we should prefer UX > maintainer burden where possible.

JoelSpeed · 2025-07-22T14:31:32Z

I can't really summarise my thoughts any better than @everettraven has, but yes, my priority here is not for maintainer burden (though I am trying hard to make that easy), but for the end user UX and making it easy to configure.

I think the points about how would we possibly add new conflicts to the built in markers without being breaking is also an important point that weighs into the decision here.

I'm very keen to go for option 1 at the moment and see if that decision ages well over time

yongruilin · 2025-07-22T22:04:03Z

Ok, I agree with this approach. I've already updated to be fully configurable with no built-in conflict sets, which fits with the generic base linter concept.
I'd like to split this into two PRs though - the current one for the underlying generic linter, and a follow-up for the opinionated wrapper linters (like defaultorrequired) that use the base linter with pre-defined configs. This should make the review process cleaner.

sivchari · 2025-07-23T05:58:11Z

docs/linters.md

+        setA:
+          - "default"
+          - "+kubebuilder:default"
+        setB:
+          - "required"
+          - "+kubebuilder:validation:Required"
+          - "+k8s:validation:required"


I think setA and setB is ambiguous. For example, how about first / second ? If this is it, when we extend set, it's easy to add like third.

I was also thinking about the naming and couldn't come up with something I was absolutely happy with, this is a hard case to name 🤔

I could see first and second working, but does that imply some sort of ordering? Where I don't think there actually is any ordering?

What if we just made it:

conflicts: - name: "conflict_one" sets: - ["default", "kubebuilder:default"] - ["required", "kubebuilder:Required"] - ...

We could, for now, limit sets to just two entries? Also, do we ever envision needing to extend this beyond 2 sets for a single conflict rule? How would/should that be handled?

Another naming option could be left and right? No ordering is signaled there and is something that I've seen used to represent comparison operations in the past.

Good idea, we actually allow multiple sets in validation-gen's lint. We should handle it that all sets are conflicting with each other.

This would open up the case for something that correctly allows you to check that you only have on of [MinItems,MaxItems], [MinProperties,MaxProperties], [MinLength,MaxLength] since we know that these are for different resources types (array, map/object, string)

@everettraven
Good idea, totally agree it.

sivchari · 2025-07-23T06:01:24Z

pkg/analysis/conflictingmarkers/initializer_test.go

+			if len(in.expectedErr) > 0 {
+				Expect(errs.ToAggregate()).To(MatchError(in.expectedErr))
+			} else {
+				Expect(errs).To(HaveLen(0), "No errors were expected")
+			}


Suggested change

if len(in.expectedErr) > 0 {

Expect(errs.ToAggregate()).To(MatchError(in.expectedErr))

} else {

Expect(errs).To(HaveLen(0), "No errors were expected")

}

if len(in.expectedErr) > 0 {

Expect(errs.ToAggregate()).To(MatchError(in.expectedErr))

return

}

Expect(errs).To(HaveLen(0), "No errors were expected")

It's easy to read.

sivchari · 2025-07-23T06:03:16Z

pkg/analysis/conflictingmarkers/testdata/src/b/b.go

Please add test case that has +marker1 and +marker2. It must be valid.

JoelSpeed · 2025-07-23T10:41:49Z

docs/linters.md

+        setA:
+          - "default"
+          - "+kubebuilder:default"
+        setB:
+          - "required"
+          - "+kubebuilder:validation:Required"
+          - "+k8s:validation:required"


I was also thinking about the naming and couldn't come up with something I was absolutely happy with, this is a hard case to name 🤔

I could see first and second working, but does that imply some sort of ordering? Where I don't think there actually is any ordering?

JoelSpeed · 2025-07-23T10:43:51Z

pkg/analysis/conflictingmarkers/initializer.go

+}
+
+// validateConfig validates the configuration in the config.ConflictingMarkersConfig struct.
+func validateConfig(cfg *ConflictingMarkersConfig, fldPath *field.Path) field.ErrorList {


We should probably validate that there is at least one item in the conflict set shouldn't we? Adding this with no config doesn't make sense

everettraven · 2025-07-23T13:45:27Z

pkg/analysis/conflictingmarkers/analyzer.go

+		Name:     name,
+		Doc:      "Check that fields do not have conflicting markers from mutually exclusive sets",


Nit: make this optionally configurable so we can easily change this in future wrappers of this analyzer

everettraven · 2025-07-23T13:46:20Z

pkg/analysis/conflictingmarkers/analyzer.go

+	conflictSets []ConflictSet
+}
+
+func newAnalyzer(cfg *ConflictingMarkersConfig) *analysis.Analyzer {


Export this so other distinct linters can wrap this in the future?

In the example I provided, I wrapped the Initializer function rather than this function, I don't think this needs to be exported does it?

Ah, I see.

What does the output of that linter look like if the initializer uses one name but the analyzer that is returned uses a different name?

In the example you provided, I suspect that the initializer properly returns that it is for the defaultorrequired linter, but the analysis.Analyzer returned would have the name conflictingmarkers. If this is true and both names (or just the analysis.Analyzer name) manifest in the output, it might be confusing to users that have disabled the conflictingmarkers linter but see that name pop up if they have the defaultorrequired linter enabled.

AFAICT the name is only used on the outer layer, we see the name of the Initializer in the linter line before the message, but, because we run the initialization and then wrap the initialized Analyzer, we override the name of the package output Initializer on the new implementation, so, I believe this is safe

pkg/analysis/conflictingmarkers/config.go

everettraven · 2025-07-23T13:52:24Z

pkg/analysis/conflictingmarkers/initializer.go

+
+	for i, conflictSet := range cfg.Conflicts {
+		if nameSet.Has(conflictSet.Name) {
+			fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("conflicts").Index(i).Child("name"), conflictSet.Name, "repeated value, names must be unique"))


The field.Duplicate() function may be more appropriate here.

Good catch, updated.

pkg/analysis/conflictingmarkers/initializer_test.go

pkg/analysis/conflictingmarkers/testdata/src/a/a.go.golden

pkg/analysis/conflictingmarkers/testdata/src/b/b.go.golden

…igurations

…er details

JoelSpeed · 2025-07-25T07:27:00Z

docs/linters.md

@@ -17,6 +18,7 @@
 - [StatusOptional](#statusoptional) - Ensures status fields are marked as optional
 - [StatusSubresource](#statussubresource) - Validates status subresource configuration
 - [UniqueMarkers](#uniquemarkers) - Ensures unique marker definitions
+- [ConflictingMarkers](#conflictingmarkers) - Detects and reports when mutually exclusive markers are used on the same field


This entry is now in the list twice, and it appears we have the documentation twice too?

JoelSpeed · 2025-07-25T07:28:58Z

pkg/analysis/conflictingmarkers/analyzer.go

+	// Use configured documentation or fall back to default
+	doc := cfg.Doc
+	if doc == "" {
+		doc = "Check that fields do not have conflicting markers from mutually exclusive sets"
+	}


Oh interesting, hadn't thought about this, when we wrap the analyzer this makes sense doesn't it

JoelSpeed · 2025-07-25T07:33:54Z

LGTM one we've fixed that docs issue of seeing double, thanks for working on this one @yongruilin

everettraven · 2025-07-25T15:18:39Z

pkg/analysis/conflictingmarkers/config.go

+	// Doc is the documentation string for the analyzer.
+	// If not provided, a default description will be used.
+	Doc string `json:"doc"`


Is this leaking implementation details into user-facing configuration options?

For example, as an end user I would now be able to configure this in a way that the conflicting markers analyzer uses a custom doc string.

I would generally think this is something that we don't want to allow users to change in configuration.

Where does this doc string actually show up? Does it matter if analyzers wrapping this one don't have the option to change it?

I believe it would show under the output of golangci-lint help linters. I do not know if golangci-lint picks up the analyzers from the custom linter config though

everettraven · 2025-07-25T15:25:48Z

pkg/analysis/conflictingmarkers/testdata/src/a/a.go

Can we add a test for the multi-way conflict that ensures we identify a conflict when only a subset of the conflicts are triggered?

In this case I would expect to see a test where there is conflicting markers from 2 sets in the 3-way conflict set still trigger the 3-way conflict.

k8s-ci-robot requested a review from JoelSpeed July 16, 2025 05:50

k8s-ci-robot requested a review from jpbetz July 16, 2025 05:50

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 16, 2025

yongruilin changed the title ~~feat: add Conflictmarkers~~ feat: add Conflictingmarkers Jul 16, 2025

JoelSpeed reviewed Jul 16, 2025

View reviewed changes

yongruilin force-pushed the conflictmarkers branch from a1482d3 to 13da92b Compare July 16, 2025 23:34

JoelSpeed reviewed Jul 17, 2025

View reviewed changes

pkg/analysis/conflictingmarkers/analyzer.go Outdated Show resolved Hide resolved

yongruilin force-pushed the conflictmarkers branch 2 times, most recently from fdabe40 to f1480a6 Compare July 17, 2025 21:34

yongruilin force-pushed the conflictmarkers branch from f1480a6 to 60f7f99 Compare July 22, 2025 21:57

sivchari reviewed Jul 23, 2025

View reviewed changes

JoelSpeed reviewed Jul 23, 2025

View reviewed changes

everettraven reviewed Jul 23, 2025

View reviewed changes

yongruilin force-pushed the conflictmarkers branch from 60f7f99 to 8bdfc41 Compare July 24, 2025 21:05

yongruilin added 4 commits July 24, 2025 21:15

feat: add conflictingmarkers linter to detect mutually exclusive markers

14a91e2

test: add unit tests for conflicting markers analyzer and custom conf…

9be033a

…igurations

docs: update linters documentation to include conflictingmarkers lint…

ebda0ef

…er details

test: add suite and initializer tests for conflicting markers analysis

ed134ff

yongruilin force-pushed the conflictmarkers branch from 8bdfc41 to ed134ff Compare July 24, 2025 21:16

JoelSpeed reviewed Jul 25, 2025

View reviewed changes

everettraven reviewed Jul 25, 2025

View reviewed changes

		- Set A: `+default`, `+kubebuilder:default`
		- Set B: `+required`, `+kubebuilder:validation:Required`

		Name: name,
		Doc: "Check that fields do not have conflicting markers from mutually exclusive sets",

feat: add Conflictingmarkers #126

Are you sure you want to change the base?

feat: add Conflictingmarkers #126

Conversation

yongruilin commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 16, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

JoelSpeed commented Jul 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

everettraven commented Jul 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

JoelSpeed commented Jul 18, 2025

Uh oh!

yongruilin commented Jul 18, 2025

Uh oh!

everettraven commented Jul 22, 2025

Uh oh!

JoelSpeed commented Jul 22, 2025

Uh oh!

yongruilin commented Jul 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

yongruilin commented Jul 16, 2025 •

edited

Loading

yongruilin commented Jul 17, 2025 •

edited

Loading

yongruilin commented Jul 17, 2025 •

edited

Loading

everettraven Jul 23, 2025 •

edited

Loading