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

update the makefile to push multi-arch images #319

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

Conversation

danbf
Copy link
Contributor

@danbf danbf commented Jul 23, 2021

Problem

current sso images don't support arm64 architecture. that means we can't run them on AWS Graviton's or Apple M1's

Solution

build a multi-arch with linux/amd64,linux/arm64 platform support

Notes

based on https://www.docker.com/blog/multi-arch-images/ and https://www.docker.com/blog/multi-arch-build-what-about-circleci/
our upstream, golang:1.14 supports these architectures and more

@danbf danbf self-assigned this Jul 23, 2021
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #319 (a135d77) into main (a1b1b74) will increase coverage by 0.22%.
The diff coverage is n/a.

❗ Current head a135d77 differs from pull request most recent head 171ce40. Consider uploading reports for the commit 171ce40 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
+ Coverage   62.73%   62.96%   +0.22%     
==========================================
  Files          58       58              
  Lines        4286     4760     +474     
==========================================
+ Hits         2689     2997     +308     
- Misses       1382     1546     +164     
- Partials      215      217       +2     
Impacted Files Coverage Δ
internal/auth/providers/group_cache.go 57.14% <0.00%> (-11.28%) ⬇️
internal/auth/logging_handler.go 28.12% <0.00%> (-2.07%) ⬇️
internal/proxy/logging_handler.go 14.75% <0.00%> (-1.58%) ⬇️
internal/pkg/httpserver/httpserver.go 66.66% <0.00%> (-1.20%) ⬇️
internal/auth/providers/google.go 58.49% <0.00%> (-0.66%) ⬇️
internal/auth/error.go 72.22% <0.00%> (-0.51%) ⬇️
internal/auth/mux.go 74.54% <0.00%> (-0.46%) ⬇️
internal/proxy/templates.go 100.00% <0.00%> (ø)
internal/pkg/aead/mock_cipher.go 0.00% <0.00%> (ø)
internal/pkg/groups/mock_cache.go 0.00% <0.00%> (ø)
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1b1b74...171ce40. Read the comment docs.

@danbf
Copy link
Contributor Author

danbf commented Jul 23, 2021

seems to be working

Screen Shot 2021-07-23 at 12 34 30 AM

@danbf danbf marked this pull request as ready for review July 23, 2021 04:35
@danbf
Copy link
Contributor Author

danbf commented Jul 23, 2021

will test tomorrow to ensure they actually run on the platforms

@danbf
Copy link
Contributor Author

danbf commented Jul 23, 2021

ok got it running in stage and it seems fine, both sso_proxy and sso_auth

Comment on lines +27 to +30
RUN ln -s /usr/bin/dpkg-split /usr/sbin/dpkg-split
RUN ln -s /usr/bin/dpkg-deb /usr/sbin/dpkg-deb
RUN ln -s /bin/tar /usr/sbin/tar
RUN ln -s /bin/rm /usr/sbin/rm
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was needed for the builds to complete. seems like the file locations are in different spots on the ARM arch.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Would it be worth adding this in a quick inline comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this remains, yes. i want to test the resultant multi-arch images in a arm64 cluster before finalizing this. i only got to test it once during hackweek before tearing down the temp cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm going to see if we need this for arm64, which is the primary secondary target we want for M1's and Graviton's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that didn't work, these are needed for arm64 also apparently.

Makefile Outdated
Comment on lines 46 to 47
docker buildx build --tag buzzfeed/sso:$(version) . --platform linux/amd64,linux/arm64,linux/arm/v7 --push
docker buildx build --tag buzzfeed/sso-dev:latest . --platform linux/amd64,linux/arm64,linux/arm/v7 --push
Copy link
Contributor

@Jusshersmith Jusshersmith Aug 2, 2021

Choose a reason for hiding this comment

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

I think we might also want to push a buzzfeed/sso:latest tag. I'm also not particularly sure we really need the buzzfeed/sso-dev:latest tag to be honest. That tag should be created with each push to the main branch (through this logic), so I'm not sure we gain anything with it being here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, this was a typo, should have been buzzfeed/sso:latest. fixing it.

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.

2 participants