From 2bb292d2ee4447f89e8ee80e70188c28f929e39c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 4 Jul 2017 19:54:45 -0400 Subject: [PATCH] refactoring buildCtx and dockerfileCtx setup by extarcting a new buildInput struct Signed-off-by: Daniel Nephin --- cli/command/image/build.go | 235 ++++++++--------------------- cli/command/image/build_context.go | 165 ++++++++++++++++++++ cli/command/image/build_session.go | 38 +++-- 3 files changed, 260 insertions(+), 178 deletions(-) create mode 100644 cli/command/image/build_context.go diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 4e795b9cbde0..10e49e13d8e3 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -7,12 +7,10 @@ import ( "encoding/json" "fmt" "io" - "io/ioutil" "os" "regexp" "runtime" - "github.com/Sirupsen/logrus" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/image/build" @@ -21,13 +19,12 @@ import ( "github.com/docker/docker/api" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/client/session" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/pkg/progress" "github.com/docker/docker/pkg/streamformatter" - "github.com/docker/docker/pkg/urlutil" units "github.com/docker/go-units" - "github.com/pkg/errors" "github.com/spf13/cobra" "golang.org/x/net/context" ) @@ -162,174 +159,62 @@ func (out *lastProgressOutput) WriteProgress(prog progress.Progress) error { return out.output.WriteProgress(prog) } -// nolint: gocyclo func runBuild(dockerCli command.Cli, options buildOptions) error { - var ( - buildCtx io.ReadCloser - dockerfileCtx io.ReadCloser - err error - contextDir string - tempDir string - relDockerfile string - remote string - ) - - if options.dockerfileFromStdin() { - if options.contextFromStdin() { - return errors.New("invalid argument: can't use stdin for both build context and dockerfile") - } - dockerfileCtx = dockerCli.In() - } - idFile := build.NewIDFile(options.imageIDFile) // Avoid leaving a stale file if we eventually fail if err := idFile.Remove(); err != nil { return err } - specifiedContext := options.context buildBuffer := newBuildBuffer(dockerCli.Out(), dockerCli.Err(), options.quiet) - switch { - case options.contextFromStdin(): - // buildCtx is tar archive. if stdin was dockerfile then it is wrapped - buildCtx, relDockerfile, err = build.GetContextFromReader(dockerCli.In(), options.dockerfileName) - case isLocalDir(specifiedContext): - contextDir, relDockerfile, err = build.GetContextFromLocalDir(specifiedContext, options.dockerfileName) - case urlutil.IsGitURL(specifiedContext): - tempDir, relDockerfile, err = build.GetContextFromGitURL(specifiedContext, options.dockerfileName) - case urlutil.IsURL(specifiedContext): - buildCtx, relDockerfile, err = build.GetContextFromURL(buildBuffer.progress, specifiedContext, options.dockerfileName) - default: - return errors.Errorf("unable to prepare context: path %q not found", specifiedContext) - } - + buildInput, err := setupContextAndDockerfile(dockerCli, buildBuffer, options) if err != nil { - buildBuffer.PrintProgressOnError() - return errors.Errorf("unable to prepare context: %s", err) - } - - if tempDir != "" { - defer os.RemoveAll(tempDir) - contextDir = tempDir - } - - // read from a directory into tar archive - if buildCtx == nil && !options.stream { - excludes, err := build.ReadDockerignore(contextDir) - if err != nil { - return err - } - - if err := build.ValidateContextDirectory(contextDir, excludes); err != nil { - return errors.Errorf("error checking context: '%s'.", err) - } - - // And canonicalize dockerfile name to a platform-independent one - relDockerfile, err = archive.CanonicalTarNameForPath(relDockerfile) - if err != nil { - return errors.Errorf("cannot canonicalize dockerfile path %s: %v", relDockerfile, err) - } - - excludes = build.TrimBuildFilesFromExcludes(excludes, relDockerfile, options.dockerfileFromStdin()) - buildCtx, err = archive.TarWithOptions(contextDir, &archive.TarOptions{ - ExcludePatterns: excludes, - }) - if err != nil { - return err - } - } - - // replace Dockerfile if it was added from stdin and there is archive context - if dockerfileCtx != nil && buildCtx != nil { - buildCtx, relDockerfile, err = build.AddDockerfileToBuildContext(dockerfileCtx, buildCtx) - if err != nil { - return err - } - } - - // if streaming and dockerfile was not from stdin then read from file - // to the same reader that is usually stdin - if options.stream && dockerfileCtx == nil { - dockerfileCtx, err = os.Open(relDockerfile) - if err != nil { - return errors.Wrapf(err, "failed to open %s", relDockerfile) - } - defer dockerfileCtx.Close() + return err } + defer buildInput.cleanup() ctx, cancel := context.WithCancel(context.Background()) defer cancel() - var resolvedTags []*resolvedTag - if command.IsTrusted() { - translator := func(ref reference.NamedTagged) (reference.Canonical, error) { - return TrustedReference(ctx, dockerCli, ref, nil) - } - // if there is a tar wrapper, the dockerfile needs to be replaced inside it - if buildCtx != nil { - // Wrap the tar archive to replace the Dockerfile entry with the rewritten - // Dockerfile which uses trusted pulls. - buildCtx, resolvedTags = rewriteDockerfileForContentTrust(buildCtx, relDockerfile, translator) - } else if dockerfileCtx != nil { - // if there was not archive context still do the possible replacements in Dockerfile - newDockerfile, _, err := rewriteDockerfileFrom(dockerfileCtx, translator) - if err != nil { - return err - } - dockerfileCtx = ioutil.NopCloser(bytes.NewBuffer(newDockerfile)) - } + resolvedTags, err := updateBuildInputForContentTrust(ctx, dockerCli, buildInput) + if err != nil { + return err } if options.compress { - buildCtx, err = build.Compress(buildCtx) + buildInput.buildCtx, err = build.Compress(buildInput.buildCtx) if err != nil { return err } } - // Setup an upload progress bar - progressOutput := streamformatter.NewProgressOutput(buildBuffer.progress) - if !dockerCli.Out().IsTerminal() { - progressOutput = &lastProgressOutput{output: progressOutput} - } - - // if up to this point nothing has set the context then we must have have - // another way for sending it(streaming) and set the context to the Dockerfile - if dockerfileCtx != nil && buildCtx == nil { - buildCtx = dockerfileCtx - } - - s, err := trySession(dockerCli, contextDir) - if err != nil { - return err + var contextSession *session.Session + if options.stream { + contextSession, err = newBuildContextSession(dockerCli, buildInput.contextDir) + if err != nil { + return err + } } + progressOutput := newProgaressOutput(dockerCli.Out(), buildBuffer.progress) var body io.Reader - if buildCtx != nil && !options.stream { - body = progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon") - } - - // add context stream to the session - if options.stream && s != nil { - syncDone := make(chan error) // used to signal first progress reporting completed. - // progress would also send errors but don't need it here as errors - // are handled by session.Run() and ImageBuild() - if err := addDirToSession(s, contextDir, progressOutput, syncDone); err != nil { + var remote string + if contextSession != nil { + buf, err := setupContextStream(progressOutput, contextSession, buildBuffer, buildInput.contextDir) + if err != nil { return err } - - buf := newBufferedWriter(syncDone, buildBuffer.output) + // TODO: move this to a method on bufferedWriter defer func() { select { case <-buf.flushed: case <-ctx.Done(): } }() - // TODO: this overrides setting a buffer on quiet, is that correct? - buildBuffer.SetOutput(buf) - remote = clientSessionRemote - body = buildCtx + body = buildInput.dockerfileCtx + } else { + body = setupRequestBody(progressOutput, buildInput.buildCtx) } configFile := dockerCli.ConfigFile() @@ -350,7 +235,7 @@ func runBuild(dockerCli command.Cli, options buildOptions) error { CPUQuota: options.cpuQuota, CPUPeriod: options.cpuPeriod, CgroupParent: options.cgroupParent, - Dockerfile: relDockerfile, + Dockerfile: buildInput.dockerfilePath, ShmSize: options.shmSize.Value(), Ulimits: options.ulimits.GetList(), BuildArgs: configFile.ParseProxyConfig(dockerCli.Client().DaemonHost(), options.buildArgs.GetAll()), @@ -365,16 +250,7 @@ func runBuild(dockerCli command.Cli, options buildOptions) error { RemoteContext: remote, } - if s != nil { - go func() { - logrus.Debugf("running session: %v", s.UUID()) - if err := s.Run(ctx, dockerCli.Client().DialSession); err != nil { - logrus.Error(err) - cancel() // cancel progress context - } - }() - buildOptions.SessionID = s.UUID() - } + buildOptions.SessionID = startSession(ctx, contextSession, dockerCli.Client(), cancel) response, err := dockerCli.Client().ImageBuild(ctx, body, buildOptions) if err != nil { @@ -390,22 +266,14 @@ func runBuild(dockerCli command.Cli, options buildOptions) error { } warnBuildWindowsToLinux(dockerCli.Out(), response.OSType, options.quiet) + buildBuffer.PrintImageIDIfQuiet() + if err := idFile.Save(imageID); err != nil { return err } - if command.IsTrusted() { - // Since the build was successful, now we must tag any of the resolved - // images from the above Dockerfile rewrite. - for _, resolved := range resolvedTags { - if err := TagTrusted(ctx, dockerCli, resolved.digestRef, resolved.tagRef); err != nil { - return err - } - } - } - - return nil + return tagTrustedImagesFromBuild(ctx, dockerCli, resolvedTags) } func isLocalDir(c string) bool { @@ -413,7 +281,31 @@ func isLocalDir(c string) bool { return err == nil } -type translatorFunc func(reference.NamedTagged) (reference.Canonical, error) +func setupRequestBody(progressOutput progress.Output, buildCtx io.ReadCloser) io.Reader { + return progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon") +} + +func setupContextStream(progressOutput progress.Output, contextSession *session.Session, buildBuffer *buildBuffer, contextDir string) (*bufferedWriter, error) { + syncDone := make(chan error) // used to signal first progress reporting completed. + // progress would also send errors but don't need it here as errors + // are handled by session.Run() and ImageBuild() + if err := addDirToSession(contextSession, contextDir, progressOutput, syncDone); err != nil { + return nil, err + } + + buf := newBufferedWriter(syncDone, buildBuffer.output) + buildBuffer.SetOutput(buf) + return buf, nil +} + +// Setup an upload progress bar +func newProgaressOutput(out *command.OutStream, progressWriter io.Writer) progress.Output { + progressOutput := streamformatter.NewProgressOutput(progressWriter) + if !out.IsTerminal() { + progressOutput = &lastProgressOutput{output: progressOutput} + } + return progressOutput +} // validateTag checks if the given image name can be resolved. func validateTag(rawRepo string) (string, error) { @@ -427,13 +319,6 @@ func validateTag(rawRepo string) (string, error) { var dockerfileFromLinePattern = regexp.MustCompile(`(?i)^[\s]*FROM[ \f\r\t\v]+(?P[^ \f\r\t\v\n#]+)`) -// resolvedTag records the repository, tag, and resolved digest reference -// from a Dockerfile rewrite. -type resolvedTag struct { - digestRef reference.Canonical - tagRef reference.NamedTagged -} - // rewriteDockerfileFrom rewrites the given Dockerfile by resolving images in // "FROM " instructions to a digest reference. `translator` is a // function that takes a repository name and tag reference and returns a @@ -583,3 +468,17 @@ func warnBuildWindowsToLinux(out io.Writer, osType string, quiet bool) { "files and directories.") } } + +func tagTrustedImagesFromBuild(ctx context.Context, dockerCli command.Cli, resolvedTags []*resolvedTag) error { + if !command.IsTrusted() { + return nil + } + // Since the build was successful, now we must tag any of the resolved + // images from the above Dockerfile rewrite. + for _, resolved := range resolvedTags { + if err := TagTrusted(ctx, dockerCli, resolved.digestRef, resolved.tagRef); err != nil { + return err + } + } + return nil +} diff --git a/cli/command/image/build_context.go b/cli/command/image/build_context.go new file mode 100644 index 000000000000..0d62744c748e --- /dev/null +++ b/cli/command/image/build_context.go @@ -0,0 +1,165 @@ +package image + +import ( + "bytes" + "io" + "io/ioutil" + "os" + + "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/command/image/build" + "github.com/docker/distribution/reference" + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/urlutil" + "github.com/pkg/errors" + "golang.org/x/net/context" +) + +type buildInput struct { + buildCtx io.ReadCloser + dockerfileCtx io.ReadCloser + dockerfilePath string + contextDir string + cleanups []func() +} + +func (bi *buildInput) cleanup() { + for _, fnc := range bi.cleanups { + fnc() + } +} + +func (bi *buildInput) addCleanup(fnc func()) { + bi.cleanups = append([]func(){fnc}, bi.cleanups...) +} + +func setupContextAndDockerfile(dockerCli command.Cli, buildBuffer *buildBuffer, options buildOptions) (*buildInput, error) { + result := &buildInput{} + + if options.dockerfileFromStdin() { + if options.contextFromStdin() { + return result, errors.New("invalid argument: can't use stdin for both build context and dockerfile") + } + result.dockerfileCtx = dockerCli.In() + } + + var err error + specifiedContext := options.context + switch { + case options.contextFromStdin(): + // buildCtx is tar archive. if stdin was dockerfile then it is wrapped + result.buildCtx, result.dockerfilePath, err = build.GetContextFromReader(dockerCli.In(), options.dockerfileName) + case isLocalDir(specifiedContext): + result.contextDir, result.dockerfilePath, err = build.GetContextFromLocalDir(specifiedContext, options.dockerfileName) + case urlutil.IsGitURL(specifiedContext): + result.contextDir, result.dockerfilePath, err = build.GetContextFromGitURL(specifiedContext, options.dockerfileName) + if result.contextDir != "" { + result.addCleanup(func() { os.RemoveAll(result.contextDir) }) + } + case urlutil.IsURL(specifiedContext): + result.buildCtx, result.dockerfilePath, err = build.GetContextFromURL(buildBuffer.progress, specifiedContext, options.dockerfileName) + default: + return result, errors.Errorf("unable to prepare context: path %q not found", specifiedContext) + } + + if err != nil { + buildBuffer.PrintProgressOnError() + return result, errors.Errorf("unable to prepare context: %s", err) + } + + if options.stream { + return createStreamBuildInput(result, specifiedContext) + } + + // Context is a git url, or a local dir + if result.buildCtx == nil { + result.buildCtx, err = createBuildContextFromLocalDir(result.contextDir, result.dockerfilePath, options.dockerfileFromStdin()) + if err != nil { + return result, err + } + } + + if result.dockerfileCtx != nil { + result.buildCtx, result.dockerfilePath, err = build.AddDockerfileToBuildContext(result.dockerfileCtx, result.buildCtx) + if err != nil { + return result, err + } + } + + return result, err +} + +func createBuildContextFromLocalDir(contextDir string, dockerfilePath string, dockerfileFromStdin bool) (io.ReadCloser, error) { + excludes, err := build.ReadDockerignore(contextDir) + if err != nil { + return nil, err + } + + if err := build.ValidateContextDirectory(contextDir, excludes); err != nil { + return nil, errors.Errorf("error checking context: '%s'.", err) + } + + // And canonicalize dockerfile name to a platform-independent one + dockerfilePath, err = archive.CanonicalTarNameForPath(dockerfilePath) + if err != nil { + return nil, errors.Errorf("cannot canonicalize dockerfile path %s: %v", dockerfilePath, err) + } + + excludes = build.TrimBuildFilesFromExcludes(excludes, dockerfilePath, dockerfileFromStdin) + return archive.TarWithOptions(contextDir, &archive.TarOptions{ + ExcludePatterns: excludes, + }) +} + +func createStreamBuildInput(result *buildInput, context string) (*buildInput, error) { + if result.buildCtx != nil { + return result, errors.Errorf("stream is not supported for context: %s", context) + } + + var err error + if result.dockerfileCtx == nil { + result.dockerfileCtx, err = os.Open(result.dockerfilePath) + if err != nil { + return result, errors.Wrapf(err, "failed to open %s", result.dockerfilePath) + } + result.addCleanup(func() { result.dockerfileCtx.Close() }) + } + return result, nil +} + +type translatorFunc func(reference.NamedTagged) (reference.Canonical, error) + +// resolvedTag records the repository, tag, and resolved digest reference +// from a Dockerfile rewrite. +type resolvedTag struct { + digestRef reference.Canonical + tagRef reference.NamedTagged +} + +func updateBuildInputForContentTrust(ctx context.Context, dockerCli command.Cli, buildInput *buildInput) ([]*resolvedTag, error) { + if !command.IsTrusted() { + return nil, nil + } + translator := func(ref reference.NamedTagged) (reference.Canonical, error) { + return TrustedReference(ctx, dockerCli, ref, nil) + } + // if there is a tar wrapper, the dockerfile needs to be replaced inside it + if buildInput.buildCtx != nil { + // Wrap the tar archive to replace the Dockerfile entry with the rewritten + // Dockerfile which uses trusted pulls. + buildCtx, resolvedTags := rewriteDockerfileForContentTrust(buildInput.buildCtx, buildInput.dockerfilePath, translator) + buildInput.buildCtx = buildCtx + return resolvedTags, nil + } + + if buildInput.dockerfileCtx != nil { + // if there was not archive context still do the possible replacements in Dockerfile + newDockerfile, _, err := rewriteDockerfileFrom(buildInput.dockerfileCtx, translator) + if err != nil { + return nil, err + } + buildInput.dockerfileCtx = ioutil.NopCloser(bytes.NewBuffer(newDockerfile)) + // TODO: shouldn't this also return resolvedTags? + } + return nil, nil +} diff --git a/cli/command/image/build_session.go b/cli/command/image/build_session.go index 86fe95782e13..774995b912c0 100644 --- a/cli/command/image/build_session.go +++ b/cli/command/image/build_session.go @@ -13,14 +13,17 @@ import ( "sync" "time" + "github.com/Sirupsen/logrus" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/image/build" cliconfig "github.com/docker/cli/cli/config" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/client" "github.com/docker/docker/client/session" "github.com/docker/docker/client/session/filesync" "github.com/docker/docker/pkg/progress" "github.com/pkg/errors" + "golang.org/x/net/context" "golang.org/x/time/rate" ) @@ -30,21 +33,36 @@ func isSessionSupported(dockerCli command.Cli) bool { return dockerCli.ServerInfo().HasExperimental && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.31") } -func trySession(dockerCli command.Cli, contextDir string) (*session.Session, error) { +func newBuildContextSession(dockerCli command.Cli, contextDir string) (*session.Session, error) { var s *session.Session - if isSessionSupported(dockerCli) { - sharedKey, err := getBuildSharedKey(contextDir) - if err != nil { - return nil, errors.Wrap(err, "failed to get build shared key") - } - s, err = session.NewSession(filepath.Base(contextDir), sharedKey) - if err != nil { - return nil, errors.Wrap(err, "failed to create session") - } + if !isSessionSupported(dockerCli) { + return s, errors.New("stream is not supported") + } + sharedKey, err := getBuildSharedKey(contextDir) + if err != nil { + return nil, errors.Wrap(err, "failed to get build shared key") + } + s, err = session.NewSession(filepath.Base(contextDir), sharedKey) + if err != nil { + return nil, errors.Wrap(err, "failed to create session") } return s, nil } +func startSession(ctx context.Context, contextSession *session.Session, client client.APIClient, cancel func()) string { + if contextSession == nil { + return "" + } + go func() { + logrus.Debugf("running session: %v", contextSession.UUID()) + if err := contextSession.Run(ctx, client.DialSession); err != nil { + logrus.Error(err) + cancel() // cancel progress context + } + }() + return contextSession.UUID() +} + func addDirToSession(session *session.Session, contextDir string, progressOutput progress.Output, done chan error) error { excludes, err := build.ReadDockerignore(contextDir) if err != nil {