Skip to content

Fix stdout and errors in image/trust #636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dnephin merged 1 commit into from
Oct 27, 2017
Merged

Fix stdout and errors in image/trust #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions cli/command/image/trust.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func PushTrustedReference(streams command.Streams, repoInfo *registry.Repository
if err := jsonmessage.DisplayJSONMessagesToStream(in, streams.Out(), nil); err != nil {
return err
}
fmt.Fprintln(streams.Out(), "No tag specified, skipping trust metadata push")
fmt.Fprintln(streams.Err(), "No tag specified, skipping trust metadata push")
return nil
}

Expand All @@ -97,16 +97,14 @@ func PushTrustedReference(streams command.Streams, repoInfo *registry.Repository
}

if target == nil {
fmt.Fprintln(streams.Out(), "No targets found, please provide a specific tag in order to sign it")
return nil

@dnephin dnephin Oct 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this is now an error, so the cli will exit with non-zero. Since the push was not actually completed it seems like this should be an error.

return errors.Errorf("no targets found, please provide a specific tag in order to sign it")
}

fmt.Fprintln(streams.Out(), "Signing and pushing trust metadata")

repo, err := trust.GetNotaryRepository(streams.In(), streams.Out(), command.UserAgent(), repoInfo, &authConfig, "push", "pull")
if err != nil {
fmt.Fprintf(streams.Out(), "Error establishing connection to notary repository: %s\n", err)
return err
return errors.Wrap(err, "error establishing connection to trust repository")
}

// get the latest repository metadata so we can figure out which roles to sign
Expand Down Expand Up @@ -146,7 +144,7 @@ func PushTrustedReference(streams command.Streams, repoInfo *registry.Repository
}

if err != nil {
fmt.Fprintf(streams.Out(), "Failed to sign %q:%s - %s\n", repoInfo.Name.Name(), tag, err.Error())
err = errors.Wrapf(err, "failed to sign %s:%s", repoInfo.Name.Name(), tag)
return trust.NotaryError(repoInfo.Name.Name(), err)
}

Expand Down Expand Up @@ -223,8 +221,7 @@ func trustedPull(ctx context.Context, cli command.Cli, imgRefAndAuth trust.Image
func getTrustedPullTargets(cli command.Cli, imgRefAndAuth trust.ImageRefAndAuth) ([]target, error) {
notaryRepo, err := cli.NotaryClient(imgRefAndAuth, trust.ActionsPullOnly)
if err != nil {
fmt.Fprintf(cli.Out(), "Error establishing connection to trust repository: %s\n", err)
return nil, err
return nil, errors.Wrap(err, "error establishing connection to trust repository")
}

ref := imgRefAndAuth.Reference()
Expand All @@ -239,7 +236,7 @@ func getTrustedPullTargets(cli command.Cli, imgRefAndAuth trust.ImageRefAndAuth)
for _, tgt := range targets {
t, err := convertTarget(tgt.Target)
if err != nil {
fmt.Fprintf(cli.Out(), "Skipping target for %q\n", reference.FamiliarName(ref))
fmt.Fprintf(cli.Err(), "Skipping target for %q\n", reference.FamiliarName(ref))
continue
}
// Only list tags in the top level targets role or the releases delegation role - ignore
Expand All @@ -265,7 +262,7 @@ func getTrustedPullTargets(cli command.Cli, imgRefAndAuth trust.ImageRefAndAuth)
return nil, trust.NotaryError(ref.Name(), errors.Errorf("No trust data for %s", tagged.Tag()))
}

logrus.Debugf("retrieving target for %s role\n", t.Role)
logrus.Debugf("retrieving target for %s role", t.Role)
r, err := convertTarget(t.Target)
return []target{r}, err
}
Expand Down Expand Up @@ -314,8 +311,7 @@ func TrustedReference(ctx context.Context, cli command.Cli, ref reference.NamedT

notaryRepo, err := trust.GetNotaryRepository(cli.In(), cli.Out(), command.UserAgent(), repoInfo, &authConfig, "pull")
if err != nil {
fmt.Fprintf(cli.Out(), "Error establishing connection to trust repository: %s\n", err)
return nil, err
return nil, errors.Wrap(err, "error establishing connection to trust repository")
}

t, err := notaryRepo.GetTargetByName(ref.Tag(), trust.ReleasesRole, data.CanonicalTargetsRole)
Expand All @@ -332,7 +328,6 @@ func TrustedReference(ctx context.Context, cli command.Cli, ref reference.NamedT
return nil, err

}

return reference.WithDigest(reference.TrimNamed(ref), r.digest)
}

Expand All @@ -355,7 +350,7 @@ func TagTrusted(ctx context.Context, cli command.Cli, trustedRef reference.Canon
familiarRef := reference.FamiliarString(ref)
trustedFamiliarRef := reference.FamiliarString(trustedRef)

fmt.Fprintf(cli.Out(), "Tagging %s as %s\n", trustedFamiliarRef, familiarRef)
fmt.Fprintf(cli.Err(), "Tagging %s as %s\n", trustedFamiliarRef, familiarRef)

@endophage endophage Oct 23, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note sure whether this should go to stderr. This is the success message.

@dnephin dnephin Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix for the issue raised in #631.

The convention we should follow for stdout vs stderr error is:

  • stdout is for "normal program output"
  • stderr is for everything else (logging, warnings, etc)

This function is called from 3 commands. For both image build and container create this is definitely not normal program output, it's logging, so stderr is definitely correct.

For the last (image pull) it is a little less clear, but I think it could still easily be considered logging, since the desired action was "pull", not "tag".

@endophage endophage Oct 23, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that depends on whether one expects command "X" to be different to command "X with trust." In my mind, we should send some minimal amount of info to stdout such that if I pipe stderr to /dev/null I can still determine the operation behaved as desired.

@dnephin dnephin Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how posix programs are supported to work. If we send anything to stdout we break anyone trying to use docker build and docker run in a script, when used with trust. The only output to stdout should be normal program output, which is the output from the command being run by docker run.

There is already a mechanism that allows you to determine if the operation behaved correctly or not, the exit status code. This PR also fixes some problems where the exit status was not being used correctly.


return cli.Client().ImageTag(ctx, trustedFamiliarRef, familiarRef)
}
Expand Down
3 changes: 2 additions & 1 deletion e2e/image/pull_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ func TestPullWithContentTrust(t *testing.T) {
image := createMaskedTrustedRemoteImage(t, "trust", "latest")

result := icmd.RunCmd(icmd.Command("docker", "pull", image), withTrustNoPassphrase)
result.Assert(t, icmd.Expected{Err: icmd.None})
result.Assert(t, icmd.Success)
golden.Assert(t, result.Stderr(), "pull-with-content-trust-err.golden")
golden.Assert(t, result.Stdout(), "pull-with-content-trust.golden")
}

Expand Down
1 change: 1 addition & 0 deletions e2e/image/testdata/pull-with-content-trust-err.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Tagging registry:5000/trust@sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d as registry:5000/trust:latest
1 change: 0 additions & 1 deletion e2e/image/testdata/pull-with-content-trust.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,3 @@ Pull (1 of 1): registry:5000/trust:latest@sha256:641b95ddb2ea9dc2af1a0113b6b348e
sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d: Pulling from trust
Digest: sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d
Status: Downloaded newer image for registry:5000/trust@sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d
Tagging registry:5000/trust@sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d as registry:5000/trust:latest