🌱 Add KAL linter for linting API conventions #11733

JoelSpeed · 2025-01-22T13:11:11Z

What this PR does / why we need it:

This PR adds KAL to the linter workflows to lint API conventions.

The linter is a WIP project that I have been working on and the focus of the project is to try and enforce API conventions via a linter, to take off some of the cognitive load of API review.

At the moment, there are 10 sub-linters implemented, each of which I've listed out within the configuration with a description of what they mean.

We should discuss each one and agree upon them, and then, the intention is that we can use this linter for the new v1beta2 types.

The set up here runs KAL separately from the main golangci-lint run, so that the GitHub workflow for linting can continue to use the official GH action, which has some caching and speed improvements over running the binary manually.

The config for KAL is set to run only on files containing api/v1beta2 in the name. Since we don't have any of those yet, I've temporarily added api/v1beta1 so that we can see what the output might look like. We will want to remove that before merging.

I've also updated GolangCI-Lint to v1.63.4. v1.63 introduced the ability for custom linters to apply fixes to the code, so lint-fix will work with KAL as well provided we use a v1.63 or later version. I figured it best to update the main version of the linter at the same time.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

k8s-ci-robot · 2025-01-22T13:11:15Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

.github/workflows/pr-golangci-lint.yaml

hack/tools/.custom-gcl.yaml

test/e2e/clusterclass_changes.go

.github/workflows/pr-golangci-lint.yaml

nrb · 2025-01-22T20:04:34Z

/area api

sbueringer

Very nice!

Obviously have to enable them incrementally and have to decide linter-by-linter which ones we can also just apply to existing APIs (because they are non-breaking, e.g. commentstart, optionalorrequired) vs. only on new API fields / apiVersions.

.golangci-kal.yml

sbueringer · 2025-01-30T12:33:23Z

.golangci-kal.yml

+      settings:
+        linters:
+          enable:
+          - "conditions" # Ensure conditions have the correct json tags and markers.


+1

Seems like the linter also enforces:

// +patchStrategy=merge // +patchMergeKey=type

Does this make sense for CRDs?

Obviously we can't fix this finding for the old condition fields that use clusterv1.Conditions

I believe those annotations still work for client-side implementations of patching. The patch strategy and key are part of the K8s extension of the OpenAPI v3 schema, so assuming controller-gen is capable of putting these in the right place , they can still be effective for clients like Kubectl patch

Looking at https://tech.aabouzaid.com/2022/11/set-openapi-patch-strategy-for-kubernetes-custom-resources-kustomize.html and other sources of generated CRDs, I don't think controller-gen currently populates these, but it looks like it would work if it did!

Perhaps dropping the patch keys for CRDs is appropriate, I'll check this with api-machinery folks, this could just be a bug in the linter

(actually didn't initially remember that I clarified this already a while back :))

https://kubernetes.slack.com/archives/C0EG7JC6T/p1718717630176899

Then that's a bug in KAL, and I will get that fixed! Thanks for highlighting

Edit: added an issue to fix this JoelSpeed/kal#35

Thx!

I guess in general having both sets of markers would make sense only for structs that are supposed to be embedded in builtin types and CRDs.

Added JoelSpeed/kal#37, which I'll update to once merged.

This will allow us to forbid Proto and PatchStrategy markers from our types, which I think is appropriate as I don't think we will be importing any of our types into core types any time soon

sbueringer · 2025-01-30T12:35:39Z

.golangci-kal.yml

+          - "commentstart" # Ensure comments start with the serialized version of the field name.
+          - "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.


+1 I would be even okay with picking some sane max values for existing fields

sbueringer · 2025-01-30T12:38:15Z

.golangci-kal.yml

+          - "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.


Not sure about this one.

Mostly because I'm not sure if every bool can be intuitively expressed as an enum instead. I think we have a lot of fields that worked quite well as a bool and we have no need/plans to evolve them

Additionally, general edge case, in CABPK / KCP we ~ inherit a significant part of our API from kubeadm. I wouldn't like to diverge from these upstream APIs to avoid this finding.

This is certainly one of the more controversial ones I agree. We should have a wider discussion about this, and how we might handle the inherited APIs from Kubeadm, if the linter is going to pick those up and we don't want to change them at all, then this could be awkward from a linter perspective

besides the inconveniences it might cause, I'd like to have it enable to force us think twice. If enabled, what would be the process to allow exceptions?

You have two options, either you // nolint:kal which would disable all linter checks on this line, or you can use the golangci-lint configuration to exclude a specific instance using their more targeted/complex exclusions

If we go for the --new-from-rev, we could also just override the check and then once it's in main, it's not going to come up again (on PRs at least)

Another reason why I don't like using enums for this. It forces everyone to handle "unknown" values just in case the field ever evolves (which might never happen)

On the other hand, if you do decide to expand later, you now have maybe two bools, where one of them is only allowed to be set when the other is set to a specific value, which can be awkward (we can probably bike shed on bools for a long time)

Yeah. I don't remember a single bool in CAPI where we hit this issue over the last years :)

I guess I would make this a case-by-case decision

sbueringer · 2025-01-30T12:40:11Z

.golangci-kal.yml

+          - "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.
+          - "nophase" # Phase fields are discouraged by the Kube API conventions, use conditions instead.


This one will be controversial. We initially wanted to drop phase, but then later realized that the phases are actually useful to users because they quickly give users an idea about the phase a Cluster/Machine is in without having to parse through a number of conditions

Interesting, we should check the history of the API convention on this one. As the comment says, the idea would generally be something like a Ready condition to show the general state, I guess if we can represent printer columns well based on the condition, then the phase may become less fundamental?

Do you happen to have a link to prior discussion on this topic?

I'm not sure where that discussion happened. But I think @vincepri / @fabriziopandini should have context on it

EDIT: Found it: #10897 (comment) & #10897 (comment)

Given that recent discussion about diverging from kube conventions, and the consensus there, I think it makes sense to capture this, and disable the nophase linter and comment why we are disabling it

.golangci-kal.yml

sbueringer · 2025-01-30T12:44:51Z

@enxebre @chrischdi @fabriziopandini @vincepri PTAL

enxebre · 2025-02-03T11:00:58Z

this is great, thanks!

.golangci-kal.yml

fabriziopandini · 2025-02-07T19:42:37Z

Great work @JoelSpeed!
I took a look at the PR / rules together with @chrischdi and @sbueringer

Based on our discussion I would propose to work to merge this PR initially with all the linter disabled, and then send follow up PR enabling one linter at time by fixing findings / adding exceptions.

For most of the linters seems we can probably fix stuff ~pretty fast directly in v1beta1 / without breaking changes (this will also avoid additional complexity when we will work on v1beta2)

optionalorrequired
requiredfields
statussubresource
commentstart
integers (might be we will be required to have a few exceptions to avoid breaking changes)
jsontags (might be we will be required to have a few exceptions to avoid breaking changes)
conditions (but this requires an exception till v1beta2)

We have to take a closer look at findings for following links, because they might be breaking

"maxlength"
"nobools"

Instead most probably we should keep the "nophase" linter off.

JoelSpeed · 2025-02-08T09:08:33Z

Sounds like a reasonable plan to me, so for now keep it focusing on the v1beta1 types, everything disabled and look to enable based on the above list.

I'll get the PR updated to reflect that

JoelSpeed · 2025-02-12T14:42:19Z

@sbueringer @fabriziopandini PR is now updated so that:

By default, we only lint changes since main
- GOLANGCI_LINT_EXTRA_ARGS= make lint-api will however lint the whole set of APIs, even those pre-existing
No linters are actually enabled
The latest version of KAL is updated

If we get this merged, I can follow up with a handful of PRs to start getting various linters enabled, and go through the discussions required for each linter

sbueringer · 2025-02-12T17:27:04Z

Makefile

@@ -667,6 +672,15 @@ lint-dockerfiles:
 lint-fix: $(GOLANGCI_LINT) ## Lint the codebase and run auto-fixers if supported by the linter
 	GOLANGCI_LINT_EXTRA_ARGS=--fix $(MAKE) lint

+.PHONY: lint-api
+lint-api: GOLANGCI_LINT_EXTRA_ARGS?=--new-from-rev=main


What happens once this Makefile will end up on a release branch?

(I think we have to ensure this is the base branch of the PR and not always main)

I think it may be simpler to not use --new-from-rev, but exclusions instead 🤔 We may have to build up that list then, but this seems to be the easier approach and always works on any branch or commit then.

Is there an env var that would expose the branch name of the base branch? I know on prow you have PULL_BASE_SHA but I don't know GH actions well

Okay with the decision to not use --new-from-rev for now this finding became irrelevant

sbueringer · 2025-02-12T17:32:14Z

@JoelSpeed Would be nice to open an umbrella issue

JoelSpeed · 2025-02-12T18:11:25Z

Created #11834 to start tracking the enablement of each of the linters, we can record decisions there as well where we decide against a linter rule for any reason

sbueringer · 2025-02-12T18:35:33Z

/lgtm

/assign @fabriziopandini

k8s-ci-robot · 2025-02-12T18:35:40Z

LGTM label has been added.

Git tree hash: 0741ff7607218615b5d24ee478bd60cfbbfb788e

sbueringer · 2025-02-12T20:18:10Z

Or
/assign @chrischdi
actually :D

fabriziopandini

/lgtm

k8s-ci-robot · 2025-02-13T14:45:50Z

LGTM label has been added.

Git tree hash: 0741ff7607218615b5d24ee478bd60cfbbfb788e

sbueringer · 2025-02-13T15:10:21Z

/approve

k8s-ci-robot · 2025-02-13T15:10:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 22, 2025

k8s-ci-robot requested review from elmiko and g-gaston January 22, 2025 13:11

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2025

JoelSpeed changed the title ~~DNM: Add KAL linter for linting API conventions~~ 🌱 DNM: Add KAL linter for linting API conventions Jan 22, 2025

JoelSpeed mentioned this pull request Jan 22, 2025

Support for module plugin system via .custom-gcl.yml golangci/golangci-lint-action#1076

Open

2 tasks

JoelSpeed commented Jan 22, 2025

View reviewed changes

.github/workflows/pr-golangci-lint.yaml Show resolved Hide resolved

nrb reviewed Jan 22, 2025

View reviewed changes

hack/tools/.custom-gcl.yaml Show resolved Hide resolved

nrb reviewed Jan 22, 2025

View reviewed changes

test/e2e/clusterclass_changes.go Outdated Show resolved Hide resolved

sbueringer reviewed Jan 22, 2025

View reviewed changes

.github/workflows/pr-golangci-lint.yaml Show resolved Hide resolved

k8s-ci-robot added area/api Issues or PRs related to the APIs and removed do-not-merge/needs-area PR is missing an area label labels Jan 22, 2025

JoelSpeed force-pushed the add-api-linter branch from 56611bc to ab11d8e Compare January 23, 2025 11:06

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2025

sbueringer reviewed Jan 30, 2025

View reviewed changes

chrischdi mentioned this pull request Feb 5, 2025

Check API for optional vs required for next revision #10915

Closed

sbueringer reviewed Feb 6, 2025

View reviewed changes

.golangci-kal.yml Show resolved Hide resolved

JoelSpeed force-pushed the add-api-linter branch from 818fc3b to cec9549 Compare February 12, 2025 14:37

JoelSpeed changed the title ~~🌱 DNM: Add KAL linter for linting API conventions~~ 🌱 Add KAL linter for linting API conventions Feb 12, 2025

JoelSpeed marked this pull request as ready for review February 12, 2025 14:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2025

k8s-ci-robot requested review from richardcase and vincepri February 12, 2025 14:39

JoelSpeed force-pushed the add-api-linter branch from cec9549 to 3ddf10c Compare February 12, 2025 14:40

sbueringer reviewed Feb 12, 2025

View reviewed changes

JoelSpeed mentioned this pull request Feb 12, 2025

API Linting Tracking Issue #11834

Open

10 tasks

JoelSpeed added 2 commits February 12, 2025 18:35

Add custom KAL golangci-lint in-place of golangci-lint

bedbb80

Add KAL to lint workflow

49f8a9a

k8s-ci-robot assigned fabriziopandini and sbueringer Feb 12, 2025

JoelSpeed force-pushed the add-api-linter branch from 3ddf10c to 49f8a9a Compare February 12, 2025 18:35

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2025

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2025

k8s-ci-robot requested review from fabriziopandini and sbueringer February 12, 2025 18:35

k8s-ci-robot assigned chrischdi Feb 12, 2025

fabriziopandini reviewed Feb 13, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2025

k8s-ci-robot merged commit 0c755da into kubernetes-sigs:main Feb 13, 2025
19 checks passed

k8s-ci-robot added this to the v1.10 milestone Feb 13, 2025

🌱 Add KAL linter for linting API conventions #11733

🌱 Add KAL linter for linting API conventions #11733

Conversation

JoelSpeed commented Jan 22, 2025

k8s-ci-robot commented Jan 22, 2025

nrb commented Jan 22, 2025

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

JoelSpeed Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

sbueringer Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Jan 30, 2025

enxebre commented Feb 3, 2025

fabriziopandini commented Feb 7, 2025 • edited Loading

JoelSpeed commented Feb 8, 2025

JoelSpeed commented Feb 12, 2025

sbueringer Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Feb 12, 2025

JoelSpeed commented Feb 12, 2025

sbueringer commented Feb 12, 2025

k8s-ci-robot commented Feb 12, 2025

sbueringer commented Feb 12, 2025

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 13, 2025

sbueringer commented Feb 13, 2025

k8s-ci-robot commented Feb 13, 2025

sbueringer left a comment •

edited

Loading

sbueringer Jan 30, 2025 •

edited

Loading

JoelSpeed Jan 31, 2025 •

edited

Loading

sbueringer Feb 3, 2025 •

edited

Loading

sbueringer Jan 30, 2025 •

edited

Loading

enxebre Feb 3, 2025 •

edited

Loading

sbueringer Feb 3, 2025 •

edited

Loading

sbueringer Feb 4, 2025 •

edited

Loading

sbueringer Jan 30, 2025 •

edited

Loading

fabriziopandini commented Feb 7, 2025 •

edited

Loading

sbueringer Feb 12, 2025 •

edited

Loading