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

fix(go/base): set GOTOOLCHAIN=local and skip building arm using the base Dockerfile #524

Merged
merged 15 commits into from
Mar 4, 2025

Conversation

dliappis
Copy link
Contributor

@dliappis dliappis commented Feb 27, 2025

Disable the automatic downloads of the Go toolchain.

For the goal of reproducible builds using golang-crossbuild, we want all of the binaries used to produce our artifacts to be self-contained in the image. Adding GOTOOLCHAIN=local disable downloads of newer toolchains.

This changes the entrypoint of the images to document that GOTOOLCHAIN is set during builds. And it sets GOTOOLCHAIN=local in the Dockerfile so that when building entrypoint.go it uses the toolchain that was previously installed.

The two Dockerfiles (base and base-arm) had different 'go build' commands. This unifies them to both have CGO_ENABLED=0 and to explicitly set GOARCH to avoid accidentally producing a binary for the wrong target architecture (which we've seen happen for unknown reasons).

Additionally we remove the arm target from the base Dockerfile, to address the CI concurrency problem described
in #523 (comment)

https://go.dev/doc/toolchain

Co-Authored by @andrewkroh

andrewkroh and others added 2 commits February 26, 2025 22:13
Disable the automatic downloads of the Go toolchain.

For the goal of reproducible builds using golang-crossbuild, we want all
of the binaries used to produce our artifacts to be self-contained in
the image. Adding GOTOOLCHAIN=local disable downloads of newer
toolchains.

This changes the entrypoint of the images to document that GOTOOLCHAIN
is set during builds.  And it sets GOTOOLCHAIN=local in the Dockerfile
so that when building entrypoint.go it uses the toolchain that was
previously installed.

The two Dockerfiles (base and base-arm) had different 'go build'
commands. This unifies them to both have CGO_ENABLED=0 and to explicitly
set GOARCH to avoid accidentally producing a binary for the wrong target
architecture (which we've seen happen for unknown reasons).

https://go.dev/doc/toolchain
Copy link

mergify bot commented Feb 27, 2025

This pull request does not have a backport label. Could you fix it @dliappis? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d is the label to automatically backport to the 1./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@dliappis dliappis changed the title Fix on top of 523 fix(go/base): set GOTOOLCHAIN=local and skip building arm using the base Dockerfile Feb 28, 2025
@dliappis dliappis requested a review from andrewkroh February 28, 2025 11:39
@dliappis dliappis self-assigned this Feb 28, 2025
@dliappis dliappis marked this pull request as ready for review February 28, 2025 11:39
@dliappis dliappis requested a review from a team as a code owner February 28, 2025 11:39
@dliappis
Copy link
Contributor Author

dliappis commented Feb 28, 2025

To help with reviews:

this PR builds on top of #523 and makes an additional change to address the problem mentioned in https://github.com/elastic/golang-crossbuild/pull/524/files#diff-78f24cf6131fffe363db9182e23c8e5ff491034e44c49dfc5678bb85ed5146e8L1

Using this PR, docker.elastic.co/observability-ci/golang-crossbuild:1.24.0-base-arm-debian9 gets built using the arm64 debian 9 step and doesn't get overwritten by the x86_64 debian 9 step.

The change has been tested by the image produced in the 1.23 branch via #527 ; using #527 me and @v1v tested the CI produced image docker.elastic.co/observability-ci/golang-crossbuild:1.23.6-base-arm-debian9 in the beats PR that bumps go to 1.23.6 -> elastic/beats#42960

swiatekm
swiatekm previously approved these changes Feb 28, 2025
Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM

v1v
v1v previously approved these changes Feb 28, 2025
andrewkroh
andrewkroh previously approved these changes Feb 28, 2025
@dliappis dliappis dismissed stale reviews from andrewkroh, v1v, and swiatekm via 7cae477 March 3, 2025 13:58
@dliappis dliappis requested a review from a team as a code owner March 3, 2025 14:03
@dliappis dliappis force-pushed the fix-on-top-of-523 branch from 8b4a1e3 to e665ff1 Compare March 3, 2025 14:22
@dliappis dliappis requested a review from pchila March 3, 2025 15:40
v1v
v1v previously approved these changes Mar 3, 2025
Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

LGTM

@dliappis
Copy link
Contributor Author

dliappis commented Mar 3, 2025

Reviewers/codeowners, I had to push a change that adds a buildkite step for the fpm image so that we have an arm64 native version of the fpm docker image: 4822107 ; with that done we could also test elastic-agent packaging steps using the arm64 staging image (docker.elastic.co/observability-ci/fpm:1.13.1) for fpm.

Everything else remains the same in this PR, so please take another look cc @andrewkroh / @swiatekm / @pazone

pazone
pazone previously approved these changes Mar 3, 2025
Copy link
Contributor

@pazone pazone left a comment

Choose a reason for hiding this comment

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

LGTM. Will update the elastic-agent PR as soon as this one is merged

v1v
v1v previously approved these changes Mar 3, 2025
pchila
pchila previously approved these changes Mar 3, 2025
Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

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

LGTM

@dliappis dliappis dismissed stale reviews from pchila, v1v, and pazone via 12d53c2 March 3, 2025 18:00
@dliappis
Copy link
Contributor Author

dliappis commented Mar 3, 2025

Reviewers/codeowners, I had to push a change that adds a buildkite step for the fpm image so that we have an arm64 native version of the fpm docker image: 4822107 ; with that done we could also test elastic-agent packaging steps using the arm64 staging image (docker.elastic.co/observability-ci/fpm:1.13.1) for fpm.

Everything else remains the same in this PR, so please take another look cc @andrewkroh / @swiatekm / @pazone

Apologies, I had to revert this; the commit 4822107 didn't properly add the -arm variant as a multiarch image, so the arm64 version of docker.elastic.co/observability-ci/fpm:1.13.1 ended up overwriting the amd64 version of docker.elastic.co/observability-ci/fpm:1.13.1.

@pazone we talked with @pchila about what this means for your PR and he explained that we don't need to have dedicated PR based packaging steps for deb+rpm on arm64 -- it's enough to only test those two on amd64, as it's been the case until now.

jlind23
jlind23 previously approved these changes Mar 3, 2025
@elasticmachine
Copy link

💚 Build Succeeded

History

cc @dliappis

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @dliappis

@elasticmachine
Copy link

elasticmachine commented Mar 3, 2025

@dliappis dliappis merged commit afb476a into elastic:main Mar 4, 2025
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants