Skip to content

[chore] add logic to update our tests k8s versions #1792

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.

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

Merged
merged 2 commits into from
May 12, 2025
Merged

Conversation

dloucasfx
Copy link
Contributor

Description:
Added a utility to get a list of supported vanilla k8s clusters, compare it to our test k8s versions and update when necessary.

I added a make target, which we can keep or remove, but the end goal is to add a gh action that uses this logic to create a PR with the changes if detected.

We currently have a bunch of EOL k8s versions that we use, it's a way to check compatibility with cloud providers that extend the support.
This is not ideal as we keep increasing our test matrix and the vanilla cluster is not identical to the one used by the cloud provider. To test for schema compatibility, we should start using kubeconform

To Do (sperate PRs):

  • Add a gh action
  • track EKS, AKS and GKE

Testing:
unit test

@dloucasfx dloucasfx requested review from a team as code owners April 22, 2025 18:26
@dloucasfx dloucasfx force-pushed the testk8sversions branch 3 times, most recently from 03a7022 to 382c2dc Compare April 23, 2025 03:20
@dloucasfx dloucasfx changed the title [chore] add logic to update our tests k8s versions ****DO NOT MERGE YET**** [chore] add logic to update our tests k8s versions Apr 23, 2025
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Nice!

@dloucasfx dloucasfx requested a review from pjanotti April 23, 2025 21:10
@dloucasfx dloucasfx force-pushed the testk8sversions branch 2 times, most recently from 6a99c36 to 6c53f1e Compare April 24, 2025 16:38
Copy link
Contributor

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Looks good! With all of my comments feel free to drop or clarify as you see fit. I'm generally trying to reduce regexes and string parsing logic as much as possible. I'd be happy to discuss any options here.

@dloucasfx dloucasfx force-pushed the testk8sversions branch 2 times, most recently from e36557b to d24da35 Compare May 2, 2025 17:30
Copy link
Contributor

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Sorry about all of the nits, once the formatting tools are introduced to the repo it'll catch earlier on in the process.

@dloucasfx dloucasfx force-pushed the testk8sversions branch 3 times, most recently from f1ab9ef to 0656790 Compare May 5, 2025 15:29
@dloucasfx
Copy link
Contributor Author

dloucasfx commented May 5, 2025

New push:

  • Addressed most of the review entries. For var shadowing, I only addressed the linter errors as the others are not necessary
  • Updated the golangci config to force strict shadow detection and excluded this tool from gosec to prevent unnecessary detections (addressing this in a separate PR)
tools/k8s_versions/update_k8s_versions.go:197:11: G306: Expect WriteFile permissions to be 0600 or less (gosec)
        if err = os.WriteFile(filePath, updatedContent, 0644); err != nil {
                 ^
tools/k8s_versions/update_k8s_versions.go:218:15: G107: Potential HTTP request made with variable url (gosec)
        resp, err := http.Get(url)
      
  • Rebased and resolved conflicts

@crobert-1
Copy link
Contributor

Once this PR has been rebased on main we can drop all of my shadowing err issues. main now has lint checks running on PRs so once lint passes on this PR my concerns are resolved 👍

@dloucasfx dloucasfx force-pushed the testk8sversions branch 3 times, most recently from 02938f3 to f4c62f5 Compare May 7, 2025 16:12
@dloucasfx
Copy link
Contributor Author

Once this PR has been rebased on main we can drop all of my shadowing err issues. main now has lint checks running on PRs so once lint passes on this PR my concerns are resolved 👍

@crobert-1
Pushed the following changes:

tools/k8s_versions/update_k8s_versions.go:220:15: G107: Potential HTTP request made with variable url (gosec)
        resp, err := http.Get(url)
  • The only lint that is failing now is
tools/k8s_versions/update_k8s_versions.go:199:11: G306: Expect WriteFile permissions to be 0640 or less (gosec)
        if err = os.WriteFile(filePath, updatedContent, 0o644); err != nil {

This is very restrictive, not if we want to no lint it or change the G306 gosec config to

    config:
      # Maximum allowed permissions mode for os.OpenFile and os.Chmod
      # Default: "0600"
      G306: "0644"

@crobert-1
Copy link
Contributor

This is very restrictive, not if we want to no lint it or change the G306 gosec config to

I'd say for now my preference is to just add a nolint comment. I generally prefer getting warnings up front on new changes to let us make an educated decision on a case by case basis. Happy to hear from others though 👍

Copy link
Contributor

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

LGTM once CI is passing. Thanks for all of your work here @dloucasfx!

Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Can be merged once the minor comment is addressed

return false, nil
}

func decrementMinorMinorVersion(version string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's called the patch version in semver :)

Suggested change
func decrementMinorMinorVersion(version string) (string, error) {
func decrementPatchVersion(version string) (string, error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

roger that!

Signed-off-by: Dani Louca <[email protected]>
@jinja2
Copy link
Collaborator

jinja2 commented May 9, 2025

@dloucasfx We have some tests which use the K8S_VERSION var to look up for expected test data since the metrics change with k8s version for some receiver. These tests (i can only remember histograms from the top of my head) will probably start failing once a new minor version is introduced by this workflow. I took a look at the code and don't see this being addressed. Is this known? we can fix the tests once the new version is added or we could probably just copy the data from new minor version - 1 when the workflow runs.

@dloucasfx dloucasfx merged commit 6a6ba28 into main May 12, 2025
87 checks passed
@dloucasfx dloucasfx deleted the testk8sversions branch May 12, 2025 14:45
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants