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

golang: bump golang 1.19.12 -> 1.20.7 #6001

Merged
merged 15 commits into from
Aug 21, 2023
Merged

golang: bump golang 1.19.12 -> 1.20.7 #6001

merged 15 commits into from
Aug 21, 2023

Conversation

mfrw
Copy link
Member

@mfrw mfrw commented Aug 16, 2023

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

What does the PR accomplish, why was it needed?
This touches moby-* and golang:

  • Golang bumped to 1.20.7. Note that for go versions >= 1.20, the minimum compiler required is 1.17.
    First the go1.4 bootstrap compiler is built.
    The result of 1.4 compiler is used to build a 1.19.12 compiler.
    The 1.19.12 compiler is used to build the actual 1.20.7 compiler.
    Since go builds fairly quickly, the overhead of this is ~2-3 minutes on a reasonable machine.

  • moby-engine & moby-cli -> bumped to 20.10.25 (which contain the fix http: invalid host header.
    If we build moby-enginne & moby-cli with 1.19.12 compiler, it still does not work.

  • moby-runc bumped to 1.1.9

  • moby-containerd bumped to 1.6.22

Change Log
  • golang: bump version 1.19.12 -> 1.20.7
  • golang: fix compile process for go >= 1.20
  • golang: add comment on how the compiler builds post go >= 1.20
  • golang: cgmanifest: update entry
  • moby-engine: bump version 20.10.24 -> 20.10.25
  • moby-engine: cgmanifest: update entry
  • moby-cli: bump version 20.10.24 -> 20.10.25
  • moby-cli: cgmanifest: update entry
  • moby-containerd: bump version 1.6.18 -> 1.6.22
  • moby-containerd: cgmanifest: add entry
  • moby-runc: bump version 1.1.5 -> 1.1.9
  • moby-runc: cgmanifest: update entry
Does this affect the toolchain?

NO

Associated issues

Fixes:

Links to CVEs
  • NA
Test Methodology
  • Local Build and verification

Pipeline Build:

PS: I used the new containerized-build feature for this and a big kudos to @neha170 for this feature
/cc @christopherco

@microsoft-github-policy-service microsoft-github-policy-service bot added the main PR Destined for main label Aug 16, 2023
mfrw added 12 commits August 17, 2023 06:36
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
@mfrw mfrw marked this pull request as ready for review August 17, 2023 12:45
@mfrw mfrw requested review from a team as code owners August 17, 2023 12:45
Copy link
Contributor

@kanikanema kanikanema left a comment

Choose a reason for hiding this comment

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

Awesome! This is a super important fix.
Changes are well described and look good to me. Thanks for patiently figuring out the root cause and testing it with all the dependent changes.

There is one comment that I did want clarification:

"If we build moby-enginne & moby-cli with 1.19.12 compiler, it still does not work."

I understand the golang upgrade to "permit invalid host header patch"
I am assuming the moby* upgrades still rely on this behaviour; what exactly do the moby* upgrades get us w.r.t. this fix?

@mfrw
Copy link
Member Author

mfrw commented Aug 18, 2023

There is one comment that I did want clarification:

"If we build moby-engine & moby-cli with 1.19.12 compiler, it still does not work."

The moby-* folks tested with go1.20.6 here. There are a few bits that are missing in go1.19.12 w.r.t go1.20.7 apart from the included patch, which is why it does not work.

I understand the golang upgrade to "permit invalid host header patch" I am assuming the moby* upgrades still rely on this behavior; what exactly do the moby* upgrades get us w.r.t. this fix?

The moby-* upgrades do not rely on this patch. The patch permit invalid host header patch is a backport of a fix that the go team admitted as a regression and has been cherry-picked on the release branch but not released. It is related to the same general issue, but is not the one that enables moby. Since we are upgrading go might as well take this patch proactively.

Copy link
Member

@anphel31 anphel31 left a comment

Choose a reason for hiding this comment

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

Great work Falak, thanks!

cgmanifest.json Outdated Show resolved Hide resolved
SPECS/moby-engine/moby-engine.spec Outdated Show resolved Hide resolved
SPECS/moby-cli/moby-cli.spec Outdated Show resolved Hide resolved
Copy link
Contributor

@kanikanema kanikanema left a comment

Choose a reason for hiding this comment

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

LGTM.

Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Signed-off-by: Muhammad Falak R Wani <[email protected]>
@mfrw mfrw merged commit d82493a into main Aug 21, 2023
10 of 11 checks passed
@mfrw mfrw deleted the mfrw/golang-1.20 branch August 21, 2023 17:01
cwyborny pushed a commit to cwyborny/CBL-Mariner that referenced this pull request Sep 29, 2023
Bump following packages:
- golang: 1.19.12 -> 1.20.7
- moby-cli: 20.10.24 -> 20.10.25
- moby-engine: 20.10.24 -> 20.10.25
- moby-containerd:1.6.18 -> 1.6.22
- moby-runc: 1.1.5 -> 1.1.9
This PR fixes docker `http: invalid Host header` error and
bootstraps the go1.20 compiler with go1.19.12 instead of go1.4

Reference: https://go.dev/doc/go1.20#bootstrap
Signed-off-by: Muhammad Falak R Wani <[email protected]>
cwyborny pushed a commit to cwyborny/CBL-Mariner that referenced this pull request Oct 3, 2023
Bump following packages:
- golang: 1.19.12 -> 1.20.7
- moby-cli: 20.10.24 -> 20.10.25
- moby-engine: 20.10.24 -> 20.10.25
- moby-containerd:1.6.18 -> 1.6.22
- moby-runc: 1.1.5 -> 1.1.9
This PR fixes docker `http: invalid Host header` error and
bootstraps the go1.20 compiler with go1.19.12 instead of go1.4

Reference: https://go.dev/doc/go1.20#bootstrap
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
main PR Destined for main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants