Enable multi-arch build for cilium-cli image #2782

marcofranssen · 2024-09-02T08:36:22Z

This PR builds on top of #2842

It also includes the cilium-cli target to the build using multi-arch.

In the PR we levarage some buildx features like --cache and --link to improve the speed of builds for multi-arch.

Resolves #2780

maintainer-s-little-helper · 2024-09-02T08:36:25Z

Commits 42d8aa8, ed8ea68 do not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

aanm

@michi-covalent @tklauser with the merge of cilium-cli into cilium/cilium it's becoming confusing to where contributors can submit their changes for cilium-cli.

aanm · 2024-09-03T07:59:37Z

Dockerfile

+# when bumping to a new version analyze the new version for security issues
+# then use crane to lookup the digest of that version so we are immutable
+# crane digest tonistiigi/xx:1.5.0
+FROM --platform=$BUILDPLATFORM tonistiigi/xx:1.5.0@sha256:0c6a569797744e45955f39d4f7538ac344bfb7ebf0a54006a0a4297b153ccf0f AS xx


Is this really necessary? It shouldn't be since we cross compile cilium without tonistiigi/xx as an example.

This is required within Docker, to compile with CGO_ENABLED=0. Don't recall the exact reason, I think it was related to alpine based builds (muslc, vs glibc).

I also use some buildx features that allow caching and parallel builds for all the supported platforms to have a speedy and optimized build.

I contributed similar to the Spire project couple of years ago.

In essence the tool does following and is basically a small little wrapper.

https://github.com/tonistiigi/xx?tab=readme-ov-file#go--cgo

It wraps the go command by setting some variables accordingly based on the buildx args.

Saves a lot of plumbing.

re-requested review from andre. let's see what he says 🙏

We can still use the caches without using this dependency. Personally, I would prefer to not have any wrappers on official builds.

this isn't related to the caching only. It is also to set the go flags accordingly for crosscompile based on the TARGETPLATFORM and TARGETARCH which is a buildx feature.

It is used here https://github.com/cilium/cilium-cli/pull/2782/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R19

Without that we would have to do all the plumbing for that in the RUN statement. I'm not a fan of reinventing the wheel :)

marcofranssen · 2024-09-04T09:18:09Z

@michi-covalent @tklauser with the merge of cilium-cli into cilium/cilium it's becoming confusing to where contributors can submit their changes for cilium-cli.

Yes, now I'm confused 😕

Should I create such PR on the cilium repo?

Maybe this repo has to be archived?

Let me know how to proceed.

michi-covalent · 2024-09-05T20:52:36Z

with the merge of cilium-cli into cilium/cilium it's becoming confusing to where contributors can submit their changes for cilium-cli.

sorry for the delay, we build release artifacts from this repo. assuming we want to build this docker image for each release tag, this is the right repo for this pull request.

@marcofranssen let's limit this pull request to Dockerfile changes only. once it gets approved & merged, i'll make necessary changes to github workflows.

Dockerfile.ci

marcofranssen · 2024-11-11T09:51:44Z

Builds on top of #2842 to reduce the scope of this PR. Can we please move forward with both. These PRs are out there now for several weeks without progress on a review of these relative simple PRs. I'm waiting to be able to use the improvements.

tklauser · 2024-11-12T08:08:34Z

@marcofranssen this PR picked up a merge conflict and needs a rebase.

marcofranssen · 2024-11-12T11:35:03Z

@marcofranssen this PR picked up a merge conflict and needs a rebase.

Done

marcofranssen · 2024-11-12T11:36:21Z

Dockerfile

+    --mount=type=cache,target=/go/pkg/mod \
+    xx-go --wrap && \
+    make && \
+    xx-verify --static /go/src/github.com/cilium/cilium-cli/cilium


It also allows verifying the binary, see here.

marcofranssen · 2024-11-12T11:36:52Z

Dockerfile

+COPY --link --from=xx / /
+RUN --mount=type=cache,target=/root/.cache/go-build \
+    --mount=type=cache,target=/go/pkg/mod \
+    xx-go --wrap && \


Small little wrapper that sets the correct go build flags and such based on TARGETPLATFORM and TARGETARCH and such.

marcofranssen · 2024-11-12T11:37:11Z

Dockerfile

-COPY --from=builder --chown=root:root --chmod=755 /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
-COPY --from=builder /go/src/github.com/cilium/cilium-cli/cilium /usr/local/bin/cilium
+COPY --link --from=builder --chown=root:root --chmod=755 /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
+COPY --link --from=builder --chown=1000:1000 --chmod=755 /go/src/github.com/cilium/cilium-cli/cilium /usr/local/bin/cilium


Run as non root

.github/workflows/images.yaml

marcofranssen · 2024-11-20T19:07:06Z

Rebased the branch again to resolve conflicts

marcofranssen · 2024-11-25T12:17:25Z

Did another rebase to get branch fully in sync with upstream

michi-covalent

LGTM overall, only one feedback regarding when to push to the non-CI repo.

pinging @aanm for re-review 🏓

michi-covalent · 2024-11-25T15:07:29Z

.github/workflows/images.yaml

@@ -57,10 +60,10 @@ jobs:
          ref: ${{ steps.tag.outputs.tag }}

      # main branch or tag pushes
-      - name: CI Build ${{ matrix.name }}
+      - name: Build ${{ matrix.name }}
        if: ${{ github.event_name != 'pull_request_target' }}


let's run step only on tag push, not branch push. https://quay.io/repository/cilium/cilium-cli?tab=tags should only contain released images 🙏

Suggested change

if: ${{ github.event_name != 'pull_request_target' }}

if: ${{ github.ref_type == 'tag' }}

@michi-covalent maybe github.ref_type == 'tag' && github.event_name == 'tag'?

This was never changed. Happy to change it, but didn't want to modify the current pipeline behavior.

Let me know and I can perform the update.

I'm not even sure if you want to change it, because this would allow the release of the image on merges to main using the commit hash.

See https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR48-R57

So changing this would change the existing behavior as that step output is used during the Build here https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR99

aanm

@michi-covalent We recently changed the cilium-cli Dockerfile in cilium/cilium to add support for multi-arch. Can't we replace cilium/cilium-cli Dockerfile with the on we have in cilium/cilium?

@marcofranssen sorry for taking so much time to review the PR. I needed some time to think about it.

aanm · 2024-11-27T15:50:42Z

.github/workflows/images.yaml

@@ -25,6 +25,9 @@ jobs:
    strategy:


On line 23 we will need to distinguish between environment: ci and environment: release. The "release" environment should only happen on push tags and the "ci" environment for everything else.

Why would you need a split for the environment?

I guess you have different secret for pushing the image?

If it needs a split we need to include the env name in the matrix. Let me know, I will update.

Updated see last commit.

aanm · 2024-11-27T15:51:21Z

.github/workflows/images.yaml

@@ -57,10 +60,10 @@ jobs:
          ref: ${{ steps.tag.outputs.tag }}

      # main branch or tag pushes
-      - name: CI Build ${{ matrix.name }}
+      - name: Build ${{ matrix.name }}
        if: ${{ github.event_name != 'pull_request_target' }}


@michi-covalent maybe github.ref_type == 'tag' && github.event_name == 'tag'?

aanm · 2024-11-27T15:55:25Z

Dockerfile

+# when bumping to a new version analyze the new version for security issues
+# then use crane to lookup the digest of that version so we are immutable
+# crane digest tonistiigi/xx:1.5.0
+FROM --platform=$BUILDPLATFORM tonistiigi/xx:1.5.0@sha256:0c6a569797744e45955f39d4f7538ac344bfb7ebf0a54006a0a4297b153ccf0f AS xx


We can still use the caches without using this dependency. Personally, I would prefer to not have any wrappers on official builds.

aanm · 2024-11-27T15:58:35Z

Dockerfile


 # cilium-cli-ci is based on ubuntu with cloud CLIs
 FROM ubuntu:24.04@sha256:99c35190e22d294cdace2783ac55effc69d32896daaa265f0bbedbcde4fbe3e5 AS cilium-cli-ci
 ENTRYPOINT []
 LABEL maintainer="[email protected]"
 WORKDIR /root/app
-COPY --from=builder /go/src/github.com/cilium/cilium-cli/cilium /usr/local/bin/cilium
+COPY --link --from=builder /go/src/github.com/cilium/cilium-cli/cilium /usr/local/bin/cilium

 # Install cloud CLIs. Based on these instructions:


Similar to https://github.com/cilium/cilium/blob/78b2e1a189bccd7c863715d3f7708f8fdab1cf59/cilium-cli/Dockerfile, I don't think this will work because, for example, awscli, downloaded on line 53, is for x86_64 only.

I can tell you it does work.

https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR33

The ci image is only build for amd64. If you want to add arm64 support for that image I'm happy to another PR for that.

Signed-off-by: Marco Franssen <[email protected]>

marcofranssen · 2024-11-27T16:05:36Z

Rebased PR again to resolve the merge conflict.

maintainer-s-little-helper · 2024-11-27T16:19:17Z

Commit b9d0135 does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

marcofranssen requested review from a team as code owners September 2, 2024 08:36

marcofranssen requested review from aanm and christarazi September 2, 2024 08:36

marcofranssen requested a review from derailed September 2, 2024 08:36

maintainer-s-little-helper bot added the dont-merge/needs-sign-off label Sep 2, 2024

marcofranssen had a problem deploying to ci September 2, 2024 08:36 — with GitHub Actions Error

marcofranssen force-pushed the cilium-cli-image branch from ed8ea68 to 0b948bf Compare September 2, 2024 08:38

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off label Sep 2, 2024

marcofranssen temporarily deployed to ci September 2, 2024 08:38 — with GitHub Actions Inactive

marcofranssen force-pushed the cilium-cli-image branch from 0b948bf to e06aa0d Compare September 2, 2024 08:46

marcofranssen temporarily deployed to ci September 2, 2024 08:46 — with GitHub Actions Inactive

marcofranssen force-pushed the cilium-cli-image branch from e06aa0d to 2bf1e74 Compare September 2, 2024 08:49

marcofranssen had a problem deploying to ci September 2, 2024 08:50 — with GitHub Actions Error

marcofranssen force-pushed the cilium-cli-image branch from 2bf1e74 to ed22c94 Compare September 2, 2024 08:50

marcofranssen temporarily deployed to ci September 2, 2024 08:50 — with GitHub Actions Inactive

marcofranssen force-pushed the cilium-cli-image branch from ed22c94 to 8b4242d Compare September 2, 2024 09:04

marcofranssen temporarily deployed to ci September 2, 2024 09:04 — with GitHub Actions Inactive

aanm requested changes Sep 3, 2024

View reviewed changes

aanm requested review from tklauser and michi-covalent September 3, 2024 08:01

michi-covalent reviewed Sep 5, 2024

View reviewed changes

Dockerfile.ci Outdated Show resolved Hide resolved

tklauser removed their request for review September 11, 2024 09:02

christarazi marked this pull request as draft September 30, 2024 17:35

marcofranssen force-pushed the cilium-cli-image branch from 8b4242d to 1a22c85 Compare October 29, 2024 07:47

marcofranssen temporarily deployed to ci October 29, 2024 07:47 — with GitHub Actions Inactive

marcofranssen force-pushed the cilium-cli-image branch from f1f64f7 to 6e217b5 Compare November 11, 2024 09:46

marcofranssen temporarily deployed to ci November 11, 2024 09:46 — with GitHub Actions Inactive

marcofranssen marked this pull request as ready for review November 11, 2024 09:50

tklauser added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 12, 2024

marcofranssen force-pushed the cilium-cli-image branch from 6e217b5 to 4c1a5a2 Compare November 12, 2024 11:34

marcofranssen temporarily deployed to ci November 12, 2024 11:34 — with GitHub Actions Inactive

marcofranssen commented Nov 12, 2024

View reviewed changes

.github/workflows/images.yaml Show resolved Hide resolved

marcofranssen force-pushed the cilium-cli-image branch from 4c1a5a2 to f73e9f6 Compare November 20, 2024 19:06

marcofranssen temporarily deployed to ci November 20, 2024 19:06 — with GitHub Actions Inactive

marcofranssen force-pushed the cilium-cli-image branch from f73e9f6 to 0a6057e Compare November 25, 2024 12:17

marcofranssen temporarily deployed to ci November 25, 2024 12:17 — with GitHub Actions Inactive

michi-covalent reviewed Nov 25, 2024

View reviewed changes

tklauser removed the needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 27, 2024

aanm requested changes Nov 27, 2024

View reviewed changes

Enable multi-arch Docker build

e5221c1

Signed-off-by: Marco Franssen <[email protected]>

marcofranssen force-pushed the cilium-cli-image branch from 0a6057e to e5221c1 Compare November 27, 2024 16:04

marcofranssen temporarily deployed to ci November 27, 2024 16:05 — with GitHub Actions Inactive

Use different GitHub environment for the images

b9d0135

maintainer-s-little-helper bot added the dont-merge/needs-sign-off label Nov 27, 2024

marcofranssen temporarily deployed to ci November 27, 2024 16:19 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multi-arch build for cilium-cli image #2782

Enable multi-arch build for cilium-cli image #2782

marcofranssen commented Sep 2, 2024 •

edited

Loading

maintainer-s-little-helper bot commented Sep 2, 2024

aanm left a comment

aanm Sep 3, 2024

marcofranssen Sep 4, 2024 •

edited

Loading

marcofranssen Oct 29, 2024

michi-covalent Oct 29, 2024

aanm Nov 27, 2024

marcofranssen Nov 27, 2024 •

edited

Loading

marcofranssen commented Sep 4, 2024

michi-covalent commented Sep 5, 2024

marcofranssen commented Nov 11, 2024

tklauser commented Nov 12, 2024

marcofranssen commented Nov 12, 2024

marcofranssen Nov 12, 2024

marcofranssen Nov 12, 2024

marcofranssen Nov 12, 2024

marcofranssen commented Nov 20, 2024

marcofranssen commented Nov 25, 2024

michi-covalent left a comment

michi-covalent Nov 25, 2024

aanm Nov 27, 2024

marcofranssen Nov 27, 2024

marcofranssen Nov 27, 2024 •

edited

Loading

aanm left a comment

aanm Nov 27, 2024

marcofranssen Nov 27, 2024 •

edited

Loading

marcofranssen Nov 27, 2024

aanm Nov 27, 2024

aanm Nov 27, 2024

aanm Nov 27, 2024

marcofranssen Nov 27, 2024 •

edited

Loading

marcofranssen commented Nov 27, 2024

maintainer-s-little-helper bot commented Nov 27, 2024

	if: ${{ github.event_name != 'pull_request_target' }}
	if: ${{ github.ref_type == 'tag' }}

Enable multi-arch build for cilium-cli image #2782

Are you sure you want to change the base?

Enable multi-arch build for cilium-cli image #2782

Conversation

marcofranssen commented Sep 2, 2024 • edited Loading

maintainer-s-little-helper bot commented Sep 2, 2024

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

marcofranssen commented Sep 4, 2024

michi-covalent commented Sep 5, 2024

marcofranssen commented Nov 11, 2024

tklauser commented Nov 12, 2024

marcofranssen commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen commented Nov 20, 2024

marcofranssen commented Nov 25, 2024

michi-covalent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

marcofranssen commented Nov 27, 2024

maintainer-s-little-helper bot commented Nov 27, 2024

marcofranssen commented Sep 2, 2024 •

edited

Loading

marcofranssen Sep 4, 2024 •

edited

Loading

marcofranssen Nov 27, 2024 •

edited

Loading

marcofranssen Nov 27, 2024 •

edited

Loading

marcofranssen Nov 27, 2024 •

edited

Loading

marcofranssen Nov 27, 2024 •

edited

Loading