Support multi architecture docker images using BuildX #3355

super-harsh · 2023-10-01T21:04:50Z

What this PR does / why we need it:

This PR adds support for publishing multi-architecture docker images using buildx.
Changes include:

Adding task for docker login
Adding task for build, tag, and push docker image using buildx
Moving controller build inside docker container to support multi-architecture build
Updating devcontainer to install buildx

codecov-commenter · 2023-10-01T21:41:28Z

Codecov Report

Merging #3355 (640ef95) into main (f553f31) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 640ef95 differs from pull request most recent head 4aa0a07. Consider uploading reports for the commit 4aa0a07 to get more accurate results

@@            Coverage Diff             @@
##             main    #3355      +/-   ##
==========================================
+ Coverage   54.46%   54.47%   +0.01%     
==========================================
  Files        1527     1527              
  Lines      639478   639478              
==========================================
+ Hits       348305   348374      +69     
+ Misses     234334   234266      -68     
+ Partials    56839    56838       -1

see 28 files with indirect coverage changes

theunrepentantgeek

A couple of comments, but I'm not yet convinced that this enables the container change we want without cutting off entire development scenarios.

.devcontainer/install-dependencies.sh

theunrepentantgeek · 2023-10-04T19:12:51Z

.github/workflows/create-release.yml

+      - name: Docker login
+        run: |
+          container_id=${{env.container_id}}
+          docker exec -e AZURE_CLIENT_ID -e AZURE_CLIENT_SECRET -e DOCKER_REGISTRY "$container_id" task docker-login


Now that these secrets are being passed in to the devcontainer, is there any possibility of them being logged or left behind as a part of the published image? If so, this opens up the chance of someone harvesting those secrets.

I don't think so - We already do pass-in secrets for live validation and az-login in other workflows as well?

Logging we're protected from because GH actually redacts the logs for the secret contents. Even if you try to log the secret it'll be redacted.

Rest I think as Harsh said, we should be OK with as long as we aren't actually saving the secret into the published container. I think it would be relatively obviously if we were based on the dockerfile for the image we're building.

See my above question though about why we have to do it this way... it seems like the docker login should work?

Or is the issue that, docker context on host and docker context in devcontainer are different, and so we need to make the login happen in the devcontainer?

Checking a git blame of this file, it was @Porges who wrote the comment

# note that all creds are on host and never passed into devcontainer

Working from the precept that smart people do things for good reasons, I'm worried that we're missing something here. To be more specific on my earlier concern, I'm not worried about disclosure during CI, I'm worried about the credentials being left lying around inside the container image, available for anyone to extract if they go nosing around inside.

By default, Docker looks for the native binary on each of the platforms, i.e. "osxkeychain" on macOS, "wincred" on windows, and "pass" on Linux. A special case is that on Linux, Docker will fall back to the "secretservice" binary if it cannot find the "pass" binary. If none of these binaries are present, it stores the credentials (i.e. password) in base64 encoding in the config files described above.

Here's the reference to how docker stores the credentials and auth tokens.

.devcontainer/install-dependencies.sh

matthchr · 2023-10-05T20:24:42Z

.github/workflows/create-release.yml

+      - name: Docker login
+        run: |
+          container_id=${{env.container_id}}
+          docker exec -e AZURE_CLIENT_ID -e AZURE_CLIENT_SECRET -e DOCKER_REGISTRY "$container_id" task docker-login


Logging we're protected from because GH actually redacts the logs for the secret contents. Even if you try to log the secret it'll be redacted.

Rest I think as Harsh said, we should be OK with as long as we aren't actually saving the secret into the published container. I think it would be relatively obviously if we were based on the dockerfile for the image we're building.

.github/workflows/create-release.yml

matthchr · 2023-10-05T20:26:54Z

.github/workflows/create-release.yml

+      - name: Docker login
+        run: |
+          container_id=${{env.container_id}}
+          docker exec -e AZURE_CLIENT_ID -e AZURE_CLIENT_SECRET -e DOCKER_REGISTRY "$container_id" task docker-login


See my above question though about why we have to do it this way... it seems like the docker login should work?

Or is the issue that, docker context on host and docker context in devcontainer are different, and so we need to make the login happen in the devcontainer?

matthchr · 2023-10-05T20:27:29Z

.github/workflows/create-release.yml

+      - name: Build, tag and push docker image
+        run: |
+          container_id=${{env.container_id}}
+          docker exec -e DOCKER_PUSH_TARGET "$container_id" task controller:docker-push-multiarch


minor: is there a "normal" docker-push target stil? If not, seems like we should just rename this to that? Yes it's multiarch but it's the only push we have?

We don't have any other docker push target instead of docker-push-local. I'm letting the local push target be there for reasons:

For local builds, we don't need multi-arch images as they take time to build and occupy space.

Buildx does not have direct support for pushing to local registries. We need to add in a few extra steps to enable it. Mentioned in Build cannot export to registry on localhost

We should probably document this on the taskfile targets, for why we have two and why different ones are used in different places.

There are some risks with this too (we're not actually testing the image we're creating), but I see that lots of people have this problem, as documented:
docker/buildx#166
docker/buildx#1152

There does seem to be a way to push buildx image to local registry now though, so I am wondering if we should try swapping the kind steps to use the multiarch image?

We can surely swap kind steps to use buildx, but not sure about the multiarch images if thats a good idea. As I noticed, for cross-platform images buildx takes around about 10-15 mins to build(on my machine). Which would increase the CI run time.

CI shouldn't be building the image every time - it's supposed to be cached. We even have a workflowthat runs weekly to keep it current.

So if switching CI to use buildx slows it down, we need to fix that.

I don't think CI caches this image @theunrepentantgeek - it caches the .devcontainer image, not the controller image (being built here). It must build the controller image every time because that's the image that:

Contains the code changes for the PR in question.

Runs in kind and is verified by the tests.

My understanding of what buildx does is that it basically calls N docker builds with different architectures and then automatically merges the manifests. So it may be OK to not use it for the local build (see the issues I linked above for where this is talked about some and people suggest using buildx w/ a single version argument to get local working, which anyway is going to be different than what we do for release)

Taskfile.yml

v2/Dockerfile

v2/.dockerignore

theunrepentantgeek

I'm tentatively convinced that this doesn't prohibit development and debugging approaches that currently work, so I'm approving. Once the comments are addressed, merge away.

.devcontainer/install-dependencies.sh

theunrepentantgeek · 2023-10-06T01:06:36Z

.github/workflows/create-release.yml

+      - name: Docker login
+        run: |
+          container_id=${{env.container_id}}
+          docker exec -e AZURE_CLIENT_ID -e AZURE_CLIENT_SECRET -e DOCKER_REGISTRY "$container_id" task docker-login


Checking a git blame of this file, it was @Porges who wrote the comment

# note that all creds are on host and never passed into devcontainer

Working from the precept that smart people do things for good reasons, I'm worried that we're missing something here. To be more specific on my earlier concern, I'm not worried about disclosure during CI, I'm worried about the credentials being left lying around inside the container image, available for anyone to extract if they go nosing around inside.

theunrepentantgeek · 2023-10-06T01:29:04Z

Taskfile.yml

@@ -35,7 +35,7 @@ vars:
  LATEST_VERSION_TAG:
    sh: git describe --tags $(git rev-list --tags=v2* --max-count=1)

-  VERSION_FLAGS: -ldflags "-X {{.PACKAGE}}/internal/version.BuildVersion={{.VERSION}}"
+  VERSION_FLAGS: '"-X {{.PACKAGE}}/internal/version.BuildVersion={{.VERSION}}"'


Why remove -ldflags - it's one of the flags required to set the version in the executable. Doesn't make sense to me to remove this here, then restate is manually everywhere else.

If you have a need for part of this value elsewhere, introduce a different variable for that purpose rather than redefining this one and making things awkward.

Taskfile.yml

…touch ups

Taskfile.yml

matthchr · 2023-10-09T16:46:12Z

Taskfile.yml

+      - docker buildx create --driver-opt network=host --use
+      - docker buildx build --push
+        --build-arg VERSION_FLAGS={{.VERSION_FLAGS}}
+        --build-arg CONTROLLER_APP={{.CONTROLLER_APP}}


minor: Leave a comment here about how we don't use multiarch platform options here because we dont need them for local testing and they don't work for local registries? (Can link the GH issues I linked earlier)

v2/.dockerignore

v2/Dockerfile

matthchr · 2023-10-11T20:34:34Z

Taskfile.yml

@@ -372,57 +373,72 @@ tasks:
      # excluding the ./apis directory here


I'm not sure this is true anymore, it's not excluded. I don't know why it should be excluded though because I do think that the controller does depend on the api folder... so maybe just delete this comment?

Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com>

super-harsh added 13 commits September 29, 2023 10:39

WIP

25861d7

Merge branch 'main' into feature/multi-arch-images

080af8a

Add buildx installation

3bbdbe1

Update buildx installation

f96b1a2

Update buildx installation

fc7e05c

WIP

b9f5ea4

Update Taskfile

94f0d23

Add QEMU action for multi platform build

86db4f6

WIP

d2cecd6

WIP

9691804

WIP

3d67c7d

WIP

2dc48de

Update create release workflow to support multi arch docker images

f07128d

super-harsh requested review from davefellows, theunrepentantgeek, matthchr and babbageclunk as code owners October 1, 2023 21:04

Add extra line in .dockerignore

df140a8

super-harsh added 3 commits October 2, 2023 11:11

Merge branch 'main' into feature/multi-arch-images

14779d5

exclude -ldflags from VERSION_FLAGS var

4d58a63

Merge branch 'main' into feature/multi-arch-images

8030ada

super-harsh self-assigned this Oct 4, 2023

super-harsh added this to the v2.4.0 milestone Oct 4, 2023

theunrepentantgeek reviewed Oct 4, 2023

View reviewed changes

matthchr reviewed Oct 5, 2023

View reviewed changes

theunrepentantgeek approved these changes Oct 6, 2023

View reviewed changes

super-harsh added 2 commits October 9, 2023 21:01

Use buildx for docker-push-local; Add LDFLAGS var to TaskFile ;Minor …

75c78f5

…touch ups

Revert controller:docker-build-and-save target

1e56895

matthchr reviewed Oct 9, 2023

View reviewed changes

use regex for including all go files

6e79bef

matthchr approved these changes Oct 11, 2023

View reviewed changes

super-harsh and others added 3 commits October 12, 2023 09:50

Update v2/Dockerfile

640ef95

Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com>

Add comment

781d35c

Merge branch 'main' into feature/multi-arch-images

4aa0a07

super-harsh merged commit 4d037c8 into main Oct 11, 2023
8 checks passed

super-harsh deleted the feature/multi-arch-images branch October 11, 2023 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi architecture docker images using BuildX #3355

Support multi architecture docker images using BuildX #3355

super-harsh commented Oct 1, 2023

codecov-commenter commented Oct 1, 2023 •

edited

Loading

theunrepentantgeek left a comment

theunrepentantgeek Oct 4, 2023

super-harsh Oct 4, 2023 •

edited

Loading

matthchr Oct 5, 2023

matthchr Oct 5, 2023

theunrepentantgeek Oct 6, 2023

super-harsh Oct 11, 2023

matthchr Oct 5, 2023

matthchr Oct 5, 2023

matthchr Oct 5, 2023

super-harsh Oct 6, 2023

matthchr Oct 6, 2023

super-harsh Oct 8, 2023 •

edited

Loading

theunrepentantgeek Oct 8, 2023

matthchr Oct 9, 2023

theunrepentantgeek left a comment

theunrepentantgeek Oct 6, 2023

theunrepentantgeek Oct 6, 2023

matthchr Oct 9, 2023

matthchr Oct 11, 2023

		@@ -372,57 +373,72 @@ tasks:
		# excluding the ./apis directory here

Support multi architecture docker images using BuildX #3355

Support multi architecture docker images using BuildX #3355

Conversation

super-harsh commented Oct 1, 2023

codecov-commenter commented Oct 1, 2023 • edited Loading

Codecov Report

theunrepentantgeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-harsh Oct 4, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-harsh Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theunrepentantgeek 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

Choose a reason for hiding this comment

codecov-commenter commented Oct 1, 2023 •

edited

Loading

super-harsh Oct 4, 2023 •

edited

Loading

super-harsh Oct 8, 2023 •

edited

Loading