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.

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

Update to Traefik v2.10, go1.21 and alpine 3.18 #842

Open
wants to merge 10 commits into
base: v1.4
Choose a base branch
from

Conversation

reneleonhardt
Copy link

@reneleonhardt reneleonhardt commented Oct 14, 2023

What does this PR do?

This PR:

  • Update to go 1.21
  • Update direct dependencies (except k8s.io/api because it's a traefik requirement)
  • Update to golangci-lint 1.55.1 (new excludes necessary)
  • Update to k3s 1.21 (to sync integration tests with docs)
  • Update to k3d 5.6.0
  • Update to alpine 3.18
  • Update to Semaphore machine ubuntu2004
  • Update most versioned docker images
  • Update to traefik 2.10 (second commit)
  • Does not update k3s to 1.25 in integration tests (to stay in sync with docs)
  • Does not add or update servicemeshinterface (one alpha update is available for all APIs)
  • Fixes 14 high and 14 medium CVEs through Traefik, alpine and dependency updates (0 CVEs in new image)

How to test it

  • Run all tests described in docs

Additional Notes

This PR updates everything to the latest versions, except:

  • k3s should be updated to 1.25 in integration tests and docs adapted accordingly
  • servicemeshinterface APIs should be updated
  • Docs toolchain should be updated
  • Tests should also cover coredns 1.10 and 1.11

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

Hello,

can you rebase your PR on the branch v1.4?

the modifications of the .golangci.yml are not right but we will talk about them after the rebase.

@reneleonhardt
Copy link
Author

Sure, I didn't see it in the build instructions, master is still the default branch.
Makes integration testing easier 😄

@ldez
Copy link
Contributor

ldez commented Oct 14, 2023

You can update Traefik v2.10 inside this PR (in a separate commit).

The update of dependencies should be linked to the Traefik update.
The indirect dependencies should not be updated (except those related to the update of Traefik)

Some notes:

  • it's better not to create a PR based on another PR.
  • it's better to open an issue before creating a PR to explain your changes.

@reneleonhardt
Copy link
Author

reneleonhardt commented Oct 14, 2023

I didn't see that, should I open an issue now? I will close the second PR.

I will remove the indirect updates and add Traefik 2.10 in a separate commit.

@ldez
Copy link
Contributor

ldez commented Oct 14, 2023

should I open an issue now?

the PR is already open, so you don't need it.
The fact that opening an issue before creating a PR is a classic for any open source project: that allows us to talk before the PR and save time.

.golangci.toml Outdated
Comment on lines 80 to 85
"Error return value of .((os\\.)?std(out|err)\\..*|.*Close|.*Flush|os\\.Remove(All)?|.*printf?|os\\.(Un)?Setenv|\\(k8s\\.io/client-go/tools/cache\\.SharedInformer\\)\\.AddEventHandler). is not checked",
"package-comments: should have a package comment",
"import '[^']+' is not allowed from list 'Main'",
"if-return: redundant if ...; err != nil check, just return error instead.",
"unused-parameter: parameter '[^']+' seems to be unused, consider removing or renaming it as _",
"tag is not aligned, should be: description:",
Copy link
Contributor

@ldez ldez Oct 14, 2023

Choose a reason for hiding this comment

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

you should revert this modification.
And add the following snippet inside the .golangci.toml file:

[linters-settings.depguard]
    [linters-settings.depguard.rules.main]
        [[linters-settings.depguard.rules.main.deny]]
            pkg = "github.com/instana/testify"
            desc = "not allowed"

        [[linters-settings.depguard.rules.main.deny]]
            pkg = "github.com/pkg/errors"
            desc = "Should be replaced by standard lib errors package"

[linters-settings.tagalign]
    align = false
    sort = true
    order = [
        "description",
        "json",
        "toml",
        "yaml",
        "yml",
        "label",
        "label-slice-as-struct",
        "file",
        "kv",
        "export"
    ]

The other problem should be fixed (and not ignored) except "package-comments: should have a package comment"

Copy link
Author

Choose a reason for hiding this comment

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

Will do except fixing the actual problems, you know your code better 😅

Copy link
Author

Choose a reason for hiding this comment

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

Could fix linter issues except AddEventHandler and unused-parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry can you revert the change related to tagalign and add this configuration:

[linters-settings.tagalign]
    align = false
    sort = true
    order = [
        "description",
        "json",
        "toml",
        "yaml",
        "yml",
        "label",
        "label-slice-as-struct",
        "file",
        "kv",
        "export"
    ]

Copy link
Contributor

Choose a reason for hiding this comment

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

You can fix unused-parameter: you just have to replace the name of the parameter with _.

And AddEventHandler can also be fixed but it's a bit more complex, so you can ignore them inside this PR but I will fix them after the merge of your PR.

Copy link
Author

Choose a reason for hiding this comment

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

Already tried that yesterday, it doesn't work, just another linter pops up instead of revive:
File is not `gci`-ed with --skip-generated -s standard -s default (gci)
It would be much easier if you fix those special cases yourself, you know your code and Go better than me.

And no single check has been run yet, can't you start the GitHub Actions manually to see if all integration tests work?

Copy link
Contributor

Choose a reason for hiding this comment

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

It works I did it locally.

@reneleonhardt reneleonhardt changed the title Update to go1.21 and alpine 3.18 Update to go1.21, alpine 3.18 and Traefik v2.10 Oct 14, 2023
@reneleonhardt reneleonhardt changed the title Update to go1.21, alpine 3.18 and Traefik v2.10 Update to Traefik v2.10, go1.21 and alpine 3.18 Oct 14, 2023
@reneleonhardt
Copy link
Author

reneleonhardt commented Oct 15, 2023

If I remove an unnecessary layer and use upx, I can shrink the image to 16 MB (58% smaller than v1.4.8).
But it takes 97 seconds longer to build locally.
I pushed a commit, the COPY certs command is still inside, I would suggest to remove it, it would only make a difference if you would rebuild daily and would be faster than the alpine rebuild.

When I use scratch instead of alpine, I can shrink the image by another 50% (8.4 MB) and also shrink the attack surface significantly (no binaries except traefik-mesh).

@reneleonhardt
Copy link
Author

I added more versions to coredns_test.go according to https://github.com/coredns/deployment/blob/master/kubernetes/CoreDNS-k8s_version.md in a separate commit.
1.9.0 is replaced by 1.9.3 (k8s 1.25 and 1.26), 1.11.1 is included too (but not used in a k8s release yet).

@reneleonhardt
Copy link
Author

https://www.freebsd.org/platforms/arm/#status states that freebsd/arm64 is fully supported since 2021, can I remove the goreleaser ignore?
https://www.openbsd.org/arm64.html seems to be usable too.

@kevinpollet
Copy link
Member

Hello @reneleonhardt and thanks for your contribution,

As said by @ldez, it is better to open issues before opening a pull request and to address only one thing in it (see our contributing guide). As this pull request is already open, I would only address the dependency, image, and go version updates, and revert the other changes (and open issues accordingly).

@reneleonhardt
Copy link
Author

Hello @reneleonhardt and thanks for your contribution,

As said by @ldez, it is better to open issues before opening a pull request and to address only one thing in it (see our contributing guide). As this pull request is already open, I would only address the dependency, image, and go version updates, and revert the other changes (and open issues accordingly).

Hello @kevinpollet okay I will try tonight, but as you know updating the images alone require many other updates like the whole docs toolchain.

@reneleonhardt
Copy link
Author

I reverted what I understood that you don't want now.
My work is finished, please run your CI for the first time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants