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

Fixed plugins using arm64 binaries in all container images #43031

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

fheinecke
Copy link
Contributor

@fheinecke fheinecke commented Jun 14, 2024

Due to a missing ARG directive + globbing, the build pulls in arm64 tarballs on top of amd64 tarballs, breaking amd64 images. This fixes the issue by adding the missing directive, and removing the globbing so that builds will fail rather than successfully produce broken images.

This will require an e ref bump after https://github.com/gravitational/teleport.e/pull/4408 lands. Passing build (for plugins at least) is here.

This is affecting the v16.0.0 release. I'll add a changelog entry to the backport PR.

@fheinecke fheinecke added backport-required no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Jun 14, 2024
@fheinecke fheinecke enabled auto-merge June 14, 2024 18:08
@fheinecke
Copy link
Contributor Author

Dev build is running here to confirm the fix.

@fheinecke fheinecke disabled auto-merge June 14, 2024 18:11
Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

We know the exact archive file name in the Makefile: $(RELEASE).tar/gz can we pass this as a docker build arg to get rid of the globbing that is fragile and can catch any other old release in the build/ directory?

@fheinecke
Copy link
Contributor Author

@hugoShaka No we cannot, at least not rewriting a large portion of the workflow that we use for building all images.

@hugoShaka
Copy link
Contributor

hugoShaka commented Jun 14, 2024

I saw that we're using the OCI Multiarch action for this. Can I suggest, after fixing this, adding the built version/image tag as a build arg in the action? Even if not every Dockerfile uses it, having a TELEPORT_VERSION args will be useful in many cases. Here we could be more specific about the globbing logic (we'd still need globbing, but at least we'd pick the right version, like this PR does for the arch)

@fheinecke
Copy link
Contributor Author

Yea I was looking at that. I think we might be able to avoid all globbing if we do what we're doing with distroless images, and build the file name inside the image. So instead of COPY *${TARGET_ARCH}* /path we'd have COPY teleport-${PLUGIN_NAME}-${VERSION}-${TARGETOS}-${TARGETARCH}-bin.tar.gz /path. This adds some additional coupling, but I think that this is probably reasonable.

I just ran a dev build where amd64 is in the tag name, which caused this exact same problem because the glob was not specific enough. This route is more of a PITA to implement as it requires twice as many PRs, but it's probably a safer approach.

@fheinecke fheinecke force-pushed the fred/fix-plugins-images-amd64-1 branch from a03a96d to 0a36ae9 Compare June 14, 2024 19:24
@fheinecke fheinecke requested a review from hugoShaka June 14, 2024 19:24
@fheinecke fheinecke enabled auto-merge June 17, 2024 09:25
@fheinecke fheinecke added this pull request to the merge queue Jun 17, 2024
Merged via the queue into master with commit 552e700 Jun 17, 2024
38 checks passed
@fheinecke fheinecke deleted the fred/fix-plugins-images-amd64-1 branch June 17, 2024 09:51
@public-teleport-github-review-bot

@fheinecke See the table below for backport results.

Branch Result
branch/v16 Failed

github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2024
…es (#43031) (#43084)

* Fixed plugins using arm64 binaries in all container images (#43031)

* bump e ref
@fheinecke fheinecke restored the fred/fix-plugins-images-amd64-1 branch August 20, 2024 08:18
fheinecke added a commit that referenced this pull request Aug 20, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 backport-required no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants