Skip to content

oci: Parameterize Debian OCI Dockerfile for multiarch#33949

Merged
camscale merged 2 commits intomasterfrom
camh/deb-oci
Nov 2, 2023
Merged

oci: Parameterize Debian OCI Dockerfile for multiarch#33949
camscale merged 2 commits intomasterfrom
camh/deb-oci

Conversation

@camscale
Copy link
Copy Markdown
Contributor

@camscale camscale commented Oct 26, 2023

Parameterize the Debian OCI Dockerfile to take a .deb filename without
an architecture so we can use ${TARGETARCH} in a multi-architecture
build. ${DEB_PATH} still has precedence, but if not specified,
${DEB_BASE} can be provided to specify the .deb file to install into
the image without the architecture or .deb suffix.

This also fixes the ${DEB_PATH?} expansion which is not a valid
expansion in a Dockerfile.

With this change, a multi-architecture image can be built with

docker buildx build --platform p1,p2,p3 \
  --build-arg DEB_BASE=teleport-ent-v1.2.3 \
  ...

and docker will set ${TARGETARCH} to the architecture of each of the
platforms p1, p2 and p3 as it builds each, having us install the
correct architecture of teleport .deb file.

Issue: #20729

Parameterize the Debian OCI Dockerfile to take a `.deb` filename without
an architecture so we can use `${TARGETARCH}` in a multi-architecture
build. `${DEB_PATH}` still has precedence, but if not specified,
`${DEB_BASE}` can be provided to specify the `.deb` file to install into
the image without the architecture or `.deb` suffix.

This also fixes the `${DEB_PATH?}` expansion which is not a valid
expansion in a Dockerfile.

With this change, a multi-architecture image can be built with

    docker buildx build --platform p1,p2,p3 \
      --build-arg DEB_BASE=teleport-ent-v1.2.3 \
      ...

and docker will set `${TARGETARCH}` to the architecture of each of the
platforms `p1`, `p2` and `p3` as it builds each, having us install the
correct architecture of teleport `.deb` file.
@camscale camscale added the no-changelog Indicates that a PR does not require a changelog entry label Oct 26, 2023
@github-actions github-actions Bot requested review from kimlisa and lxea October 26, 2023 20:49
@camscale camscale requested review from fheinecke, r0mant and tcsc and removed request for kimlisa and lxea October 26, 2023 20:50
Comment thread build.assets/charts/Dockerfile Outdated
COPY ${DEB_PATH?} /tmp/teleport.deb
ARG DEB_BASE
ARG TARGETARCH
COPY ${DEB_PATH:-${DEB_BASE}_${TARGETARCH}.deb} /tmp/teleport.deb
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Possibly out of scope for this, but is it worth pulling out this COPY & adding a mount point to the context dir so that we can install directly from there in the script?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's an option I guess, but I'm not sure it buys us much. We're still going to need to parameterise enough to select the right .deb file. I might be more inclined to pull the deb archive from the release server or S3 bucket inside the dockerfile.

But either way, this being a legacy component, I'll probably just leave it as is and let it disappear in 12 months when teleport 14 drops out of support (the last version we'll publish with legacy OCI images).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this removes a layer from the final image, which reduces the image size. Not 100% sure.

I might be more inclined to pull the deb archive from the release server or S3 bucket inside the dockerfile.

I had considered this when I rewrote this file. I think I chose not to do this because:

  • It is intended that developers can use this to build Teleport and produce an image from that build
  • It would have added some additional complexity to our Drone setup due to additional interdependencies

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I think I flaked a little bit working on this. I saw the "FROM ..." at the end and assumed a standard multi-stage build, but at the same time I knew it was only for setting the FIPS entrypoint. But this is leaving the /tmp/teleport.deb file behind in the image. It would be good not to do this, and that's what the volume mount would give us, perhaps using https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---mount RUN --mount. I'll try that out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tcsc @fheinecke I've reworked this to use a bind mount for the deb file so it is no longer copied in, "bloating" the image. I also separated the installation of stuff with apt-get and the installation of the teleport .deb. This will allow caching of the apt-get layer which I'll make use of in the workflow building OSS and Enterprise (and FIPS) in the same workflow sequentially. Since most is cached, the subsequent builds are quite fast. Thank you both for prompting me on this - this is much better now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since I've changed it materially, I would appreciate it if you could give it another look over.

Use `RUN --mount=target-/ctx` to mount the Docker build context to
access the `.deb` file to install instead of `COPY`ing it in. This saves
space on the image.

Separate the installation of the Teleport `.deb` file to come after
installing the prerequisites with `apt-get`. This allows that layer to
be cached so we can build the OSS and Enterprise images sharing that
base. This reduces the network traffic too and the risk of failure due
to transient network errors. The build time of a subsequent image is
seconds so we don't lose anything due to the removal of potential
parallelism.
@camscale camscale added this pull request to the merge queue Nov 2, 2023
Merged via the queue into master with commit cfc3480 Nov 2, 2023
@camscale camscale deleted the camh/deb-oci branch November 2, 2023 23:25
@public-teleport-github-review-bot
Copy link
Copy Markdown

@camscale See the table below for backport results.

Branch Result
branch/v12 Create PR
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants