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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcofranssen
Copy link
Contributor

@marcofranssen marcofranssen commented Sep 2, 2024

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

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

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

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

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@marcofranssen marcofranssen Sep 4, 2024

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@marcofranssen
Copy link
Contributor Author

@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
Copy link
Contributor

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 Outdated Show resolved Hide resolved
Comment on lines 30 to 34
platforms: linux/amd64,linux/arm64
- name: cilium-cli-ci
dockerfile: ./Dockerfile
platforms: linux/amd64

Copy link
Contributor

@michi-covalent michi-covalent Oct 29, 2024

Choose a reason for hiding this comment

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

let's make this workflow change (pushing to quay.io/cilium/cilium-cli) in a separate pull request as a follow up. i (or @tklauser) needs to work with cilium maintainers to create new quay.io repo and such.

for this pull request, let's just push the multi arch image to quay.io/cilium/cilium-cli-ci.

edit: turned out https://quay.io/repository/cilium/cilium-cli?tab=settings already exists. we still need to set up proper credentials to push images to the repo 🚀🙏

Copy link
Contributor

@leppeK leppeK Nov 1, 2024

Choose a reason for hiding this comment

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

Am i wrong to think this is quite duplicate of what i was trying to achieve with #2755 and PR #2770? we already have the option to build multiarch with the changes there

@marcofranssen
Copy link
Contributor Author

@michi-covalent maybe merge this one first #2842

This PR builds on top of that and reduces the scope of this PR.

@marcofranssen
Copy link
Contributor Author

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 tklauser added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 12, 2024
@tklauser
Copy link
Member

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

@marcofranssen
Copy link
Contributor Author

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

Done

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

Choose a reason for hiding this comment

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

It also allows verifying the binary, see here.

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

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run as non root

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

# main branch or tag pushes
- name: CI Build ${{ matrix.name }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed because the wf now builds both the ci and plain cli image.

Signed-off-by: Marco Franssen <[email protected]>
@marcofranssen
Copy link
Contributor Author

Rebased the branch again to resolve conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase This PR needs to be rebased because it has merge conflicts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slim version of the cilium-cli image
5 participants