-
Notifications
You must be signed in to change notification settings - Fork 209
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
base: main
Are you sure you want to change the base?
Conversation
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 |
ed8ea68
to
0b948bf
Compare
0b948bf
to
e06aa0d
Compare
e06aa0d
to
2bf1e74
Compare
2bf1e74
to
ed22c94
Compare
ed22c94
to
8b4242d
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙏
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. |
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. |
8b4242d
to
1a22c85
Compare
8ff4a28
to
3b7f584
Compare
3b7f584
to
cafcdf6
Compare
.github/workflows/images.yaml
Outdated
platforms: linux/amd64,linux/arm64 | ||
- name: cilium-cli-ci | ||
dockerfile: ./Dockerfile | ||
platforms: linux/amd64 | ||
|
There was a problem hiding this comment.
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 🚀🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michi-covalent maybe merge this one first #2842 This PR builds on top of that and reduces the scope of this PR. |
cafcdf6
to
f1f64f7
Compare
f1f64f7
to
6e217b5
Compare
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. |
@marcofranssen this PR picked up a merge conflict and needs a rebase. |
6e217b5
to
4c1a5a2
Compare
Done |
--mount=type=cache,target=/go/pkg/mod \ | ||
xx-go --wrap && \ | ||
make && \ | ||
xx-verify --static /go/src/github.com/cilium/cilium-cli/cilium |
There was a problem hiding this comment.
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 && \ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 }} |
There was a problem hiding this comment.
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]>
4c1a5a2
to
f73e9f6
Compare
Rebased the branch again to resolve conflicts |
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