From 67824e989715b0bdd3793f4df3d2e55d643052d8 Mon Sep 17 00:00:00 2001 From: Natalie Arellano Date: Wed, 25 Oct 2023 14:07:23 -0400 Subject: [PATCH 1/2] Fix local images when daemon uses containerd storage Signed-off-by: Natalie Arellano --- acceptance/reproducibility_test.go | 6 ++ local/local_test.go | 35 ++++--- local/save.go | 147 ++++++++++++++++++----------- local/save_file.go | 63 ++----------- remote/remote_test.go | 2 - testhelpers/testhelpers.go | 33 ++++--- 6 files changed, 142 insertions(+), 144 deletions(-) diff --git a/acceptance/reproducibility_test.go b/acceptance/reproducibility_test.go index ecc35c1d..d365132b 100644 --- a/acceptance/reproducibility_test.go +++ b/acceptance/reproducibility_test.go @@ -164,6 +164,12 @@ func compare(t *testing.T, img1, img2 string) { cfg2, err := v1img2.ConfigFile() h.AssertNil(t, err) + // images that were created locally may have `DockerVersion` equal to "dev" and missing `Config.Image` + cfg1.DockerVersion = "" + cfg2.DockerVersion = "" + cfg1.Config.Image = "" + cfg2.Config.Image = "" + h.AssertEq(t, cfg1, cfg2) h.AssertEq(t, ref1.Identifier(), ref2.Identifier()) diff --git a/local/local_test.go b/local/local_test.go index d9b2a8a5..276309f3 100644 --- a/local/local_test.go +++ b/local/local_test.go @@ -1114,15 +1114,15 @@ func testImage(t *testing.T, when spec.G, it spec.S) { when("#Rebase", func() { when("image exists", func() { var ( - repoName = newTestImageName() - oldBase, oldTopLayer, newBase, origID string - oldBaseLayer1DiffID string - oldBaseLayer2DiffID string - newBaseLayer1DiffID string - newBaseLayer2DiffID string - imgLayer1DiffID string - imgLayer2DiffID string - origNumLayers int + repoName = newTestImageName() + oldBase, oldTopLayer, newBase string + oldBaseLayer1DiffID string + oldBaseLayer2DiffID string + newBaseLayer1DiffID string + newBaseLayer2DiffID string + imgLayer1DiffID string + imgLayer2DiffID string + origNumLayers int ) it.Before(func() { @@ -1198,11 +1198,10 @@ func testImage(t *testing.T, when spec.G, it spec.S) { inspect, _, err = dockerClient.ImageInspectWithRaw(context.TODO(), repoName) h.AssertNil(t, err) origNumLayers = len(inspect.RootFS.Layers) - origID = inspect.ID }) it.After(func() { - h.AssertNil(t, h.DockerRmi(dockerClient, repoName, oldBase, newBase, origID)) + h.AssertNil(t, h.DockerRmi(dockerClient, repoName, oldBase, newBase)) }) it("switches the base", func() { @@ -1604,6 +1603,8 @@ func testImage(t *testing.T, when spec.G, it spec.S) { repoName = newTestImageName() prevLayer1SHA string prevLayer2SHA string + layer1Path string + layer2Path string ) it.Before(func() { @@ -1616,13 +1617,11 @@ func testImage(t *testing.T, when spec.G, it spec.S) { ) h.AssertNil(t, err) - layer1Path, err := h.CreateSingleFileLayerTar("/layer-1.txt", "old-layer-1", daemonOS) + layer1Path, err = h.CreateSingleFileLayerTar("/layer-1.txt", "old-layer-1", daemonOS) h.AssertNil(t, err) - defer os.Remove(layer1Path) - layer2Path, err := h.CreateSingleFileLayerTar("/layer-2.txt", "old-layer-2", daemonOS) + layer2Path, err = h.CreateSingleFileLayerTar("/layer-2.txt", "old-layer-2", daemonOS) h.AssertNil(t, err) - defer os.Remove(layer2Path) h.AssertNil(t, prevImage.AddLayer(layer1Path)) h.AssertNil(t, prevImage.AddLayer(layer2Path)) @@ -1638,6 +1637,8 @@ func testImage(t *testing.T, when spec.G, it spec.S) { it.After(func() { h.AssertNil(t, h.DockerRmi(dockerClient, repoName, prevImageName)) + h.AssertNil(t, os.RemoveAll(layer1Path)) + h.AssertNil(t, os.RemoveAll(layer2Path)) }) it("reuses a layer", func() { @@ -1869,7 +1870,7 @@ func testImage(t *testing.T, when spec.G, it spec.S) { it.After(func() { h.AssertNil(t, os.Remove(tarPath)) - h.AssertNil(t, h.DockerRmi(dockerClient, repoName, origID)) + h.AssertNil(t, h.DockerRmi(dockerClient, repoName)) }) it("saved image overrides image with new ID", func() { @@ -1909,7 +1910,6 @@ func testImage(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, inspect.Created, imgutil.NormalizedDateTime.Format(time.RFC3339)) h.AssertEq(t, inspect.Container, "") - h.AssertEq(t, inspect.DockerVersion, "") history, err := dockerClient.ImageHistory(context.TODO(), repoName) h.AssertNil(t, err) @@ -1940,7 +1940,6 @@ func testImage(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, inspect.Created, expectedTime.Format(time.RFC3339)) h.AssertEq(t, inspect.Container, "") - h.AssertEq(t, inspect.DockerVersion, "") history, err := dockerClient.ImageHistory(context.TODO(), repoName) h.AssertNil(t, err) diff --git a/local/save.go b/local/save.go index 4e6cede2..a8db9730 100644 --- a/local/save.go +++ b/local/save.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/docker/docker/api/types" "github.com/docker/docker/client" @@ -24,10 +25,17 @@ func (i *Image) Save(additionalNames ...string) error { } func (i *Image) SaveAs(name string, additionalNames ...string) error { - // during the first save attempt some layers may be excluded. 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 err != nil { + var ( + inspect types.ImageInspect + err error + ) + canOmitBaseLayers := !usesContainerdStorage(i.docker) + if canOmitBaseLayers { + // During the first save attempt some layers may be excluded. + // 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 { // populate all layer paths and try again without the above performance optimization. if err := i.downloadBaseLayersOnce(); err != nil { return err @@ -58,6 +66,14 @@ func (i *Image) SaveAs(name string, additionalNames ...string) error { return nil } +func usesContainerdStorage(docker DockerClient) bool { + info, err := docker.Info(context.Background()) + if err != nil { + return false + } + return strings.Contains(info.Driver, "stargz") +} + func (i *Image) doSaveAs(name string) (types.ImageInspect, error) { ctx := context.Background() done := make(chan error) @@ -94,58 +110,11 @@ func (i *Image) doSaveAs(name string) (types.ImageInspect, error) { }() tw := tar.NewWriter(pw) - defer tw.Close() - - configFile, err := i.newConfigFile() - if err != nil { - return types.ImageInspect{}, errors.Wrap(err, "generating config file") - } - - id := fmt.Sprintf("%x", sha256.Sum256(configFile)) - if err := addTextToTar(tw, id+".json", configFile); err != nil { - return types.ImageInspect{}, err - } - - var blankIdx int - var layerPaths []string - for _, path := range i.layerPaths { - if path == "" { - layerName := fmt.Sprintf("blank_%d", blankIdx) - blankIdx++ - hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0} - if err := tw.WriteHeader(hdr); err != nil { - return types.ImageInspect{}, err - } - layerPaths = append(layerPaths, layerName) - } else { - layerName := fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path))) - f, err := os.Open(filepath.Clean(path)) - if err != nil { - return types.ImageInspect{}, err - } - defer f.Close() - if err := addFileToTar(tw, layerName, f); err != nil { - return types.ImageInspect{}, err - } - f.Close() - layerPaths = append(layerPaths, layerName) - } - } - - manifest, err := json.Marshal([]map[string]interface{}{ - { - "Config": id + ".json", - "RepoTags": []string{repoName}, - "Layers": layerPaths, - }, - }) + configHash, err := i.addImageToTar(tw, repoName) if err != nil { return types.ImageInspect{}, err } - - if err := addTextToTar(tw, "manifest.json", manifest); err != nil { - return types.ImageInspect{}, err - } + defer tw.Close() tw.Close() pw.Close() @@ -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) if err != nil { if client.IsErrNotFound(err) { return types.ImageInspect{}, errors.Wrapf(err, "saving image %q", i.repoName) } return types.ImageInspect{}, err } + if err = i.validateInspect(inspect, configHash); err != nil { + return types.ImageInspect{}, err + } return inspect, nil } @@ -247,6 +219,73 @@ func (i *Image) downloadBaseLayers() error { return nil } +func (i *Image) addImageToTar(tw *tar.Writer, repoName string) (string, error) { + configFile, err := i.newConfigFile() + if err != nil { + return "", errors.Wrap(err, "generating config file") + } + + configHash := fmt.Sprintf("%x", sha256.Sum256(configFile)) + if err := addTextToTar(tw, configHash+".json", configFile); err != nil { + return "", err + } + + var blankIdx int + var layerPaths []string + for _, path := range i.layerPaths { + if path == "" { + layerName := fmt.Sprintf("blank_%d", blankIdx) + blankIdx++ + hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0} + if err := tw.WriteHeader(hdr); err != nil { + return "", err + } + layerPaths = append(layerPaths, layerName) + } else { + layerName := fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path))) + f, err := os.Open(filepath.Clean(path)) + if err != nil { + return "", err + } + defer f.Close() + if err := addFileToTar(tw, layerName, f); err != nil { + return "", err + } + f.Close() + layerPaths = append(layerPaths, layerName) + } + } + + manifest, err := json.Marshal([]map[string]interface{}{ + { + "Config": configHash + ".json", + "RepoTags": []string{repoName}, + "Layers": layerPaths, + }, + }) + if err != nil { + return "", err + } + + return configHash, addTextToTar(tw, "manifest.json", manifest) +} + +func (i *Image) validateInspect(inspect types.ImageInspect, givenConfigHash string) error { + foundConfig, err := v1Config(inspect, i.createdAt, i.history) + if err != nil { + return fmt.Errorf("failed to get config file from inspect: %w", err) + } + foundConfigFile, err := json.Marshal(foundConfig) + if err != nil { + return fmt.Errorf("failed to marshal config file: %w", err) + } + foundID := fmt.Sprintf("%x", sha256.Sum256(foundConfigFile)) + if foundID != givenConfigHash { + return fmt.Errorf("expected config hash %q; got %q", givenConfigHash, foundID) + } + return nil +} + // helpers func checkResponseError(r io.Reader) error { diff --git a/local/save_file.go b/local/save_file.go index 9931a343..1a238d1b 100644 --- a/local/save_file.go +++ b/local/save_file.go @@ -3,7 +3,6 @@ package local import ( "archive/tar" "context" - "crypto/sha256" "encoding/json" "fmt" "io" @@ -31,9 +30,10 @@ func (i *Image) SaveFile() (string, error) { } }() - // All layers need to be present here. Missing layers are either due to utilization of: (1) WithPreviousImage(), - // or (2) FromBaseImage(). The former is only relevant if ReuseLayers() has been called which takes care of - // resolving them. The latter case needs to be handled explicitly. + // All layers need to be present here. Missing layers are either due to utilization of + // (1) WithPreviousImage(), or (2) FromBaseImage(). + // The former is only relevant if ReuseLayers() has been called which takes care of resolving them. + // The latter case needs to be handled explicitly. if err := i.downloadBaseLayersOnce(); err != nil { return "", errors.Wrap(err, "failed to fetch base layers") } @@ -55,63 +55,16 @@ func (i *Image) SaveFile() (string, error) { tw := tar.NewWriter(pw) defer tw.Close() - config, err := i.newConfigFile() - if err != nil { - return errors.Wrap(err, "failed to generate config file") - } - - configName := fmt.Sprintf("/%x.json", sha256.Sum256(config)) - if err := addTextToTar(tw, configName, config); err != nil { - return errors.Wrap(err, "failed to add config file to tar archive") - } - - for _, path := range i.layerPaths { - err := func() error { - f, err := os.Open(filepath.Clean(path)) - if err != nil { - return errors.Wrapf(err, "failed to open layer path: %s", path) - } - defer f.Close() - - layerName := fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path))) - if err := addFileToTar(tw, layerName, f); err != nil { - return errors.Wrapf(err, "failed to add layer to tar archive from path: %s", path) - } - - return nil - }() - - if err != nil { - return err - } - } - t, err := registryName.NewTag(i.repoName, registryName.WeakValidation) if err != nil { return errors.Wrap(err, "failed to create tag") } - layers := make([]string, 0, len(i.layerPaths)) - for _, path := range i.layerPaths { - layers = append(layers, fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path)))) - } + // returns valid 'name:tag' appending 'latest', if missing tag + repoName := t.Name() - manifest, err := json.Marshal([]map[string]interface{}{ - { - "Config": configName, - "RepoTags": []string{t.Name()}, - "Layers": layers, - }, - }) - if err != nil { - return errors.Wrap(err, "failed to create manifest") - } - - if err := addTextToTar(tw, "/manifest.json", manifest); err != nil { - return errors.Wrap(err, "failed to add manifest to tar archive") - } - - return nil + _, err = i.addImageToTar(tw, repoName) + return err }) err = errs.Wait() diff --git a/remote/remote_test.go b/remote/remote_test.go index 5a78d395..84a3ef1b 100644 --- a/remote/remote_test.go +++ b/remote/remote_test.go @@ -1726,7 +1726,6 @@ func testImage(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, configFile.Created.Time, imgutil.NormalizedDateTime) h.AssertEq(t, configFile.Container, "") - h.AssertEq(t, configFile.DockerVersion, "") h.AssertEq(t, len(configFile.History), len(configFile.RootFS.DiffIDs)) for _, item := range configFile.History { @@ -1754,7 +1753,6 @@ func testImage(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, configFile.Created.Time, expectedTime) h.AssertEq(t, configFile.Container, "") - h.AssertEq(t, configFile.DockerVersion, "") h.AssertEq(t, len(configFile.History), len(configFile.RootFS.DiffIDs)) for _, item := range configFile.History { diff --git a/testhelpers/testhelpers.go b/testhelpers/testhelpers.go index 2d64d16c..cb1abc40 100644 --- a/testhelpers/testhelpers.go +++ b/testhelpers/testhelpers.go @@ -5,7 +5,6 @@ import ( "bytes" "context" "crypto/sha256" - "encoding/base64" "encoding/hex" "encoding/json" "fmt" @@ -14,6 +13,7 @@ import ( "net/http" "net/url" "os" + "os/exec" "path/filepath" "regexp" "runtime" @@ -206,25 +206,28 @@ func DockerRmi(dockerCli dockercli.CommonAPIClient, repoNames ...string) error { // PushImage pushes an image to a registry, optionally using credentials from any set DOCKER_CONFIG func PushImage(t *testing.T, dockerCli dockercli.CommonAPIClient, refStr string) { - ref, err := name.ParseReference(refStr, name.WeakValidation) - AssertNil(t, err) + t.Helper() + Run(t, exec.Command("docker", "push", refStr)) // #nosec G204 +} - auth, err := authn.DefaultKeychain.Resolve(ref.Context().Registry) - AssertNil(t, err) - authConfig, err := auth.Authorization() +func Run(t *testing.T, cmd *exec.Cmd) string { + t.Helper() + txt, _, err := RunE(cmd) AssertNil(t, err) + return txt +} - encodedJSON, err := json.Marshal(authConfig) - AssertNil(t, err) +func RunE(cmd *exec.Cmd) (output string, exitCode int, err error) { + var stderr bytes.Buffer + cmd.Stderr = &stderr - rc, err := dockerCli.ImagePush(context.Background(), refStr, dockertypes.ImagePushOptions{RegistryAuth: base64.URLEncoding.EncodeToString(encodedJSON)}) - AssertNil(t, err) - defer rc.Close() - - AssertNil(t, checkResponseError(rc)) + stdout, err := cmd.Output() + if err != nil { + formattedErr := fmt.Errorf("failed to execute command: %v, %s, %s, %s", cmd.Args, err, stderr.String(), stdout) + return "", -1, formattedErr + } - _, err = io.Copy(io.Discard, rc) - AssertNil(t, err) + return string(stdout), 0, nil } // DeleteRegistryBlob deletes the blob with the given digest from the registry by issuing an HTTP DELETE request. From 9c78088140039182f12d0f04f8a99274c51d00b6 Mon Sep 17 00:00:00 2001 From: Natalie Arellano Date: Wed, 25 Oct 2023 14:32:34 -0400 Subject: [PATCH 2/2] Update acceptance/reproducibility_test.go Signed-off-by: Natalie Arellano --- acceptance/reproducibility_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/reproducibility_test.go b/acceptance/reproducibility_test.go index d365132b..170a99b3 100644 --- a/acceptance/reproducibility_test.go +++ b/acceptance/reproducibility_test.go @@ -164,7 +164,7 @@ func compare(t *testing.T, img1, img2 string) { cfg2, err := v1img2.ConfigFile() h.AssertNil(t, err) - // images that were created locally may have `DockerVersion` equal to "dev" and missing `Config.Image` + // images that were created locally may have `DockerVersion` equal to "dev" and missing `Config.Image` if the daemon uses containerd storage cfg1.DockerVersion = "" cfg2.DockerVersion = "" cfg1.Config.Image = ""