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 go1.17 #4822

Merged
merged 4 commits into from Oct 15, 2021
Merged

Update to go1.17 #4822

merged 4 commits into from Oct 15, 2021

Conversation

ialidzhikov
Copy link
Member

@ialidzhikov ialidzhikov commented Oct 11, 2021

/area open-source
/kind enhancement

This PR consists of the following parts:

  1. Update of the go version in the Dockerfile
  2. Update of the go version in the TestDefinitions
  3. make generate changes caused by gofmt that synchronizes //go:build lines with // +build lines - ref https://golang.org/doc/go1.17#gofmt
  4. make format for the ./hack dir to synchronizes the build lines there as well

I didn't update the go version of the module (the one in go.mod) because to be honest I am not able to completely follow the changes to the module graph pruning. go1.17 should be able to work with go1.16 module. This is actually also how kubernetes/kubernetes upgraded to go1.17 - the go runtime version is go1.17 but the module version is still go1.16 - ref kubernetes/kubernetes#103692. So it should not be a problem if we delay to module version update to another PR.

TODOs before merging this:

  • Adapt branch.cfg to use go1.17 for the master branch and go1.16 for the release branches that still require go1.16 (and example commit with the adaptation - ialidzhikov@4200bf9) -> e204dee

TODOs after merging this PR:

Special notes for your reviewer:

  • There is a breaking change in the net pkg - the ParseIP and ParseCIDR functions now reject IPv4 addresses which contain decimal components with leading zeros (ref https://golang.org/doc/go1.17#net). The change tackles the following CVE - https://nvd.nist.gov/vuln/detail/CVE-2021-29923.
    kubernetes/kubernetes decided to preserve the backwards compatibility and K8s will still allow IPv4 addresses that contain decimal components with leading zeros (ref golang 1.17 fails to parse IPs with leading zeros kubernetes/kubernetes#104368).
    So from a Gardener side it is a little bit tricky how to tackle this breaking change - from one side we have a breaking change, on the other side if we decide to take approach like kubernetes/kubernetes to preserve the compatibility we will be still affected by the high CVE. I decided to take the first approach and "accept the breaking change". But if someone has suggestions/objections, they are more than welcome.

Release note:

Since go1.17 both `net.ParseIP` and `net.ParseCIDR` reject leading zeros in the dot-decimal notation of IPv4 addresses. With the update to go1.17, gardener-apiserver now rejects Shoot objects with CIDR ranges that have such leading zeros in the dot-decimal notation. Before updating to this version of gardener-apiserver, make sure that there are no Shoot objects with leading zeros in the dot-decimal notation of an IPv4 address. For reference: https://nvd.nist.gov/vuln/detail/CVE-2021-29923
The golang version is now updated to `1.17.2`.

@gardener-robot gardener-robot added area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/enhancement Enhancement, improvement, extension needs/review size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2021
@ialidzhikov ialidzhikov marked this pull request as ready for review October 13, 2021 08:19
@ialidzhikov ialidzhikov requested review from a team as code owners October 13, 2021 08:19
@timebertt
Copy link
Member

I think, we need to decide, whether to merge this one or #4825 first, because it will conflict (see #4825 (comment))

rfranzke
rfranzke previously approved these changes Oct 14, 2021
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

TODOs after merging this PR:
Update the go.mod to go1.17

Can you open an issue for it? :)

@rfranzke
Copy link
Member

@timebertt In the Daily we agreed to merge #4825 first and then @ialidzhikov can rebase this PR.

Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
@ialidzhikov
Copy link
Member Author

ialidzhikov commented Oct 14, 2021

As expected the verify step will continue to fail until we update the branch.cfg to use go1.17 for the master branch.

@rfranzke can you check whether ialidzhikov/gardener@4200bf9 will do the trick for pipeline update for go1.17? Thanks in advance.

@rfranzke
Copy link
Member

As expected the verify step will continue to fail until we update the branch.cfg to use go1.17 for the master branch.

@rfranzke can you check whether ialidzhikov/gardener@4200bf9 will do the trick for pipeline update for go1.17? Thanks in advance.

I think it looks good @ialidzhikov. Shall we go ahead merging this PR?

@ialidzhikov
Copy link
Member Author

ialidzhikov commented Oct 14, 2021

I think it looks good @ialidzhikov. Shall we go ahead merging this PR?

The PR looks fine to me. Thank you for checking the branch.cfg change and for the approval. I am in general not in a hurry with this PR. After merging this PR new verify runs of existing PRs will fail (they will need to rebase). So to make it 1 idea less breaking for the other PRs, I can proceed with it on Friday evening (when I don't expect a high ratio of PR updates). I first plan to update branch.cfg, then to retrigger the pipeline run and finally merge.

@rfranzke
Copy link
Member

OK, sounds good, thank you very much!

@ialidzhikov ialidzhikov merged commit f06a603 into gardener:master Oct 15, 2021
@ialidzhikov ialidzhikov deleted the go@1.17 branch October 18, 2021 06:58
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
* Update the go version in the Dockerfile

* Update the go version in the TestDefinitions

* make generate

* Format the ./hack dir
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
* Update the go version in the Dockerfile

* Update the go version in the TestDefinitions

* make generate

* Format the ./hack dir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/enhancement Enhancement, improvement, extension size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants