New Linter: markerscope | KAL fails to detect minproperties validation.

[nayuta723]

The kube-api-linter tool successfully lints a Kubernetes API definition even when the +kubebuilder:validation:MinProperties marker is incorrectly applied to a field that is not an object type (a struct). This leads to a subsequent failure when controller-gen is run, as it correctly enforces this constraint.

How to reproduce

1. Create a Kubernetes API definition with a MinProperties marker, like the following example:

   type FooSpec struct {
     // foo is a foo.
     // +optional
     // +kubebuilder:validation:MinProperties=1
     Foo Foo `json:"foo,omitzero"`
   }
   
   type Foo struct {
     // bar is a bar.
     // +optional
     // +kubebuilder:validation:MinLength=1
     // +kubebuilder:validation:MaxLength=10
     Bar string `json:"bar,omitempty"`
   }

2. Run kube-api-linter against this file.
   The linter will complete successfully without any warnings or errors.

3. Run controller-gen against this file.
   controller-gen will fail with an error similar to: must apply minproperties to an object.

Proposed Solution
I believe a new linter rule should be added that specifically validates the correct usage of +kubebuilder:validation:MinProperties on object types, ensuring that its application does not contradict other API conventions or cause controller-gen to fail. This would make the linter a more effective tool for API development.

[JoelSpeed]

Yep, this seems like a reasonable request to me. We don't currently have any sense of the scope of a paritcular marker, so a new markerscope rule is probably worth implementing that allows us to configure the linter to check not only the scope of the MinProperties marker, but also other markers to ensure they are applied in the correct place.

As another example, you cannot have an optional/required object, only a field, so this would be the inverse of the minproperties in terms of its scope requirements.

[JoelSpeed]

/help
/kind feature

[k8s-ci-robot]

@JoelSpeed:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

Details

In response to this:

/help
/kind feature

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.

[nayuta723]

I'm interested in this. Would it be okay for me to work on it?

[nayuta723]

I'd like to confirm the requirements for this linter.

• All markers must have a scope defined.
  • The scope should be based on controller-gen marker packages.
  • MaxProperties and MinProperties have special considerations because their handling of Object types is strict.
    • https://github.com/kubernetes-sigs/controller-tools/blob/main/pkg/crd/markers/validation.go#L504-L520
• Any verification outside of scope is out of scope.

The following points are still undecided:

• Should we implement a SuggestFix feature?
• Should users be able to select which markers to apply?

Regarding the SuggestFix feature, its implementation seems relatively straightforward. If a FieldScope marker were defined on a TypeScope, the fix would simply be a matter of moving that marker to the FieldScope. Because the change is minimal, I believe it would be a beneficial improvement for users.
For the linter rules, I think it would be better to apply all markers by default and use an exclude rule to filter them out.

Please provide your comments on the above.

[sbueringer]

I wonder if controller-gen should be extended to support MinProperties/MaxProperties on fields (that have a struct type)

[JoelSpeed]

I'll admit, I was slightly surprised that this doesn't work, I can't see a good reason not to support it on fields. Though the markerscope idea I talked about I think is valid for other use cases, so useful even if we don't need it for this specific use case.

[sbueringer]

Agree. The markerscope linter is useful independent of this specific case.

[nayuta723]

If you'd entrust the MarkerScope linter to me, I'd like to clarify the specification. Could you please answer the questions?

[JoelSpeed]

All markers must have a scope defined.

• The scope should be based on controller-gen marker packages.
• MaxProperties and MinProperties have special considerations because their handling of Object types is strict.
  https://github.com/kubernetes-sigs/controller-tools/blob/main/pkg/crd/markers/validation.go#L504-L520

Yes, this is correct, though look at other markers too, for example the MaxItems only applies to arrays, we should report if this marker is applied to an object

I would hope that we can build an internal list of markers we are aware of, and come up with a way to represent where they are allowed:

• On field vs on type
• For arrays, or strings, or objects

The following points are still undecided:

• Should we implement a SuggestFix feature?
• Should users be able to select which markers to apply?

Yes, I think a suggest fix makes sense to remove the markers when they are applied in the wrong location. Folks who find this annoying (I lost the detail because it was in the wrong place), should be able to turn this off (Warn)

I think we should also have the ability for users to include their own custom markers and describe the scope of those markers within the config. But we should include defaults based on what's in Kubebuilder and the declarative validation project already

[nayuta723]

/assign