Add method to flatten and extract image fs by nkubala · Pull Request #99 · google/go-containerregistry

nkubala · 2018-04-23T21:28:12Z

This will let us pass in any image tar and extract its flattened filesystem. We'll use this in https://github.com/GoogleContainerTools/container-diff to do image filesystem comparisons among other things. This can also be used later to implement flattening of arbitrary images.

dlorenc · 2018-04-23T21:30:47Z

+		return fmt.Errorf("Error creating tar file: %v", err)
+	}
+	defer tarFile.Close()
+	fileWriter := bufio.NewWriter(tarFile)


why do we need the bufio writer?

we don't :)

dlorenc · 2018-04-23T21:31:27Z

+	if err != nil {
+		return fmt.Errorf("Error retriving image layers: %v", err)
+	}
+	for i := len(layers) - 1; i >= 0; i-- {


Can you add a comment on why we go in reverse?

An overall link to the python implementation might help people understand.

dlorenc · 2018-04-23T21:32:59Z

+	return nil
+}
+
+func inWhiteoutDir(fileMap map[string]bool, file string) bool {


some unit tests for this helper would be useful.

added a few

dlorenc · 2018-04-23T21:33:59Z

+			// any entries with a matching (or child) name
+			fileMap[name] = tombstone || !(header.Typeflag == tar.TypeDir)
+			if !tombstone {
+				buf := make([]byte, header.Size)


I think you can use io.Copy from one tar into the other here, after writing the header.

You only have to do it if size != 0.

ah yeah, for some reason I thought I couldn't copy straight from the tar reader. fixed

jonjohnsonjr · 2018-04-23T21:37:07Z

+
+	layers, err := img.Layers()
+	if err != nil {
+		return fmt.Errorf("Error retriving image layers: %v", err)


nit: retrieving

imjasonh · 2018-04-23T22:04:50Z

+	if err != nil {
+		t.Errorf("Error when opening tar file for reading: %v", err)
+	}
+	r := bufio.NewReader(f)


Don't need the bufio.Reader here either.

dlorenc · 2018-04-23T22:27:03Z

+	tarWriter := tar.NewWriter(tarFile)
+	defer tarWriter.Close()
+
+	fileMap := make(map[string]bool, 0)


nit: I find the map literal form a bit more straightforward: fileMap := map[string]bool{}

fair enough

dlorenc · 2018-04-23T22:27:37Z

 	"github.com/google/go-containerregistry/v1/v1util"
 )

+const whiteoutPrefix string = ".wh."


I think you can drop the "string" here.

dlorenc · 2018-04-23T22:28:09Z

+			if !tombstone {
+				tarWriter.WriteHeader(header)
+				if header.Size > 0 {
+					io.Copy(tarWriter, tarReader)


nit: check the error

dlorenc · 2018-04-23T22:29:03Z

 )

+func TestFlatten(t *testing.T) {
+	img, err := ImageFromPath("testdata/whiteout.tar", nil)


Would it be possible to construct this with bazel and reference it as a dependency of the test, instead of checking it in? It would make the contents/functionality of this test more transparent.

yep, updated to use container_layer to manually create a whiteout layer and test that that file isn't in the final tarball

dlorenc · 2018-04-23T22:29:39Z

+		name := header.Name
+		// this image was built by creating a directory called "foo",
+		// touching "/foo/bar", and then removing the whole directory.
+		for _, part := range filepath.SplitList(name) {


We should also add a file to the tar that exercises a file being overwritten in a subsequent layer.

ah yeah good idea.

jonjohnsonjr · 2018-04-23T23:46:29Z

Is it possible to make this just a generic v1.Image -> (v1.Image, error) function so we can use it with any source or destination?

For example, to flatten a remote image:

src, err := remote.Image(tag, auth, http.DefaultTransport)
if err != nil {
  return err
}

flattened, err := mutate.Flatten(src)
if err != nil {
  return err
}

if err := remote.Write(tag, flattened, http.DefaultTransport, remote.WriteOptions{}); err != nil {
  return err
}

Or to flatten a local tarball:

src, err := tarball.ImageFromPath(path, nil)
if err != nil {
  log.Fatalln(err)
}

flattened, err := mutate.Flatten(src)
if err != nil {
  return err
}

if err := tarball.Write(path, "", flattened, &tarball.WriteOptions{}); err != nil {
  return err
}

This currently just writes the output to a tarball on disk, which seems like it would be hard to use as a library.

Or even if it just took a tar.Writer instead of tarFilePath, I think it would be easier to use.

nkubala · 2018-04-24T17:20:00Z

@jonjohnsonjr I think I was a little off with the naming/description of this method. this is an adaptation of the extract() function in the old containerregistry that essentially pulls out the final fs of an image as a tarball. so the idea was never actually to end up with an image here, just the fs tarball.

that being said, there's not really any reason why this can't be a little more generic so it can be used modularly. @dlorenc had suggested returning either a []byte or an io.Reader that can then be used by other functions to create the desired resulting object.

extracting the final filesystem from a remote image:

src, err := remote.Image(tag, auth, http.DefaultTransport)
if err != nil {
  return err
}

fsReader, err := mutate.Flatten(src) // reader containing the extracted fs contents
if err != nil {
  return err
}

tarWriter := tar.NewWriter(some_file_path)
if _, err := io.Copy(tarWriter, tarReader); err != nil {
  return err
}

we should be able to use this to flatten any arbitrary image (and actually return a v1.Image, not a tarball) as well. WDYT?

jonjohnsonjr · 2018-04-24T17:57:05Z

SGTM!

…kage

jonjohnsonjr

Just one nit

jonjohnsonjr · 2018-04-24T22:05:54Z

+	return tar.NewReader(&b), nil
+}
+
+func InWhiteoutDir(fileMap map[string]bool, file string) bool {


Does this need to be public?

whoops, nope thanks

add method to flatten image fs directly into tarball

533f2ba

dlorenc reviewed Apr 23, 2018

View reviewed changes

jonjohnsonjr reviewed Apr 23, 2018

View reviewed changes

imjasonh reviewed Apr 23, 2018

View reviewed changes

review comments

085c804

nkubala force-pushed the flatten branch from 74ac16e to 085c804 Compare April 23, 2018 22:27

dlorenc reviewed Apr 23, 2018

View reviewed changes

build test images with bazel

d450d6d

nkubala force-pushed the flatten branch from 2324c22 to d450d6d Compare April 23, 2018 23:22

buildifier

8d20190

change signature to return reader of extracted fs. move to mutate pac…

0e8f942

…kage

nkubala force-pushed the flatten branch from f3ea9fb to 0e8f942 Compare April 24, 2018 18:50

nkubala changed the title ~~Add method to flatten image fs directly into tarball~~ Add method to flatten and extract image fs Apr 24, 2018

jonjohnsonjr approved these changes Apr 24, 2018

View reviewed changes

make helper method private

3664f4c

dlorenc approved these changes Apr 24, 2018

View reviewed changes

dlorenc merged commit 4f6b76b into google:master Apr 24, 2018

jonjohnsonjr mentioned this pull request Apr 28, 2018

Implement daemon.Write #114

Merged

Conversation

nkubala commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkubala commented Apr 24, 2018

Uh oh!

jonjohnsonjr commented Apr 24, 2018

Uh oh!

jonjohnsonjr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nkubala commented Apr 23, 2018 •

edited

Loading

jonjohnsonjr commented Apr 23, 2018 •

edited

Loading