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

Discussion: Bumping Go in older Cluster API releases #7974

Closed
sbueringer opened this issue Jan 23, 2023 · 6 comments · Fixed by #9415
Closed

Discussion: Bumping Go in older Cluster API releases #7974

sbueringer opened this issue Jan 23, 2023 · 6 comments · Fixed by #9415
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sbueringer
Copy link
Member

sbueringer commented Jan 23, 2023

Upstream k/k is bumping the Go version to 1.19 on patch versions of older Kubernetes minor releases (v1.23 + v1.24) (xref: https://groups.google.com/a/kubernetes.io/g/dev/c/RollV1z4fNQ)

They are doing this for two reasons:

  • To ensure v1.23 can be released with a CVE free Go 1.19 (instead of Go 1.17)
  • To ensure that they are using a supported Go version (1.18) for the remaining 4 months that v1.24 is in support.

Goal of is this issue is to discuss if this applies to Cluster API as well.

Current state Cluster API:

  • Go version we use to compile clusterctl and our binaries (Makefile):
    • main: 1.19.4
    • release-1.3: 1.19.4
    • release-1.2: 1.18.9
  • Go version in go.mod:
    • main: 1.19
    • release-1.3: 1.19
    • release-1.2: 1.17

Additionally our test jobs are using the following:

  • Go version used for unit and e2e tests (+ a few binaries like ginkgo)
    • main: gcr.io/k8s-staging-test-infra/kubekins-e2e:v20230120-fd0935fac9-1.26 => Go 1.19.5
    • release-1.3: gcr.io/k8s-staging-test-infra/kubekins-e2e:v20230120-fd0935fac9-1.25 => Go 1.19.5
    • release-1.2: gcr.io/k8s-staging-test-infra/kubekins-e2e:v20230120-fd0935fac9-1.24 => Go 1.19.5

Some information about Go releases:

  • CVE free (as of today): 1.19.4+, 1.18.9+
  • latest: 1.19.5, 1.18.10
  • Go 1.18 EOL: 2023-03-15 (Cluster API v1.2 EOL: 2023-03-28)

I would propose:

In general I would recommend:

  • Let's bump to newer Go minor versions if there is a significant period (> 1 month ? TBD) that a Cluster API release is in support after the corresponding Go version is already out of support
  • Let's definitely bump if bumping allows us to fix a CVE

Somewhat related I did a few experiments around the Go version in go.mod:

  • To use new language features each module has to declare the go version it requires for those features (e.g. 1.18 for generics) (xref: https://go.dev/doc/modules/gomod-ref#go)
  • It's fine if dependencies have a higher go version in go.mod as the current module.
  • It is only necessary that the go version that is used to compile the binary is high enough to compile all dependencies (i.e. if a dependency of CAPI is declaring it requires Go 1.19 language features, CAPI has to be compiled with Go 1.19, the version in the CAPI go.mod could stay on 1.17 if we want to)
  • This means that when we would bump the version in go.mod, we would require all our consumers to use at least this Go minor version to compile their binaries, while they could still keep an older go version in their go.mod file.

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 23, 2023
@sbueringer
Copy link
Member Author

cc @fabriziopandini @jackfrancis

@jackfrancis
Copy link
Contributor

The above lgtm.

My only recommendation would be a statement that we should aim to have no delta between the go.mod outcome and the version of golang used to compile release binaries. If we do then strictly speaking we aren't testing what we're releasing.

Not saying that's the lowest hanging fruit, but if we're compiling aspirational goals I'd vote for that to be in the list

Thanks for putting all this together @sbueringer!

@fabriziopandini
Copy link
Member

/triage accepted
Thanks for this work of reasearch!

+1 to bump to 1.19.5 and 1.18.10
I consider that the skew we had in 1.2 (go.mod 1.17, compile to 1.18) has been an exception due to the introduction of generics, so we should be ok from now on.

Wondering if we should write something in our contribution guidelines. something like

For each branch, we will bump to the latest go patch release available for the go version in use of that branch
If the go version for a specific branch goes out of support, we will consider if to bump to a newer go version case by case bases, because this might have impacts on everyone importing Cluster API

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 23, 2023
@sbueringer
Copy link
Member Author

sbueringer commented Jan 24, 2023

My only recommendation would be a statement that we should aim to have no delta between the go.mod outcome and the version of golang used to compile release binaries. If we do then strictly speaking we aren't testing what we're releasing.

Not sure I understand that correctly.

In my interpretation, the version in the go.mod file controls which language feature this module can use. For example if a module wants to use generics it has to declare that it requires at least 1.18 in the go.mod file. This does not mean that the module is actually compiled against this go version, it just means that it requires/uses language features of Go 1.18.

The module is always compiled with the Go version that is used to compile (aka the version of the go binary used for go build), not the version in the go.mod file.

So tl;dr whatever Go version we use to run go build is the version with which we compile our module and all its dependencies. The version in the go.mod file just specifices which language features this specific module is allowed to use.

If we do then strictly speaking we aren't testing what we're releasing

The go version in the Makefile is used to compile our controllers for release and for all our e2e tests, i.e. we always e2e test the controllers that we release.

There is a divergence, but it is that our unit tests are using whatever go version is in the kubekins image not the version from our Makefile. I.e. on release-1.2 our unit tests are running with Go 1.19.5 (Go version from kubekins) but our release & test binaries are compiled with Go 1.18 (Go version from the Makefile). Similarly our e2e test code is also run with the Go version in the kubekins image.

Not sure if you meant that.

This could probably be addressed by running our unit and e2e test inside a container. Then we should be independent of the Go version from kubekins.

@jackfrancis
Copy link
Contributor

jackfrancis commented Jan 24, 2023

@sbueringer those are all good points, but I would probably say the following:

For the part of go.mod's responsibility that includes allowing (or disallowing) certain golang features/builtins, the best way to ensure hygiene between source and compiled binaries to is to compile w/ the same major.minor of go that is declared in go.mod. For example, as you say, a go.mod of 1.18 doesn't require generics, but it will certainly allow them. If our CI is set to compile @ golang 1.17 and our go.mod is configured @ 1.18 then we are going to have a problem.

I think a good pattern to copy is various github actions who re-use the version of golang in the repo's go.mod to determine the version of go as input for the action. For example, I think this is more durable:

      - name: install go
        uses: actions/setup-go@v3
        with:
          go-version-file: 'go.mod'

compared to this:

    - uses: actions/setup-go@v3
      with:
        go-version: '^1.19'

@sbueringer
Copy link
Member Author

sbueringer commented Jan 25, 2023

For the part of go.mod's responsibility that includes allowing (or disallowing) certain golang features/builtins, the best way to ensure hygiene between source and compiled binaries to is to compile w/ the same major.minor of go that is declared in go.mod.

Why? go.mod declares we require certain minor Go version features. It doesn't give any indication about which Go version it should or has to be compiled with (especially not the patch version, as it cannot be specified in go.mod).

For example, as you say, a go.mod of 1.18 doesn't require generics, but it will certainly allow them.

A go.mod of 1.18 allows generics, but only in that module (and in that module they are either used or not, that won't be influenced by folks importing our module). It doesn't make any statements about other modules. For example you can have Go 1.17 in your current module and one of your dependencies can have 1.18 and use generics. Your module will still work. The only limitation now is that you have to compile your module with Go >= 1.18 as one of your dependencies requires 1.18. But it's still totally valid to have 1.17 in your go.mod.

If our CI is set to compile @ golang 1.17 and our go.mod is configured @ 1.18 then we are going to have a problem.

Yes. The problem is that it won't even compile, so we will never get a PR merged which would lead to this state. We always have to compile with a Go version that is >= the go.mod versions in our modules and all of their dependencies.

I think a good pattern to copy is various github actions who re-use the version of golang in the repo's go.mod to determine the version of go as input for the action. For example, I think this is more durable:

I think it would be good to reduce the number of places where we specify the go version. But I think we should use the same version we use to compile our binaries and controllers for the GitHub actions. And this version is specified (including the patch version) in our Makefile. So I think re-using the version we specify in our Makefile for our actions is reasonable.

Just using e.g. "1.19" from our go.mod is not precise enough as we explicitly want e.g. v1.19.5 (and not an earlier or later patch version).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants