Fix local images when daemon uses containerd storage #222

natalieparellano · 2023-10-25T18:18:55Z

In service of:

pack build fails with experimental containerd feature enabled in Docker Desktop (macos) pack#1519
Feature Request: Support exporting images to a containerd socket lifecycle#829

On Mac: Docker > Settings > Features in development > Use containerd for pulling and storing images
Tests pass for me locally with both configurations.

Local image creation works with these changes, but is unsatisfactory as we can no longer omit base layers that are already present in the daemon. Omitting the layers creates a runnable image, but if you try to docker push that image to a registry the daemon will crash, I'm not sure why. But if you do a docker save on that image you will get a base layer that is 0 in size. It's possible this is something that will be fixed before containerd storage becomes the default configuration. We should keep an eye on it (or maybe there's another way around it).

Signed-off-by: Natalie Arellano <narellano@vmware.com>

acceptance/reproducibility_test.go

natalieparellano · 2023-10-25T18:20:51Z

acceptance/reproducibility_test.go

+	cfg1.DockerVersion = ""
+	cfg2.DockerVersion = ""


I tried explicitly setting this about here:

imgutil/local/save_file.go

Line 210 in 67824e9

config = v1.Config{

but it seems to have no effect.

natalieparellano · 2023-10-25T18:22:12Z

testhelpers/testhelpers.go

-	ref, err := name.ParseReference(refStr, name.WeakValidation)
-	AssertNil(t, err)
+	t.Helper()
+	Run(t, exec.Command("docker", "push", refStr)) // #nosec G204


This was interesting. dockerCli.ImagePush returns a 401 error, but docker image push works fine. I didn't look too deeply into it, but this is another rough edge that could be worth investigating. Our test registry is from github.com/google/go-containerregistry/pkg/registry - maybe another registry would behave differently, but I didn't try it.

That is very weird. Maybe we should open a bug on this one. I captured the flow a bit.

// our docker.Push wrapper fmt.Println("DEBUG (DockerPush)", authConfig.Username, authConfig.Password) rc, err := dockerCli.ImagePush(context.Background(), refStr, dockertypes.ImagePushOptions{ RegistryAuth: base64.URLEncoding.EncodeToString(encodedJSON), PrivilegeFunc: func() (string, error) { fmt.Println("DEBUG (DockerPush)", "PrivilegeFunc") return "bob", nil }, }) // the registry bit func BasicAuth(handler http.Handler, username, password, realm string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { user, pass, ok := r.BasicAuth() fmt.Println("DEBUG (RegistryBasicAuth)", "user:", user, "pass:", pass, "ok:", ok) // relevant debug output DEBUG (DockerPush) vvqijndzyz wiqldsvicu DEBUG (RegistryBasicAuth) user: pass: ok: false DEBUG (RegistryBasicAuth) user: pass: ok: false DEBUG (RegistryBasicAuth) user: pass: ok: false

The generated auth is passed into ImagePushOptions. The first empty user/pass at the registry auth check is typical but the subsequent ones are not. I tried adding the PrivilegeFunc` to see if the 401 would trigger an auth send but the logs show it isn't executed in either situation. Like the auth is ignored entirely 😞

Here are the logs when using the overlayfs for comparison.

DEBUG (DockerPush) zitozndflk dhlzgeqnku DEBUG (RegistryBasicAuth) user: pass: ok: false DEBUG (RegistryBasicAuth) user: zitozndflk pass: dhlzgeqnku ok: true DEBUG (RegistryBasicAuth) user: zitozndflk pass: dhlzgeqnku ok: true

natalieparellano · 2023-10-25T18:25:49Z

local/save_file.go

-		}
-
-		return nil
+		_, err = i.addImageToTar(tw, repoName)


This reduces some duplication but also without the refactor I got an "unexpected format" error when trying to load the image in the daemon. Which is weird because the code looks basically the same...

natalieparellano · 2023-10-25T18:27:52Z

local/save.go

@@ -154,13 +123,16 @@ func (i *Image) doSaveAs(name string) (types.ImageInspect, error) {
 		return types.ImageInspect{}, errors.Wrapf(err, "loading image %q. first error", i.repoName)
 	}

-	inspect, _, err := i.docker.ImageInspectWithRaw(context.Background(), id)
+	inspect, _, err := i.docker.ImageInspectWithRaw(context.Background(), i.repoName)


With containerd storage the image ID is the sha of the manifest, which technically we could compute but it would be hard. To avoid collision problems we can validate that the data in inspect matches the config that we expect.

natalieparellano · 2023-10-25T18:30:55Z

local/save.go

+		// The docker daemon allows this if the given set of layers already exists in the daemon in the given order.
+		inspect, err = i.doSaveAs(name)
+	}
+	if !canOmitBaseLayers || err != nil {


This is the sorriest part... it's going to lead to a huge performance penalty unless we can work around it. Maybe with containerd storage we could get individual layers from the daemon, IDK. If anyone has further context here it would be much appreciated.

I also couldn't find a way around this. The trick of giving a tar header with no bytes does not work so we always have to pay the price it seems. At least until moby/moby#44369 ships

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-10-25T18:32:58Z

acceptance/reproducibility_test.go

+	cfg1.Config.Image = ""
+	cfg2.Config.Image = ""


This seems unspec'd so IDK if we care.

jabrown85

Great work! This seems shippable for me since the storage system is in beta on the docker side.

jabrown85 · 2023-10-30T14:10:28Z

local/save.go

+		// The docker daemon allows this if the given set of layers already exists in the daemon in the given order.
+		inspect, err = i.doSaveAs(name)
+	}
+	if !canOmitBaseLayers || err != nil {


I also couldn't find a way around this. The trick of giving a tar header with no bytes does not work so we always have to pay the price it seems. At least until moby/moby#44369 ships

jabrown85 · 2023-10-30T14:52:22Z

testhelpers/testhelpers.go

-	ref, err := name.ParseReference(refStr, name.WeakValidation)
-	AssertNil(t, err)
+	t.Helper()
+	Run(t, exec.Command("docker", "push", refStr)) // #nosec G204


That is very weird. Maybe we should open a bug on this one. I captured the flow a bit.

// our docker.Push wrapper fmt.Println("DEBUG (DockerPush)", authConfig.Username, authConfig.Password) rc, err := dockerCli.ImagePush(context.Background(), refStr, dockertypes.ImagePushOptions{ RegistryAuth: base64.URLEncoding.EncodeToString(encodedJSON), PrivilegeFunc: func() (string, error) { fmt.Println("DEBUG (DockerPush)", "PrivilegeFunc") return "bob", nil }, }) // the registry bit func BasicAuth(handler http.Handler, username, password, realm string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { user, pass, ok := r.BasicAuth() fmt.Println("DEBUG (RegistryBasicAuth)", "user:", user, "pass:", pass, "ok:", ok) // relevant debug output DEBUG (DockerPush) vvqijndzyz wiqldsvicu DEBUG (RegistryBasicAuth) user: pass: ok: false DEBUG (RegistryBasicAuth) user: pass: ok: false DEBUG (RegistryBasicAuth) user: pass: ok: false

The generated auth is passed into ImagePushOptions. The first empty user/pass at the registry auth check is typical but the subsequent ones are not. I tried adding the PrivilegeFunc` to see if the 401 would trigger an auth send but the logs show it isn't executed in either situation. Like the auth is ignored entirely 😞

Here are the logs when using the overlayfs for comparison.

DEBUG (DockerPush) zitozndflk dhlzgeqnku DEBUG (RegistryBasicAuth) user: pass: ok: false DEBUG (RegistryBasicAuth) user: zitozndflk pass: dhlzgeqnku ok: true DEBUG (RegistryBasicAuth) user: zitozndflk pass: dhlzgeqnku ok: true

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Before #222, we calculated the sha of the config file that we sent to the daemon, and verified that we could inspect an image with that ID. After #222, since the image ID may be the sha of the config file or the sha of the manifest file (depending on whether containerd storage is enabled), we still calculated the sha of the config file that we sent to the daemon, but instead of trying to inspect an image with that ID, we inspected the image by name, derived the config file from the data we got back from inspect, and then verified that the sha of the derived config file matches the sha of the config file that we sent in. This check turned out to be brittle (see buildpacks/pack#2000 and https://cloud-native.slack.com/archives/C0331B61A1Y/p1701976103265489) and we agreed that it should be safe to remove this check. Signed-off-by: Natalie Arellano <narellano@vmware.com>

Fix local images when daemon uses containerd storage

67824e9

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano requested a review from a team as a code owner October 25, 2023 18:18

natalieparellano commented Oct 25, 2023

View reviewed changes

acceptance/reproducibility_test.go Outdated Show resolved Hide resolved

natalieparellano commented Oct 25, 2023

View reviewed changes

Update acceptance/reproducibility_test.go

9c78088

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano commented Oct 25, 2023

View reviewed changes

natalieparellano requested a review from jabrown85 October 25, 2023 18:34

jabrown85 approved these changes Oct 30, 2023

View reviewed changes

jabrown85 mentioned this pull request Oct 30, 2023

c8d: ImagePush ignores provided RegistryAuth moby/moby#46743

Closed

Merge branch 'main' into fix/containerd-hack

dc9c424

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano merged commit 1aff6e5 into main Oct 30, 2023
4 checks passed

natalieparellano deleted the fix/containerd-hack branch October 30, 2023 17:52

natalieparellano mentioned this pull request Jan 17, 2024

Skip validating inspect ID after save #239

Merged

natalieparellano mentioned this pull request Jul 2, 2024

pack should not create an ephemeral builder if it isn't truly needed buildpacks/pack#2195

Closed

1 task

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix local images when daemon uses containerd storage #222

Fix local images when daemon uses containerd storage #222

natalieparellano commented Oct 25, 2023 •

edited

Loading

natalieparellano Oct 25, 2023

natalieparellano Oct 25, 2023 •

edited

Loading

jabrown85 Oct 30, 2023

natalieparellano Oct 25, 2023

natalieparellano Oct 25, 2023

natalieparellano Oct 25, 2023

jabrown85 Oct 30, 2023

natalieparellano Oct 25, 2023

jabrown85 left a comment

jabrown85 Oct 30, 2023

jabrown85 Oct 30, 2023

Fix local images when daemon uses containerd storage #222

Fix local images when daemon uses containerd storage #222

Conversation

natalieparellano commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

natalieparellano Oct 25, 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

jabrown85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalieparellano commented Oct 25, 2023 •

edited

Loading

natalieparellano Oct 25, 2023 •

edited

Loading