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(docker): fix base image for multi-platform build #2099

Merged
merged 8 commits into from
Mar 2, 2022
Merged

fix(docker): fix base image for multi-platform build #2099

merged 8 commits into from
Mar 2, 2022

Conversation

Tenzer
Copy link
Contributor

@Tenzer Tenzer commented Mar 2, 2022

This work was inspired by getting the Atlantis Docker image to work on ARM architectures (#2001).

I noticed the base image installs binaries for the amd64 architecture and doesn't take into account what architecture the image actually is intended to be built for, so this fixes that.

I've also done a bit of tidying of the Dockerfile at the same time. Everything is split into separate commits so it should be easier to review.

Tenzer added 8 commits March 2, 2022 10:02
…uild time

This allows us to now repeat the packages which need to be uninstalled again by
making use of a virtual package, which - when removed - removes the packages
installed as a dependency of it.
It's no needed when `apt add` is run with the `--no-cache` option.
This can help find issues where binaries are downloaded for the wrong platform
compared to the architecture the Docker image is built for.
It's available as a package for Alpine Linux in version 1.2.5 as well, which
makes it easier to handle for the different architectures.
This makes use of the `TARGETPLATFORM` argument which automatically is
populated by Docker BuildKit with a string such as "linux/amd64" when the image
is being build for an x86_64 architecture.
The `case` statement was taken from
https://github.com/BretFisher/multi-platform-docker-build as a way of
translating the platform name into what we needed for downloading gosu.
case ${TARGETPLATFORM} in \
"linux/amd64") GOSU_ARCH=amd64 ;; \
"linux/arm64") GOSU_ARCH=arm64 ;; \
"linux/arm/v7") GOSU_ARCH=armhf ;; \
Copy link
Member

Choose a reason for hiding this comment

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

Would this armhf work for linux/arm/v7

Looks like the image does not get built.
https://github.com/tianon/gosu/blob/4c33a2fd492a38f1a3ff0bb773cd7ef2c32bef6b/Dockerfile#L42-L45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a lot of experience with the armhf/armel variants and what they equate to, but it does seem like the armhf binary is built here:
https://github.com/tianon/gosu/blob/23e63902e1ade697a167e7874a5a12243ac0bae8/Dockerfile#L38-L40

I'm guessing it's compatible with an ARMv7 architecture, but I don't have a way of testing that myself, so this is a bit of an opportunistic guess, although I hadn't seen how the files were built before this, and I just saw there was binaries available on the releases page.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I think we might need to try it ourselves.

As you can see in here,GOARM=7 is for armv7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, I doubt it's any more wrong than using the binary for amd64 :)

Copy link
Member

Choose a reason for hiding this comment

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

haha, that is true.

@chenrui333 chenrui333 changed the title Fixes to the base image fix(docker): fix base image for multi-platform build Mar 2, 2022
@@ -17,21 +17,35 @@ RUN addgroup atlantis && \
chmod g=u /home/atlantis/ && \
chmod g=u /etc/passwd

# Install dumb-init, gosu and git-lfs.
ENV DUMB_INIT_VERSION=1.2.5
Copy link
Member

Choose a reason for hiding this comment

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

good call 👍

@chenrui333 chenrui333 merged commit 571543f into runatlantis:master Mar 2, 2022
@chenrui333
Copy link
Member

Thanks @Tenzer!

@Tenzer Tenzer deleted the base-image-fixes branch March 2, 2022 16:15
@chenrui333
Copy link
Member

The merge build actually failed in CI, but I can actually build in my local (on m1), I will need to check a bit later.

Did it work for your local as well?

@Tenzer
Copy link
Contributor Author

Tenzer commented Mar 2, 2022

Doh! I just realised that ${TARGETPLATFORM##*/} with $TARGETPLATFORM set to linux/arm/v7 results in v7, so that fails in the image build: https://github.com/runatlantis/atlantis/runs/5394563734?check_suite_focus=true. I'll try and make a fix.

@chenrui333
Copy link
Member

cool, thanks!

@Tenzer
Copy link
Contributor Author

Tenzer commented Mar 2, 2022

#2100 should fix it.

chenrui333 added a commit that referenced this pull request Mar 3, 2022
chenrui333 added a commit that referenced this pull request Mar 3, 2022
* debug setup

* Revert "fix(docker): download Terraform and conftest versions maching image architecture (#2101)"

This reverts commit 579e583.

* Revert "fix(docker): fix installation of git-lfs in armv7 image (#2100)"

This reverts commit 8af7883.

* Revert "fix(docker): fix base image for multi-platform build (#2099)"

This reverts commit 571543f.

* Revert "debug setup"

This reverts commit 274501a.
chenrui333 pushed a commit that referenced this pull request Mar 4, 2022
* fix(docker): fix base image for multi-platform build (#2099)

* Correct indentation of run commands

* Split installation of packages into the ones needed at run time and build time

This allows us to now repeat the packages which need to be uninstalled again by
making use of a virtual package, which - when removed - removes the packages
installed as a dependency of it.

* Remove unnecessary `rm -rf /var/cache/apk/*` command

It's no needed when `apt add` is run with the `--no-cache` option.

* Add vertical spacing so it's clearer what is happening when

* Test the downloaded binaries to make sure they work on the platform

This can help find issues where binaries are downloaded for the wrong platform
compared to the architecture the Docker image is built for.

* Install dumb-init via apk

It's available as a package for Alpine Linux in version 1.2.5 as well, which
makes it easier to handle for the different architectures.

* Get git-lfs binaries in the right architecture for the Docker image

This makes use of the `TARGETPLATFORM` argument which automatically is
populated by Docker BuildKit with a string such as "linux/amd64" when the image
is being build for an x86_64 architecture.

* Install gosu for the right architecture

The `case` statement was taken from
https://github.com/BretFisher/multi-platform-docker-build as a way of
translating the platform name into what we needed for downloading gosu.

* fix(docker): fix installation of git-lfs in armv7 image (#2100)

This uses a similar pattern than what is used for `GOSU_ARCH` to map the
`TARGETPLATFORM` argument into the name of the architecture git-lfs use for
their release binaries, as "linux/arm/v7" otherwise would be mapped into "v7"
which is wrong.

* fix(docker): download Terraform and conftest versions maching image architecture (#2101)

* Remove Terraform versions from Docker image which don't have all archs

Terraform versions earlier than 0.11.15 does not have binaries available for
both amd64, arm64 and armv7, so are being dropped as we can't install the older
versions in all the architectures the Docker image is built for.

* Download Terraform version depending on the architecture Docker image is for

This avoids us having arm64 binaries for the ARM Docker images, which won't
work.

* Download arm64 conftest binaries for arm64 Docker image

This doesn't fix the armv7 Docker image because conftest doesn't have a binary
available for that, so it for now still downloads the x86_64 binary, which is
likely to not work - but it's the same as it did before.

* Correct path to dumb-init in docker-entrypoint.sh

The path changed after dumb-init was switched to be installed via `apk` rather
than downloaded directly as a binary.
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
* Correct indentation of run commands

* Split installation of packages into the ones needed at run time and build time

This allows us to now repeat the packages which need to be uninstalled again by
making use of a virtual package, which - when removed - removes the packages
installed as a dependency of it.

* Remove unnecessary `rm -rf /var/cache/apk/*` command

It's no needed when `apt add` is run with the `--no-cache` option.

* Add vertical spacing so it's clearer what is happening when

* Test the downloaded binaries to make sure they work on the platform

This can help find issues where binaries are downloaded for the wrong platform
compared to the architecture the Docker image is built for.

* Install dumb-init via apk

It's available as a package for Alpine Linux in version 1.2.5 as well, which
makes it easier to handle for the different architectures.

* Get git-lfs binaries in the right architecture for the Docker image

This makes use of the `TARGETPLATFORM` argument which automatically is
populated by Docker BuildKit with a string such as "linux/amd64" when the image
is being build for an x86_64 architecture.

* Install gosu for the right architecture

The `case` statement was taken from
https://github.com/BretFisher/multi-platform-docker-build as a way of
translating the platform name into what we needed for downloading gosu.
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
* debug setup

* Revert "fix(docker): download Terraform and conftest versions maching image architecture (runatlantis#2101)"

This reverts commit 579e583.

* Revert "fix(docker): fix installation of git-lfs in armv7 image (runatlantis#2100)"

This reverts commit 8af7883.

* Revert "fix(docker): fix base image for multi-platform build (runatlantis#2099)"

This reverts commit 571543f.

* Revert "debug setup"

This reverts commit 274501a.
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
* fix(docker): fix base image for multi-platform build (runatlantis#2099)

* Correct indentation of run commands

* Split installation of packages into the ones needed at run time and build time

This allows us to now repeat the packages which need to be uninstalled again by
making use of a virtual package, which - when removed - removes the packages
installed as a dependency of it.

* Remove unnecessary `rm -rf /var/cache/apk/*` command

It's no needed when `apt add` is run with the `--no-cache` option.

* Add vertical spacing so it's clearer what is happening when

* Test the downloaded binaries to make sure they work on the platform

This can help find issues where binaries are downloaded for the wrong platform
compared to the architecture the Docker image is built for.

* Install dumb-init via apk

It's available as a package for Alpine Linux in version 1.2.5 as well, which
makes it easier to handle for the different architectures.

* Get git-lfs binaries in the right architecture for the Docker image

This makes use of the `TARGETPLATFORM` argument which automatically is
populated by Docker BuildKit with a string such as "linux/amd64" when the image
is being build for an x86_64 architecture.

* Install gosu for the right architecture

The `case` statement was taken from
https://github.com/BretFisher/multi-platform-docker-build as a way of
translating the platform name into what we needed for downloading gosu.

* fix(docker): fix installation of git-lfs in armv7 image (runatlantis#2100)

This uses a similar pattern than what is used for `GOSU_ARCH` to map the
`TARGETPLATFORM` argument into the name of the architecture git-lfs use for
their release binaries, as "linux/arm/v7" otherwise would be mapped into "v7"
which is wrong.

* fix(docker): download Terraform and conftest versions maching image architecture (runatlantis#2101)

* Remove Terraform versions from Docker image which don't have all archs

Terraform versions earlier than 0.11.15 does not have binaries available for
both amd64, arm64 and armv7, so are being dropped as we can't install the older
versions in all the architectures the Docker image is built for.

* Download Terraform version depending on the architecture Docker image is for

This avoids us having arm64 binaries for the ARM Docker images, which won't
work.

* Download arm64 conftest binaries for arm64 Docker image

This doesn't fix the armv7 Docker image because conftest doesn't have a binary
available for that, so it for now still downloads the x86_64 binary, which is
likely to not work - but it's the same as it did before.

* Correct path to dumb-init in docker-entrypoint.sh

The path changed after dumb-init was switched to be installed via `apk` rather
than downloaded directly as a binary.
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.

2 participants