[DRAFT] Add kubeapi linter #3905
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
@rikatz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| - "nophase" # Phase fields are discouraged by the Kube API conventions, use conditions instead. | ||
| - "optionalfields" # Ensure that all fields marked as optional adhere to being pointers and | ||
| # having the `omitempty` value in their `json` tag where appropriate. | ||
| - "optionalorrequired" # Every field should be marked as `+optional` or `+required`. |
There was a problem hiding this comment.
I think I disagree with this one, or at the very least would rather start with this off. I'd always treated "required" as default and "optional" as the only thing that needed to be clearly specified.
There was a problem hiding this comment.
For some background on why this is important
Recent updates to the kubernetes API conventions have requested new fields added are either optional or required explicitly.
We've also come to realise that previous advice around omitempty was incorrect, and that all fields, no matter required or optional, should have omitempty to create consistent required/optional behaviour between fields.
This creates an issue for previous tooling that assumes that anything with omitempty, if it isn't tagged explicitly, is optional.
To quote the paragraph:
Note that for backward compatibility, any field that has the omitempty struct tag, and is not explicitly marked as +required, will be considered to be optional. This is expected to change in the future, and new fields should explicitly set either an +optional or +required comment tag.
It is also very easy to add a package level optional marker (you are using Kubebuilder markers?) to switch the default from optional to required, which can then go unnoticed in future reviews, leaving folks creating APIs where they think a field is required, but actually, it is optional.
There are also going to be other rules (optionalfields/requiredfields) that help to get serialization right, that depend on being marked explicitly, so you wouldn't be able to benefit from these if you chose not to explicitly mark required fields.
| - "duplicatemarkers" # Ensure there are no exact duplicate markers. for types and fields. | ||
| - "integers" # Ensure only int32 and int64 are used for integers. | ||
| - "jsontags" # Ensure every field has a json tag. | ||
| - "maxlength" # Ensure all strings and arrays have maximum lengths/maximum items. |
There was a problem hiding this comment.
This one is not particularly useful when there's already a corresponding regex that ensures a max length (the case with our duration types).
There was a problem hiding this comment.
Do you have an example of a field?
One of the key reasons for setting upper bounds on fields is to allow accurate cost estimates for CEL rules. CEL has a number of exceptions within it where it knows the correct maximum length (e.g. it can determine from a list of enums what the maximum length is) and the linter implements the same exceptions (so it won't ask for maxlength on an enum).
For regexes, I don't think CEL would have a way to know (assuming you're using pattern) what the maximum length is, and as such, to enable future CEL rules on the field, it is best practice to set a maximum length, even if that duplicates something already in the regex.
Note also that while it's not implemented yet, a minimumlength rule is also planned. For example, most required string fields would not expect "" to be a valid value, and as such should be setting a minimum length to prevent this. If you have any thoughts regaring this or other rules I'm all ears, trying to gather feedback as much as possible at the moment and make this something that is really useful for the community.
| lintersConfig: | ||
| conditions: | ||
| isFirstField: Warn # Require conditions to be the first field in the status struct. | ||
| usePatchStrategy: Forbid # Require conditions to be the first field in the status struct. |
There was a problem hiding this comment.
The comment on this one isn't right, this one is about the patch markers which don't work for CRDs
| destination: ./bin | ||
| plugins: | ||
| - module: 'sigs.k8s.io/kube-api-linter' | ||
| version: 'main' |
There was a problem hiding this comment.
I would recommend to pin a specific commit. While we are still in the "early days" we haven't yet committed to producing actual releases, and I'm concerned that if you track main, we might break you accidentally.
In paritcular, I know that we need to rework how requiredfields works after recent discussion with upstream API reviewers, so that one will probably be surprising if it suddenly starts reporting issues
| - "integers" # Ensure only int32 and int64 are used for integers. | ||
| - "jsontags" # Ensure every field has a json tag. | ||
| - "maxlength" # Ensure all strings and arrays have maximum lengths/maximum items. | ||
| - "nobools" # Bools do not evolve over time, should use enums instead. |
There was a problem hiding this comment.
@JoelSpeed would this preclude the flat boolean we're considering using for CORS here?
What type of PR is this?
/kind test
What this PR does / why we need it: This PR is an attempt to add kube-api-linter as part of gw-api pipelines