Add .zip file support for docker context import (alternative) by thaJeztah · Pull Request #1908 · docker/cli

thaJeztah · 2019-05-25T18:10:50Z

Quick stab at making some changes that were suggested in the review of #1895

This will need squashing, and we'll likely want a test (could be in a follow-up)

- What I did
I've introduced a way for docker context import to accept .zip files.

- How I did it
I've tried to implement it in a way that it can land itself better for what future additions also bring, with addition of Cli enum type. Once we identify which type of import we are trying to get for context, then I go ahead and use the specific import functions, although these functions share as much functionality as possible. I've tried to also make use of an interface of fileData to tie these two functionalities together.

- How to verify it
I've updated the existing tests to define their import types as Cli representing the source. One can test the functionality with using the existing any Docker Context tar file and using the contents to create a .zip file. docker context import will work the same. I've also added new tests to check if the import functions properly to determine these different import types.

- Description for the changelog

Introduces a .zip import support for docker context.

Adds capabilities to import a .zip file Introduces ImportType enum to control import func Introduces fileData interface to share functionality between types Created an abstract layer between with doImport to share functionality Implemented re-usable parsers for both .zip and .tar cases Updated instances that call store.Import to have the new ImportType.Cli arg Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Adds a file size check of 10 MB on individual files in zip or tar If bigger than allowed amount will error Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

This util will be used for context imports instead of using ioutil.ReadAll ReadAll by default may cause memory issues and has no limit io.LimitReader can be used to introduce these limitations but has no error handling, which in turn may cause red herrings With LimitedReadAll util we will be able to identify if the file is too big, and error to the user properly Also removed checkFileSize from context import functionality as it's no longer needed Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Removed exported error type ReadExceedsLimitError, since it was only used for testing purposes. Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Moved the funcionality to determing reader and import type out to, getReaderAndImportType and added a test to check for Cli, Tar & Zip. Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Certain tests were failing per not having a proper cleanup Also fixes a linter issue Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

codecov-io · 2019-05-25T18:16:43Z

Codecov Report

Merging #1908 into master will decrease coverage by 0.02%.
The diff coverage is 41.3%.

@@            Coverage Diff             @@
##           master    #1908      +/-   ##
==========================================
- Coverage    56.7%   56.68%   -0.03%     
==========================================
  Files         309      310       +1     
  Lines       21721    21757      +36     
==========================================
+ Hits        12316    12332      +16     
- Misses       8504     8521      +17     
- Partials      901      904       +3

thaJeztah · 2019-05-25T18:15:33Z

cli/context/store/store.go

 func Import(name string, s Writer, reader io.Reader) error {
-	tr := tar.NewReader(reader)
+	// need a buffered reader to read the header without advancing the reader
+	r := bufio.NewReader(reader)


Decided to go with adding detection here; it adds a bit of complexity here, but by doing it here, the Import() signature remains unmodified. Also, by detecting, piping a ZIP works (whereas previously it required a file as the extension was used).

This and usage of the DetectContentType looks good to me.

thaJeztah · 2019-05-25T18:16:32Z

cli/context/store/store.go

 }

+func importZip(name string, s Writer, reader io.Reader) error {
+	body, err := limitedReadAll(reader, maxFileSize)


Moved the limiting here; previously, this would to a ioutils.ReadAll()

thaJeztah · 2019-05-25T18:20:11Z

cli/context/store/store.go

+			// skip this entry, only taking files into account
+			continue
+		}
+		if zf.Name == metaFile {


I went for just duplicating this import, instead of trying to abstract it; it felt like the abstraction was adding a bit too much complexity (e.g. the addition of the fileData interface, the newZipFileData and newTarFileData etc.) for a (not too) complicated function. We could probably do a bit more refactoring, but I left that for later if we want to.

The main reason for the abstraction was in fact removing the code duplication, when I was doing it I was envisioning having multiples of different file types, but as we moved along, it's clear that it's very unlikely that we will get support in for more than tar & zip, thus I am good with removing the abstracted layer.

As you said, if we end up needing more and more file types, then probably would be the time to re-factor and create that additional layer.

Yes, it's a pity it wasn't easy to reuse the code, due to reading, and iterating over the files in Tar and Zip archives is quite different

thaJeztah · 2019-05-25T18:21:28Z

cli/context/store/store.go

+			if err != nil {
+				return err
+			}
+			data, err := ioutil.ReadAll(f)


This could be limitedReadAll, but that would only be for really corner cases (specially crafted ZIP containing a sparse-file, e.g.)

I've mentioned this specific section on the other PR, I am still wondering if adding limited reader for all of these is the safest way to go, since we already have the interface implemented.

I initially had it here, but removed it; I'm fine with having it in both places (whole file, and individual files reading from the Zip)

For the tar, we won't need it, as it's not compressed, so if the file as a whole is within limits, files inside the tar should be as well

thaJeztah · 2019-05-25T18:23:30Z

cli/context/store/store.go

+		return errors.New("archive format is invalid")
+	}
+
+	epName := parts[0]


Removed the tlsData.Push() and inlined it here. Push() was effectively only used here (during import), so there was no need yet to add it as a new "interface"

The odd thing here for me was the function returning nil, and creating a side-affect by changing content of the arguments at the same time. I've tried to change this function to be pure, and that's why I've added the Push, but I am guessing it added more complexity than it should've.

thaJeztah · 2019-05-29T20:53:09Z

closing, because #1895 was merged

goksu and others added 8 commits May 20, 2019 09:38

Fix linter & add file size check

a72c879

Adds a file size check of 10 MB on individual files in zip or tar If bigger than allowed amount will error Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Fix linter for unnecessary variable declaration

3b67d8f

Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Change var def to a block & remove exported err type

cc48aa2

Removed exported error type ReadExceedsLimitError, since it was only used for testing purposes. Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Added a new test to check reader & importType for ctx

436f5b1

Moved the funcionality to determing reader and import type out to, getReaderAndImportType and added a test to check for Cli, Tar & Zip. Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Adds a proper cleanup to getReaderAndImportType

4690a04

Certain tests were failing per not having a proper cleanup Also fixes a linter issue Signed-off-by: Goksu Toprak <goksu.toprak@docker.com>

Use LimitReader for outer file, and some refactoring

872d9ab

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

GordonTheTurtle added the status/0-triage label May 25, 2019

thaJeztah mentioned this pull request May 25, 2019

Add .zip file support for docker context import #1895

Merged

thaJeztah commented May 25, 2019

View reviewed changes

thaJeztah added area/context process/cherry-pick status/2-code-review and removed status/0-triage labels May 25, 2019

thaJeztah closed this May 29, 2019

thaJeztah deleted the carry_1895_import_zip branch May 29, 2019 20:53

thaJeztah removed the process/cherry-pick label Jul 4, 2019

Conversation

thaJeztah commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 25, 2019

Codecov Report

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!

thaJeztah commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented May 25, 2019 •

edited

Loading