Introduce and use resto for registry operations. Remove save and ls. #314

mnottale · 2018-07-25T13:19:24Z

Signed-off-by: Matthieu Nottale matthieu.nottale@docker.com

- What I did

Use our own image-backing mechanism to handle registry interactions.
We win windows support, but we loose ls and save as the local docker daemon is bypassed entirely.
Compatibility is preserved with images pushed prior to this patch.

- How I did it

Introduce pkg/resto with registry operations, use it in place of the old system.

- How to verify it

e2e test suite should cover us.

- Description for the changelog

Windows users can now use the push operation, and use applications hosted on a registry.

- A picture of a cute animal (not mandatory but encouraged)

codecov · 2018-07-25T15:30:42Z

Codecov Report

Merging #314 into master will decrease coverage by 9.55%.
The diff coverage is 48.41%.

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage   62.87%   53.31%   -9.56%     
==========================================
  Files          61       49      -12     
  Lines        3189     2757     -432     
==========================================
- Hits         2005     1470     -535     
- Misses        944     1046     +102     
- Partials      240      241       +1

Impacted Files	Coverage Δ
cmd/docker-app/root.go	`76.47% <ø> (-0.32%)`	⬇️
internal/image/image.go	`0% <ø> (-25.65%)`	⬇️
cmd/docker-app/pull.go	`0% <0%> (ø)`	⬆️
pkg/resto/registry.go	`32.18% <32.18%> (ø)`
pkg/resto/resto.go	`51.15% <51.15%> (ø)`
internal/packager/extract.go	`55.3% <60%> (-11.37%)`	⬇️
internal/packager/registry.go	`64.44% <70.27%> (-16.01%)`	⬇️
internal/packager/init.go	`51.23% <0%> (-14.71%)`	⬇️
internal/helm/helm.go	`55.32% <0%> (-7.43%)`	⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b83fb00...b939b8b. Read the comment docs.

chris-crone · 2018-07-26T09:30:50Z

cmd/docker-app/pull.go

@@ -12,7 +12,7 @@ func pullCmd() *cobra.Command {
 		Short: "Pull an application from a registry",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return packager.Pull(args[0])
+			return packager.Pull(args[0], ".")


Should we outright remove this now that we don't have push?

We have push. Pull is in experimental for now.

chris-crone · 2018-07-26T10:31:48Z

pkg/resto/resto.go

+	return parsedReference{"https://" + domain, reference.Path(ref), tag}, nil
+}
+
+func getCredentials(domain string) (string, string) {


Since there's a chance we miss an error from this function, would be good to make a Credentials struct and return that with an error or return string, string, error.

chris-crone · 2018-07-26T10:39:23Z

pkg/resto/resto.go

+}
+
+// ListRegistry lists all the repositories in a registry
+func ListRegistry(ctx context.Context, endpoint string, username, password string, insecure bool) ([]string, error) {


endpoint string -> endpoint

chris-crone · 2018-07-26T11:01:32Z

pkg/resto/resto.go

+}
+
+// ListRepository lists all the tags in a repository
+func ListRepository(ctx context.Context, reponame string, username, password string, insecure bool) ([]string, error) {


reponame string -> repoName

chris-crone · 2018-07-26T11:01:51Z

pkg/resto/resto.go

+}
+
+// PullConfig pulls a configuration file from a registry
+func PullConfig(ctx context.Context, repoTag string, username, password string, insecure bool) (string, error) {


repoTag string -> repoTag

chris-crone · 2018-07-26T11:04:10Z

pkg/resto/resto.go

+}
+
+// PullConfigMulti pulls a set of configuration files from a registry
+func PullConfigMulti(ctx context.Context, repoTag string, username, password string, insecure bool) (map[string]string, error) {


repoTag string -> repoTag

chris-crone · 2018-07-26T11:05:31Z

pkg/resto/resto.go

+}
+
+// PushConfig pushes a configuration file to a registry and returns its digest
+func PushConfig(ctx context.Context, payload string, repoTag string, username, password string, insecure bool, labels map[string]string) (string, error) {


payload string, repoTag string -> payload, repoTag

chris-crone · 2018-07-26T11:05:55Z

pkg/resto/resto.go

+}
+
+// PushConfigMulti pushes a set of configuration files to a registry and returns its digest
+func PushConfigMulti(ctx context.Context, payload map[string]string, repoTag string, username, password string, insecure bool, labels map[string]string) (string, error) {


repoTag string -> repoTag

chris-crone · 2018-07-26T11:08:25Z

pkg/resto/resto.go

+	res := make(map[string]string)
+	for {
+		header, err := tarReader.Next()
+		if err == io.EOF {


Must also handle other errors here otherwise we never leave this loop.

chris-crone · 2018-07-26T11:10:37Z

pkg/resto/resto.go

+	if !strings.Contains(err.Error(), "manifest invalid") && !strings.Contains(err.Error(), "manifest Unknown") {
+		return "", err
+	}
+	// try legacy mode


Would be good to separate out the legacy and new modes into functions so that this is easier to follow

vdemeester · 2018-07-27T14:08:36Z

pkg/resto/resto.go

+}
+
+// PullConfigMulti pulls a set of configuration files from a registry
+func PullConfigMulti(ctx context.Context, repoTag, username, password string, insecure bool) (map[string]string, error) {


We should use a simpler signature for PullConfigMulti, with the required element as argument (like repotag above) and a config object for the rest.

func PullConfigMulti(ctx context.Context, reference string, options PullOptions) …

vdemeester · 2018-07-27T14:16:00Z

internal/packager/registry.go

-		return errors.Wrapf(err, "error loading image %s", repotag)
+	repoComps := strings.Split(repotag, ":")
+	repo := repoComps[0]
+	if len(repoComps) == 3 || (len(repoComps) == 2 && strings.Contains(repoComps[1], "/")) {


I really isn't clear what we're testing here (worst case scenario would be to extract this into a well named function)

vdemeester · 2018-07-27T14:23:58Z

internal/packager/registry.go

-		if err == io.EOF {
-			break
+	for k, v := range payload {
+		// poor man's security


vdemeester · 2018-07-27T14:28:17Z

internal/packager/registry.go

-			}
-			err = ioutil.WriteFile(filepath.Join(outputDir, internal.DirNameFromAppName(filepath.Base(repo))), data, 0644)
-			return errors.Wrap(err, "error writing output file")
+			return errors.Wrap(err, "failed to write output file "+target)


The target will be in the *wrapped` error, so we don't need to add it to the message

errors.Wrap(err, "failed to write output file")

vdemeester · 2018-07-27T14:30:42Z

internal/packager/registry.go

 		}
 	}
-	return fmt.Errorf("failed to find our layer in tarball")
+	return nil


Could be simplified with

errors.Wrap(ioutil.WriteFile(target, []byte(v), 0644), "failed to write output file"))

(errors.Wrap will return nil if the err is nil)

vdemeester · 2018-07-27T14:59:38Z

pkg/resto/resto.go

+		return "", "", err
+	}
+	cr.Close()
+	if domain == "https://registry-1.docker.io" {


That part too should be handled somewhere in the docker cli, we should use that.

git-grepped for registry-1.docker.io, could not find the relevant bit

vdemeester · 2018-07-27T15:02:48Z

pkg/resto/resto.go

+	return auth.Username, auth.Password, nil
+}
+
+func makeTarGz(content map[string]string) ([]byte, digest.Digest, error) {


Couldn't we use what docker/docker provide and uses (i.e. pkg/archive) and what's done in docker/docker/distribution for pushing code.

Won't make things any simpler from what I see of docker/docker/distribution's interface

vdemeester · 2018-07-27T15:09:12Z

pkg/resto/resto.go

+		return nil, err
+	}
+	if username == "" {
+		username, password, _ = getCredentials(pr.domain)


Why ignoring the error here ?

Because failure to obtain credentials is not an error. One might not need them if pushing to a plain registry.

vdemeester · 2018-07-27T15:15:15Z

pkg/resto/resto.go

+	}
+	repo, err := NewRepository(ctx, pr.domain, pr.path, username, password, insecure)
+	if err != nil {
+		repo, err = NewRepository(ctx, strings.Replace(pr.domain, "https://", "http://", 1), pr.path, username, password, insecure)


Why do we do that ? 🤔

because repotag does not contain the protocol to use

vdemeester · 2018-07-27T15:17:11Z

pkg/resto/resto.go

+}
+
+// PushConfigMulti pushes a set of configuration files to a registry and returns its digest
+func PushConfigMulti(ctx context.Context, payload map[string]string, repoTag, username, password string, insecure bool, labels map[string]string) (string, error) {


Same comment as Pull…

thaJeztah · 2018-07-27T15:24:08Z

pkg/resto/resto.go

+	}
+	repo, err := NewRepository(ctx, pr.domain, pr.path, username, password, insecure)
+	if err != nil {
+		repo, err = NewRepository(ctx, strings.Replace(pr.domain, "https://", "http://", 1), pr.path, username, password, insecure)


This looks fishy; something failed (we don't know what), so leak credentials over plain http://?

I changed it so that it drops credentials by default when fallbacking to http, and so that it fallbacks only if the error suggests we should.

vdemeester

Also, there is no unit tests at all for pkg/resto 😭

thaJeztah · 2018-07-30T11:16:19Z

pkg/resto/registry.go

+	if !strings.Contains(err.Error(), "HTTP response to HTTPS client") {
+		return repo, err
+	}
+	if os.Getenv("RESTO_INSECURE_AUTH") != "1" {


This looks to be global configuration, which means that Docker Hub can be MITM'd; For comparison, the docker daemon requires each registry that is allowed to be accessed insecurely to be configured (through the --insecure-registry option).

For the cli, connecting to an insecure registry, there should be an option to set it for each action (e.g., similar to how docker manifest inspect has an --insecure flag)

We are not a daemon though. By default docker-app/resto won't transmit credentials over an insecure channel unless this override environment variable is used.

So, what will happen is that someone sets RESTO_INSECURE_AUTH=1 to test against a local registry, but now opens a major security hole by allowing insecure access all registries

That seems an unlikely scenario.

So is people doing chmod 777 /var/run/docker.sock, or sudo rm -rf /var/lib/docker

I don't particularly intend to document this env variable, I see it more as an escape hatch in the unlikely scenario one user end up needing it. If that ever happens we will make sure to emphasis the advice to pass it directly on the command line and not to export it or god forbid add it to a .rc file.

It's not an unlikely scenario moby/moby#8887, and it will happen; add this escape hatch, and there'll be no turning back

justincormack · 2018-07-30T14:08:36Z

pkg/resto/registry.go

+		Mirror:    false,
+		URL:       url,
+		Version:   2,
+		TLSConfig: &tls.Config{InsecureSkipVerify: opts.Insecure},


Do not use InsecureSkipVerify. This should never be the default.

It's not the default. It's disabled by default, and enabled by a cli argument.

silvin-lubecki · 2018-07-25T13:25:30Z

internal/image/image.go

-
-	return cmd.Run()
-}
+}


nit: missing end of line

silvin-lubecki · 2018-08-06T08:38:31Z

internal/packager/registry.go

-	app, err := Extract(appname)
+// Pull loads an app from a registry
+func Pull(repotag string, outputDir string) error {
+	payload, err := resto.PullConfigMulti(context.Background(), repotag, resto.RegistryOptions{Insecure: os.Getenv("DOCKERAPP_INSECURE_REGISTRY") != ""})


Related to @thaJeztah comment

silvin-lubecki · 2018-08-06T09:10:37Z

internal/packager/registry.go

-	cmd.Stderr = os.Stderr
-	if err := cmd.Run(); err != nil {
-		return errors.Wrapf(err, "error loading image %s", repotag)
+	repoComps := strings.Split(repotag, ":")


This 5 following lines should be extracted to a sub function, and should be tested

func getRepo(repotag string) string{ repoComps := strings.Split(repotag, ":") // handle the case where a port was specified in the domain part of repotag if len(repoComps) == 3 || (len(repoComps) == 2 && strings.Contains(repoComps[1], "/")) { return repoComps[1] } return repoComps[0] }

Not that distribution/reference might probably handle that case already 👼

silvin-lubecki · 2018-08-06T09:34:17Z

internal/packager/registry.go

-	defer os.Remove(file)
-	f, err := os.Open(file)
+	appDir := filepath.Join(outputDir, internal.DirNameFromAppName(filepath.Base(repo)))
+	err = os.Mkdir(appDir, 0755)


Use Mkdirall instead?

Certainly not. I want an error if the path already exists, And I want an error if outputDir was not already created.

silvin-lubecki · 2018-08-06T09:36:04Z

internal/packager/registry.go

-			break
+	for k, v := range payload {
+		// poor man's security
+		if strings.Contains(k, "/") || strings.Contains(k, "\\") {


At least log something instead of dropping it silently?

silvin-lubecki · 2018-08-06T14:58:27Z

pkg/resto/resto.go

+		err = json.Unmarshal([]byte(ma.Payload), &res)
+		return res, err
+	}
+	// legacy image mode


Extract the following code to a subfunction ?

20 lines called only once.

It's more a matter of readability (or even reviewability 😄 )

silvin-lubecki · 2018-08-06T14:59:12Z

pkg/resto/resto.go

+			return res, err
+		}
+		if header.Typeflag == tar.TypeReg {
+			content := bytes.NewBuffer(nil)


content := &bytes.Buffer{}

That was voted the "least future-proof one-liner of the century" by an independent research team.

Just reading the golang doc https://golang.org/pkg/bytes/#NewBuffer

In most cases, new(Buffer) (or just declaring a Buffer variable) is sufficient to initialize a Buffer.`

They say it's sufficient, not that it's recommended.

silvin-lubecki · 2018-08-06T14:59:54Z

pkg/resto/resto.go

+		return "", err
+	}
+	if opts.Username == "" {
+		opts.Username, opts.Password, _ = getCredentials(pr.domain)


Why dropping the error?

same as above, creds failure, not fatal

At least log it in debug?

silvin-lubecki · 2018-08-06T15:31:38Z

pkg/resto/resto.go

+	if err == nil {
+		return dgst.String(), nil
+	}
+	if !strings.Contains(err.Error(), "manifest invalid") && !strings.Contains(err.Error(), "manifest Unknown") {


Invert the conditions? It will be more readable:

if strings.Contains(err.Error(), "manifest invalid") || strings.Contains(err.Error(), "manifest Unknown") { return "", unsupportedMediaType{} } return "", err

Or even a switch could be nice:

switch{ case strings.Contains(err.Error(), "manifest invalid"): return "", unsupportedMediaType{} case strings.Contains(err.Error(), "manifest Unknown"): return "", unsupportedMediaType{} default: return "", err }

silvin-lubecki · 2018-08-06T15:34:31Z

pkg/resto/resto.go

+		},
+		RootFS: ociv1.RootFS{
+			Type:    "layers",
+			DiffIDs: []digest.Digest{payloadUncompressedDigest}, //nope { payloadDesc.Digest},


I don't understand this comment

silvin-lubecki · 2018-08-06T15:39:55Z

Please rebase 😇

silvin-lubecki · 2018-08-06T15:40:36Z

Also, there is no unit tests at all for pkg/resto 😭

+1

vdemeester · 2018-08-07T09:45:08Z

internal/packager/registry.go

-	cmd.Stderr = os.Stderr
-	if err := cmd.Run(); err != nil {
-		return errors.Wrapf(err, "error loading image %s", repotag)
+	repoComps := strings.Split(repotag, ":")


Not that distribution/reference might probably handle that case already 👼

vdemeester · 2018-08-07T13:03:03Z

pkg/resto/registry.go

+}
+
+// MediaTypeConfig is the media type used for configuration files.
+const MediaTypeConfig = "application/vndr.docker.config"


Looking at some existing media type, this one should be something like application/vnd.docker.app.config.tar

It's not app-specific, the aim is to target arbitrary config files with resto, and it's not a tarball.

then application/vnd.docker.config.tar (my point was mainly about the tar part)

It's not a tar!

vdemeester · 2018-08-07T14:01:40Z

Also …

e2e test suite should cover us.

This is not really the case as the registry we use (either a local registry or the hub) do not seem to support the new media-type… so we're not really testing the new implementation, just the legacy behavior…

mnottale

Yes, short of spawning patched registries there is no way to test the 'mediatype' implementation. And there is no unit tests because the only thing resto does is pushing/pulling to registries, and I'm not spawning a registry in an unit test.

silvin-lubecki · 2018-08-23T14:35:29Z

pkg/resto/resto.go

+}
+
+// ListRegistry lists all the repositories in a registry
+func ListRegistry(ctx context.Context, endpoint string, opts RegistryOptions) ([]string, error) {


Rename to ListRegistries (as you called the other one ListTags).

silvin-lubecki

LGTM

👼

Signed-off-by: Matthieu Nottale <matthieu.nottale@docker.com>

GordonTheTurtle added the status/0-triage label Jul 25, 2018

vdemeester added status/1-design-review and removed status/0-triage labels Jul 25, 2018

mnottale force-pushed the resto branch 4 times, most recently from 91b3f54 to b4d8594 Compare July 25, 2018 15:24

mnottale requested a review from chris-crone as a code owner July 25, 2018 15:24

chris-crone reviewed Jul 26, 2018

View reviewed changes

mnottale force-pushed the resto branch 2 times, most recently from 9de6a74 to d610a77 Compare July 27, 2018 09:42

chris-crone approved these changes Jul 27, 2018

View reviewed changes

vdemeester requested review from vdemeester, simonferquel, mat007 and silvin-lubecki July 27, 2018 12:53

vdemeester suggested changes Jul 27, 2018

View reviewed changes

thaJeztah suggested changes Jul 27, 2018

View reviewed changes

vdemeester suggested changes Jul 27, 2018

View reviewed changes

mnottale force-pushed the resto branch from d610a77 to 8e8e31e Compare July 30, 2018 11:02

thaJeztah reviewed Jul 30, 2018

View reviewed changes

justincormack suggested changes Jul 30, 2018

View reviewed changes

vdemeester mentioned this pull request Jul 31, 2018

Use pkg/archive from docker/docker instead of plain archive/tar #329

Closed

mnottale force-pushed the resto branch 3 times, most recently from 9459ea1 to 1cd97e5 Compare August 2, 2018 12:02

silvin-lubecki suggested changes Aug 6, 2018

View reviewed changes

vdemeester previously requested changes Aug 7, 2018

View reviewed changes

vdemeester reviewed Aug 7, 2018

View reviewed changes

mnottale commented Aug 20, 2018

View reviewed changes

mnottale force-pushed the resto branch 2 times, most recently from 5a490df to 960de41 Compare August 20, 2018 13:34

silvin-lubecki reviewed Aug 23, 2018

View reviewed changes

mnottale force-pushed the resto branch 4 times, most recently from 81d1a63 to d078381 Compare August 27, 2018 08:30

silvin-lubecki approved these changes Aug 28, 2018

View reviewed changes

mnottale force-pushed the resto branch from d078381 to 6095af5 Compare August 28, 2018 13:49

Introduce and use resto for registry operations. Remove save and ls.

b939b8b

Signed-off-by: Matthieu Nottale <matthieu.nottale@docker.com>

mnottale force-pushed the resto branch from 6095af5 to b939b8b Compare August 28, 2018 13:58

mnottale merged commit 5ede3cc into docker:master Aug 29, 2018

mnottale deleted the resto branch August 29, 2018 08:39

chris-crone mentioned this pull request Aug 29, 2018

Expose settings functions externally #366

Merged

vdemeester mentioned this pull request Sep 17, 2018

"Operating system not supported" #382

Closed

Introduce and use resto for registry operations. Remove save and ls. #314

Introduce and use resto for registry operations. Remove save and ls. #314

Conversation

mnottale commented Jul 25, 2018 • edited

codecov bot commented Jul 25, 2018 • edited

Codecov Report

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

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

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

vdemeester 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

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

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

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

silvin-lubecki commented Aug 6, 2018

silvin-lubecki commented Aug 6, 2018 • edited

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

vdemeester commented Aug 7, 2018

mnottale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

mnottale commented Jul 25, 2018 •

edited

codecov bot commented Jul 25, 2018 •

edited

silvin-lubecki commented Aug 6, 2018 •

edited