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

Adding architecture support for arm/arm64/amd64 docker images #1781

Merged
merged 3 commits into from
Sep 27, 2020
Merged

Adding architecture support for arm/arm64/amd64 docker images #1781

merged 3 commits into from
Sep 27, 2020

Conversation

xunholy
Copy link
Contributor

@xunholy xunholy commented Aug 18, 2020

Adding architecture support for arm/arm64/amd64 docker images using docker buildx functionality.

Tested arm64 running on a local Kubernetes cluster to confirm it was working without issues.

Signed-off-by: Michael Fornaro 20387402+xUnholy@users.noreply.github.com

@xunholy xunholy marked this pull request as ready for review August 18, 2020 10:45
@xunholy
Copy link
Contributor Author

xunholy commented Aug 19, 2020

@sagikazarmark @JoelSpeed Despite the CI failure it's because my proposal also will change the Github workflow. It would be great to get this reviewed, I'm currently building and maintaining an unofficial image here: https://github.com/raspbernetes/multi-arch-images/blob/master/build/cloudflared/Dockerfile

However, my goal is to have this support in the upstream - Using Golang makes this somewhat trivial but happy to make any tweaks as you see necessary.

@sagikazarmark
Copy link
Member

I'll try to review this, but it's not on my high prio list.

My problem with these kind of changes (similar to connector changes) is that we don't have an easy way to test them to prevent regressions.

We can probably merge this PR, but we need to find a way to run tests on these architectures as well.

@xunholy
Copy link
Contributor Author

xunholy commented Aug 19, 2020

@sagikazarmark we could run a different tag and put this in a unique workflow and run both images to avoid breaking backward compatibility.

@JoelSpeed
Copy link
Contributor

Changes look sane from my perspective, though I share similar concerns about testing. I wonder if there is some way we can test test these images automatically so that we don't introduce regressions in the future?

@sagikazarmark
Copy link
Member

I'm looking at self-hosted runners for GitHub Actions. In theory, they support ARM, so we could at least compile and run tests against ARM, though I'm not sure the entire pipeline can run on ARM architecture.

CNCF could probably help with funding.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@xunholy
Copy link
Contributor Author

xunholy commented Sep 15, 2020

@bonifaido @sagikazarmark @JoelSpeed Thanks for reviewing the PR - slight adjustments to now use the official docker github actions to execute multi-arch support using buildx .

I've also used the native docker ARGS in the Dockerfile to simplify the logic.

@xunholy
Copy link
Contributor Author

xunholy commented Sep 15, 2020

Working & Tested Image here: https://hub.docker.com/r/raspbernetes/dex

Copy link
Member

@bonifaido bonifaido left a comment

Choose a reason for hiding this comment

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

TBH, I'm happy with the changes, we just need to set up some secrets here to be able to try this out, and some minor requested changed on the tagging side of the release.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

@xunholy thanks a lot for the changes.

I'd prefer if you could minimize the changes and kept them strictly related to arm support. We shouldn't lose functionality. I wonder if upgrading the push action in a separate PR and adding arm support in another one would make sense.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@xunholy
Copy link
Contributor Author

xunholy commented Sep 15, 2020

Thanks, @sagikazarmark appreciate the comments.

We shouldn't lose functionality.

my understanding is there is no "lost functionality" you're using the old docker push which doesn't support multi-arch so it's a required change.

@sagikazarmark
Copy link
Member

By lost functionality, I meant:

  • All PRs are pushed, not just master (already fixed)
  • extra labels are missing because in v2 it is removed

Also, can you please sign-off your commits? Thanks!

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>

update Dockerfile

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>

Update ci.yml

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>

update to use new official docker buildx action

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>

update Dockerfile

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>

include version image for tagged releases

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>

Update .github/workflows/ci.yml

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com>

add push based on event

Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM, I think we should add a comment/TODO to the images so that we remember why we are using master and eventually pin to a new release

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/release.yml Show resolved Hide resolved
Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>
@xunholy
Copy link
Contributor Author

xunholy commented Sep 26, 2020

@sagikazarmark @bonifaido all feedback has now been addressed, if you could both please rereview 🙏

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @xunholy !

I think it's good enough for now. I have a bunch of changes incoming to the Dockerfile and the way we build images, so let's merge this ASAP.

Personally, I'd consider arm support experimental until we can find a way to make sure we don't break it.

@sagikazarmark
Copy link
Member

Breaking test is okay, it's not supposed to work in PRs I think. (Note to self: fix that too)

@xunholy
Copy link
Contributor Author

xunholy commented Sep 27, 2020

@sagikazarmark agreed. I've been testing the arm64 variant and using it for some time now without issues.

Happy to review any Dockerfile changes you make in future to ensure its still going to work and has the most efficient and optimised approach.

@sagikazarmark sagikazarmark merged commit 549b67b into dexidp:master Sep 27, 2020
sagikazarmark added a commit that referenced this pull request Sep 27, 2020
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
sagikazarmark added a commit that referenced this pull request Sep 27, 2020
fix: typo in environment variables introduced in #1781
elffjs pushed a commit to DIMO-Network/dex that referenced this pull request Jun 27, 2022
…#1781)

add multi-arch image support for armv7/arm64/amd64 architectures
elffjs pushed a commit to DIMO-Network/dex that referenced this pull request Jun 27, 2022
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants