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-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

the latest workflow file looks good to me. just wanted to make sure we don't push to quay.io/cilium/cilium-cli on push to main branch 🚀🙏

hmm don't we still end up pushing to quay.io/cilium/cilium-cli repo on push to main?

cilium-cli/.github/workflows/images.yaml

Lines 64 to 77 in f996cce

# main branch or tag pushes

- name: Build ${{ matrix.name }}

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

uses: docker/build-push-action@48aba3b46d1b1fec4febb7c5d0c644b249a11355 # v6.10.0

id: docker_build_main

with:

context: .

file: ${{ matrix.dockerfile }}

target: ${{ matrix.name }}

platforms: ${{ matrix.platforms }}

push: true

tags: |

quay.io/${{ github.repository_owner }}/${{ matrix.name }}:latest

quay.io/${{ github.repository_owner }}/${{ matrix.name }}:${{ steps.tag.outputs.tag }}

💭

i suggest keeping this pull request for changing Dockerfile only. let's forcus on getting Dockerfile change merged in this PR. @tklauser and i will set up a release image workflow. we need to create a separate github environment anyways.

we'll do something like this for the release image workflow once the Dockerfile change gets merged => c6cfc8b

@michi-covalent the separete github env is already created based on earlier feedback.

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

i didn't mean to update the workflow file to refer to a release environment. i need to configure a github environment for this repo and make sure that it works.

let's limit the scope of this pull request to the Dockerfile change 🙏

Without the workflow it won't work right. Hence #2782 (comment)

The Github environment you have to configure basically needs to match the name here https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR31

Guess the only thing you need to do is configure the secrets for publishing the image int there.

Why don't you add the environment, (give it a different name to your desires) and then we update it here in the workflow.

I'm not fully getting the problem with the workflow here. Basically all required to use a different gh-env is in place. You only need to add the secret to it.

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.

.github/workflows/images.yaml

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.

What I meant was that it will not work for arm64 because of the binary downloaded on line 53. The build will work but the resulting image will not work for arm64.

Let me state it differently, because we are not building this image for arm64 in the workflow, it won't be a problem until we change that in the workflow.

For now this PR only focusses on building the cilium image in both architectures, the ci image will stay with amd64 support only.

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

maintainer-s-little-helper · 2024-12-07T12:02:16Z

Commit 553f2d9 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

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

maintainer-s-little-helper · 2024-12-07T12:02:52Z

Commit 553f2d9 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 · 2024-12-07T12:03:35Z

Rebased once more and signed off the one commit I forgot about.

michi-covalent · 2024-12-08T00:13:44Z

reminder for @tklauser and myself: set up release environment before the next release 🚀

michi-covalent · 2024-12-08T00:20:38Z

ping @cilium/ci-structure for review 🚀🙏

edit: never mind, i requested to update this PR to only modify Dockerfile 👍 #2782 (comment)

aanm

Also, can't we replace cilium/cilium-cli Dockerfile with the on we have in cilium/cilium? It seems this remains unanswered.

aanm · 2024-12-09T11:52: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:


What I meant was that it will not work for arm64 because of the binary downloaded on line 53. The build will work but the resulting image will not work for arm64.

marcofranssen · 2024-12-16T19:02:47Z

What should I do to get this merged? I adjusted the workflow to use a separate GH environment for the release image? Went trough the open conversations but not sure how to progress now.

I feel all required changes are in.

tklauser · 2024-12-17T10:54:31Z

What should I do to get this merged? I adjusted the workflow to use a separate GH environment for the release image? Went trough the open conversations but not sure how to progress now.

There are still two open questions from @aanm which AFAICT haven't been addressed or answered:
#2782 (review) and #2782 (comment)

marcofranssen · 2024-12-17T12:23:15Z

Also, can't we replace cilium/cilium-cli Dockerfile with the on we have in cilium/cilium? It seems this remains unanswered.

In a previous PR I contributed this was also already the question. It seems that is something to be figured out within the cilium repo owners.

In my previous PR it was decided to continue using this repo until a decision is made. Would be great if we can unblock this PR and move forward and figure out the other part in a follow-up.

marcofranssen · 2024-12-17T12:24:36Z

What should I do to get this merged? I adjusted the workflow to use a separate GH environment for the release image? Went trough the open conversations but not sure how to progress now.

There are still two open questions from @aanm which AFAICT haven't been addressed or answered: #2782 (review) and #2782 (comment)

I feel one was already answered, but gave it another shot with different wording.

The other one seems to be unrelated to this PR. It was already a discussion on a previous PR and there it was decided to not hold back community contribution while the cilium team figures it out internally on how and when to make that change.

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

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

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

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

aanm self-requested a review December 6, 2024 11:11

marcofranssen force-pushed the cilium-cli-image branch from b9d0135 to 553f2d9 Compare December 7, 2024 12:02

marcofranssen had a problem deploying to ci December 7, 2024 12:02 — with GitHub Actions Error

marcofranssen added 2 commits December 7, 2024 13:02

Enable multi-arch Docker build

aed83dc

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

Use different GitHub environment for the images

f996cce

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

marcofranssen force-pushed the cilium-cli-image branch from 553f2d9 to f996cce Compare December 7, 2024 12:02

marcofranssen temporarily deployed to ci December 7, 2024 12:03 — with GitHub Actions Inactive

michi-covalent approved these changes Dec 8, 2024

View reviewed changes

michi-covalent removed the dont-merge/needs-sign-off label Dec 8, 2024

michi-covalent mentioned this pull request Dec 8, 2024

Pr/michi/test ci image #2881

Closed

aanm requested changes Dec 9, 2024

View reviewed changes

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

	# main branch or tag pushes
	- name: Build ${{ matrix.name }}
	if: ${{ github.event_name != 'pull_request_target' }}
	uses: docker/build-push-action@48aba3b46d1b1fec4febb7c5d0c644b249a11355 # v6.10.0
	id: docker_build_main
	with:
	context: .
	file: ${{ matrix.dockerfile }}
	target: ${{ matrix.name }}
	platforms: ${{ matrix.platforms }}
	push: true
	tags: \|
	quay.io/${{ github.repository_owner }}/${{ matrix.name }}:latest
	quay.io/${{ github.repository_owner }}/${{ matrix.name }}:${{ steps.tag.outputs.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 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

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

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

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

marcofranssen commented Nov 27, 2024

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

maintainer-s-little-helper bot commented Dec 7, 2024

maintainer-s-little-helper bot commented Dec 7, 2024

marcofranssen commented Dec 7, 2024

michi-covalent commented Dec 8, 2024

michi-covalent commented Dec 8, 2024 • edited Loading

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcofranssen commented Dec 16, 2024

tklauser commented Dec 17, 2024

marcofranssen commented Dec 17, 2024

marcofranssen commented Dec 17, 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

michi-covalent commented Dec 8, 2024 •

edited

Loading