Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 enable maxlength linter #11906

Merged
merged 2 commits into from
Mar 3, 2025
+6 −2
Merged

🌱 enable maxlength linter #11906

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d319312
enable maxlength linter
sivchari Mar 3, 2025
6d4e7a9
add exclude section for maxlength
sivchari Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .golangci-kal.yml
View file
Original file line number Diff line number Diff line change
@@ -20,6 +20,7 @@ linters-settings:
- "integers" # Ensure only int32 and int64 are used for integers.
- "statussubresource" # All root objects that have a `status` field should have a status subresource.
- "nofloats" # Ensure floats are not used.
- "maxlength" # Ensure all strings and arrays have maximum lengths/maximum items.

# Per discussion in July 2024, we are keeping phase fields for now.
# See https://github.com/kubernetes-sigs/cluster-api/pull/10897#discussion_r1685929508
@@ -29,7 +30,6 @@ linters-settings:
# Linters below this line are disabled, pending conversation on how and when to enable them.
# - "commentstart" # Ensure comments start with the serialized version of the field name.
# - "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.
# - "optionalorrequired" # Every field should be marked as `+optional` or `+required`.
# - "requiredfields" # Required fields should not be pointers, and should not have `omitempty`.
@@ -59,7 +59,7 @@ issues:
max-issues-per-linter: 0
exclude-rules:
# KAL should only run on API folders.
- path-except: "api/*"
- path-except: "api//*"
linters:
- kal
- path: "api/v1beta1/*|api/v1alpha1/*"
@@ -74,3 +74,7 @@ issues:
text: "field Prefix should not use an int, int8 or int16. Use int32 or int64 depending on bounding requirements"
linters:
- kal
- path: "api/v1alpha1/*|api/v1alpha3/*|api/v1beta1/*"
text: "maxlength"
linters:
- kal
Comment on lines +77 to +80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this exclude how many exceptions are there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I met 302 exceptions.

make lint-api | grep 'maxlength' | wc -l
fatal: No names found, cannot describe anything.
INFO golangci-lint has version v1.63.4-custom-gcl built with go1.23.6 from ? on 2025-02-26 11:02:08.494916 +0000 UTC
INFO [config_reader] Used config file .golangci-kal.yml
INFO Loaded : kal
INFO [lintersdb] Active 1 linters: [kal]
INFO [loader] Go packages loading at mode 8767 (compiled_files|files|name|deps|exports_file|imports|types_sizes) took 1.189558333s
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 52.915458ms
INFO [linters_context/goanalysis] analyzers took 0s with no stages
INFO [runner] Issues before processing: 3256, after processing: 302
INFO [runner] Processors filtering stat (in/out): diff: 302/302, path_prettifier: 3256/3256, skip_files: 3256/1381, skip_dirs: 1381/1381, exclude: 1381/1381, exclude-rules: 1381/333, nolint: 333/333, max_per_file_from_linter: 302/302, severity-rules: 302/302, autogenerated_exclude: 1381/1381, identifier_marker: 1381/1381, max_from_linter: 302/302, fixer: 302/302, cgo: 3256/3256, filename_unadjuster: 3256/3256, invalid_issue: 3256/3256, uniq_by_line: 333/302, max_same_issues: 302/302, source_code: 302/302, path_shortener: 302/302, path_prefixer: 302/302, sort_results: 302/302
INFO [runner] processing took 54.777832ms with stages: identifier_marker: 18.22425ms, exclude-rules: 11.041875ms, path_prettifier: 8.284042ms, autogenerated_exclude: 6.610333ms, nolint: 5.056917ms, skip_files: 2.764125ms, source_code: 883.834µs, skip_dirs: 821.25µs, invalid_issue: 528.834µs, cgo: 266.541µs, filename_unadjuster: 225.084µs, uniq_by_line: 38.417µs, path_shortener: 23.875µs, max_per_file_from_linter: 5.75µs, max_same_issues: 916ns, fixer: 458ns, sort_results: 374ns, diff: 292ns, max_from_linter: 250ns, exclude: 250ns, severity-rules: 83ns, path_prefixer: 82ns
INFO [runner] linters took 480.364834ms with stages: goanalysis_metalinter: 425.345917ms
INFO File cache stats: 29 entries of total size 428.5KiB
INFO Memory: 19 samples, avg is 44.7MB, max is 80.6MB
INFO Execution took 1.740711833s
make: *** [lint-api] Error 1
     302

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something to fix lightly then! Cool

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. So what is the plan with this linter?

If we ignore it only in v1beta1, what do we plan to do with v1beta2 that we cannot do today?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to fix this linter, we need to add a maxLength or maxItems to strings and lists respectively.

Now, this could be seen as a backwards incompatible change, however, there are ways to mitigate that

  • Choosing high enough numbers that we are confident it won't actually break anyone (be confident that it's way more than anyone could ever use)
  • Rely on ratcheting validation (1.30) to enable limits and therefore, anyone who is over the limit continue to function, but won't be able to write a new value unless it's under the limit

Now, the question about moving to v1beta2. In theory, we will have to make the same trade-offs at this point, because we need to be able to convert from v1beta1 to v1beta2.
What I'm not quite sure on is whether the converted resource then goes through the v1beta2 validations or not, or whether that's only when written via the v1beta2 API.

We are probably going to have to take the hit here and say that we accept some risk in imposing length limits, or add an exception for every existing field

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we're talking about a huge number of case-by-case decisions. Which is why I was suggesting to take it over.

The alternative is probably me going over all the fields and then writing up the list of length limits we want to use, but that's a huge amount of work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Stefan here, this needs case by case analysis which is quite the undertaking. I'd suggest we try to tackle this group by group, starting with the v1beta1 core types before moving through the experimental stuff

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also do a first PR on 1-2 API types. And once we established some patterns you can follow-up for other types

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, should I open the issue about this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're good, we'll use the existing umbrella issue