From d31d44f473592213e09dc7e805b4f6071b81ed51 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 14 Oct 2015 13:11:34 -0400 Subject: [PATCH] Allow S2I behavior to be overriden by caller Origin needs to be able to download and assemble the source itself and then pass it to s2i. This commit adds changes to allow an Overrides object to be passed to the s2i initializer code to provide a custom Downloader. This also makes changes to allow better reuse of upstream s2i code and removes a write to disk for the intermediate tar (which can be streamed instead). --- pkg/build/interfaces.go | 6 ++ pkg/build/strategies/layered/layered.go | 3 +- pkg/build/strategies/onbuild/onbuild.go | 24 +++++--- pkg/build/strategies/sti/sti.go | 56 +++++++++++------- pkg/build/strategies/sti/sti_test.go | 75 ++++++++++++++++++------- pkg/build/strategies/sti/usage.go | 2 +- pkg/build/strategies/strategies.go | 11 +++- pkg/tar/tar.go | 6 +- pkg/test/tar.go | 16 ++++++ 9 files changed, 147 insertions(+), 52 deletions(-) diff --git a/pkg/build/interfaces.go b/pkg/build/interfaces.go index 99549b24b..aa3517f7a 100644 --- a/pkg/build/interfaces.go +++ b/pkg/build/interfaces.go @@ -62,3 +62,9 @@ type SourceHandler interface { type LayeredDockerBuilder interface { Builder } + +// Overrides are interfaces that may be passed into build strategies to +// alter the behavior of a strategy. +type Overrides struct { + Downloader Downloader +} diff --git a/pkg/build/strategies/layered/layered.go b/pkg/build/strategies/layered/layered.go index f8e5d2990..c479a1c9b 100644 --- a/pkg/build/strategies/layered/layered.go +++ b/pkg/build/strategies/layered/layered.go @@ -27,7 +27,7 @@ type Layered struct { scripts build.ScriptsHandler } -func New(config *api.Config, scripts build.ScriptsHandler) (*Layered, error) { +func New(config *api.Config, scripts build.ScriptsHandler, overrides build.Overrides) (*Layered, error) { d, err := docker.New(config.DockerConfig, config.PullAuthentication) if err != nil { return nil, err @@ -82,6 +82,7 @@ func (b *Layered) CreateDockerfile(config *api.Config) error { return nil } +// TODO: this should stop generating a file, and instead stream the tar. func (b *Layered) SourceTar(config *api.Config) (io.ReadCloser, error) { uploadDir := filepath.Join(config.WorkingDir, "upload") tarFileName, err := b.tar.CreateTarFile(b.config.WorkingDir, uploadDir) diff --git a/pkg/build/strategies/onbuild/onbuild.go b/pkg/build/strategies/onbuild/onbuild.go index 24254c208..de325f334 100644 --- a/pkg/build/strategies/onbuild/onbuild.go +++ b/pkg/build/strategies/onbuild/onbuild.go @@ -38,7 +38,7 @@ type onBuildSourceHandler struct { } // New returns a new instance of OnBuild builder -func New(config *api.Config) (*OnBuild, error) { +func New(config *api.Config, overrides build.Overrides) (*OnBuild, error) { dockerHandler, err := docker.New(config.DockerConfig, config.PullAuthentication) if err != nil { return nil, err @@ -50,15 +50,25 @@ func New(config *api.Config) (*OnBuild, error) { tar: tar.New(), } // Use STI Prepare() and download the 'run' script optionally. - s, err := sti.New(config) + s, err := sti.New(config, overrides) s.SetScripts([]string{}, []string{api.Assemble, api.Run}) - downloader, sourceUrl, err := scm.DownloaderForSource(config.Source) - if err != nil { - return nil, err + + downloader := overrides.Downloader + if downloader == nil { + d, sourceURL, err := scm.DownloaderForSource(config.Source) + if err != nil { + return nil, err + } + downloader = d + config.Source = sourceURL + } + + b.source = onBuildSourceHandler{ + Downloader: downloader, + Preparer: s, + Ignorer: &ignore.DockerIgnorer{}, } - config.Source = sourceUrl - b.source = onBuildSourceHandler{downloader, s, &ignore.DockerIgnorer{}} b.garbage = &build.DefaultCleaner{b.fs, b.docker} return b, nil } diff --git a/pkg/build/strategies/sti/sti.go b/pkg/build/strategies/sti/sti.go index ca3c0d789..5d216869f 100644 --- a/pkg/build/strategies/sti/sti.go +++ b/pkg/build/strategies/sti/sti.go @@ -6,6 +6,7 @@ import ( "path/filepath" "regexp" "strings" + "sync" "github.com/golang/glog" "github.com/openshift/source-to-image/pkg/api" @@ -66,7 +67,7 @@ type STI struct { // If the layeredBuilder parameter is specified, then the builder provided will // be used for the case that the base Docker image does not have 'tar' or 'bash' // installed. -func New(req *api.Config) (*STI, error) { +func New(req *api.Config, overrides build.Overrides) (*STI, error) { docker, err := dockerpkg.New(req.DockerConfig, req.PullAuthentication) if err != nil { return nil, err @@ -99,13 +100,18 @@ func New(req *api.Config) (*STI, error) { // The sources are downloaded using the GIT downloader. // TODO: Add more SCM in future. - b.source, req.Source, err = scm.DownloaderForSource(req.Source) - if err != nil { - return nil, err + b.source = overrides.Downloader + if b.source == nil { + downloader, sourceURL, err := scm.DownloaderForSource(req.Source) + if err != nil { + return nil, err + } + b.source = downloader + req.Source = sourceURL } b.garbage = &build.DefaultCleaner{b.fs, b.docker} - b.layered, err = layered.New(req, b) + b.layered, err = layered.New(req, b, overrides) // Set interfaces b.preparer = b @@ -168,8 +174,10 @@ func (b *STI) Build(config *api.Config) (*api.Result, error) { // struct Build func leverages the STI struct Prepare func directly below func (b *STI) Prepare(config *api.Config) error { var err error - if config.WorkingDir, err = b.fs.CreateWorkingDirectory(); err != nil { - return err + if len(config.WorkingDir) == 0 { + if config.WorkingDir, err = b.fs.CreateWorkingDirectory(); err != nil { + return err + } } b.result = &api.Result{ @@ -374,18 +382,6 @@ func (b *STI) Execute(command string, config *api.Config) error { buildEnv := append(scripts.ConvertEnvironment(env), b.generateConfigEnv()...) - uploadDir := filepath.Join(config.WorkingDir, "upload") - tarFileName, err := b.tar.CreateTarFile(config.WorkingDir, uploadDir) - if err != nil { - return err - } - - tarFile, err := b.fs.Open(tarFileName) - if err != nil { - return err - } - defer tarFile.Close() - errOutput := "" outReader, outWriter := io.Pipe() errReader, errWriter := io.Pipe() @@ -410,8 +406,28 @@ func (b *STI) Execute(command string, config *api.Config) error { Env: buildEnv, PostExec: b.postExecutor, } + if !config.LayeredBuild { - opts.Stdin = tarFile + wg := sync.WaitGroup{} + wg.Add(1) + uploadDir := filepath.Join(config.WorkingDir, "upload") + + // TODO: be able to pass a stream directly to the Docker build to avoid the double temp hit + r, w := io.Pipe() + go func() { + var err error + defer func() { + w.CloseWithError(err) + if r := recover(); r != nil { + glog.Errorf("recovered panic: %#v", r) + } + wg.Done() + }() + err = b.tar.CreateTarStream(uploadDir, false, w) + }() + + opts.Stdin = r + defer wg.Wait() } go func(reader io.Reader) { diff --git a/pkg/build/strategies/sti/sti_test.go b/pkg/build/strategies/sti/sti_test.go index 6d21b7b68..96f78c367 100644 --- a/pkg/build/strategies/sti/sti_test.go +++ b/pkg/build/strategies/sti/sti_test.go @@ -3,6 +3,7 @@ package sti import ( "errors" "fmt" + "io" "reflect" "testing" @@ -10,6 +11,7 @@ import ( "github.com/openshift/source-to-image/pkg/build" stierr "github.com/openshift/source-to-image/pkg/errors" "github.com/openshift/source-to-image/pkg/ignore" + "github.com/openshift/source-to-image/pkg/scm/file" "github.com/openshift/source-to-image/pkg/scm/git" "github.com/openshift/source-to-image/pkg/test" ) @@ -24,6 +26,7 @@ type FakeSTI struct { ExistsError error BuildRequest *api.Config BuildResult *api.Result + DownloadError error SaveArtifactsCalled bool SaveArtifactsError error FetchSourceCalled bool @@ -103,8 +106,8 @@ func (f *FakeSTI) fetchSource() error { return f.FetchSourceError } -func (f *FakeSTI) Download(*api.Config) error { - return nil +func (f *FakeSTI) Download(*api.Config) (*api.SourceInfo, error) { + return nil, f.DownloadError } func (f *FakeSTI) Execute(command string, r *api.Config) error { @@ -131,6 +134,41 @@ func (f *FakeDockerBuild) Build(*api.Config) (*api.Result, error) { return nil, f.LayeredBuildError } +func TestDefaultSource(t *testing.T) { + config := &api.Config{ + Source: "file://.", + DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"}, + } + sti, err := New(config, build.Overrides{}) + if err != nil { + t.Fatal(err) + } + if config.Source == "" { + t.Errorf("Config.Source not set: %v", config.Source) + } + if _, ok := sti.source.(*file.File); !ok || sti.source == nil { + t.Errorf("Source interface not set: %#v", sti.source) + } +} + +func TestOverrides(t *testing.T) { + fd := &FakeSTI{} + sti, err := New( + &api.Config{ + DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"}, + }, + build.Overrides{ + Downloader: fd, + }, + ) + if err != nil { + t.Fatal(err) + } + if sti.source != fd { + t.Errorf("Override of downloader not set: %#v", sti) + } +} + func TestBuild(t *testing.T) { incrementalTest := []bool{false, true} for _, incremental := range incrementalTest { @@ -681,7 +719,8 @@ func TestExecuteOK(t *testing.T) { if err != nil { t.Errorf("Unexpected error returned: %v", err) } - if th.CreateTarBase != "/working-dir" { + th = rh.tar.(*test.FakeTar).Copy() + if th.CreateTarBase != "" { t.Errorf("Unexpected tar base directory: %s", th.CreateTarBase) } if th.CreateTarDir != "/working-dir/upload" { @@ -691,26 +730,26 @@ func TestExecuteOK(t *testing.T) { if !ok { t.Fatalf("Unable to convert %v to FakeFilesystem", rh.fs) } - if fh.OpenFile != "/working-dir/test.tar" { - t.Errorf("Unexpected file opened: %s", fh.OpenFile) + if fh.OpenFile != "" { + t.Fatalf("Unexpected file opened: %s", fh.OpenFile) } - if !fh.OpenFileResult.CloseCalled { - t.Errorf("Tar file was not closed.") + if fh.OpenFileResult != nil { + t.Errorf("Tar file was opened.") } ro := fd.RunContainerOpts if ro.Image != rh.config.BuilderImage { t.Errorf("Unexpected Image passed to RunContainer") } - if ro.Stdin != fh.OpenFileResult { - t.Errorf("Unexpected input stream: %#v", fd.RunContainerOpts.Stdin) + if _, ok := ro.Stdin.(*io.PipeReader); !ok { + t.Errorf("Unexpected input stream: %#v", ro.Stdin) } if !ro.PullImage { t.Errorf("PullImage is not true for RunContainer") } if ro.Command != "test-command" { t.Errorf("Unexpected command passed to RunContainer: %s", - fd.RunContainerOpts.Command) + ro.Command) } if pe.PostExecuteContainerID != "1234" { t.Errorf("PostExecutor not called with expected ID: %s", @@ -725,17 +764,15 @@ func TestExecuteErrorCreateTarFile(t *testing.T) { rh := newFakeSTI(&FakeSTI{}) rh.tar.(*test.FakeTar).CreateTarError = errors.New("CreateTarError") err := rh.Execute("test-command", rh.config) - if err == nil || err.Error() != "CreateTarError" { + if err != nil { t.Errorf("An error was expected for CreateTarFile, but got different: %v", err) } -} - -func TestExecuteErrorOpenTarFile(t *testing.T) { - rh := newFakeSTI(&FakeSTI{}) - rh.fs.(*test.FakeFileSystem).OpenError = errors.New("OpenTarError") - err := rh.Execute("test-command", rh.config) - if err == nil || err.Error() != "OpenTarError" { - t.Errorf("An error was expected for OpenTarFile, but got different: %v", err) + ro := rh.docker.(*test.FakeDocker).RunContainerOpts + if ro.Stdin == nil { + t.Fatalf("Stream not passed to Docker interface") + } + if _, err := ro.Stdin.Read(make([]byte, 5)); err == nil || err.Error() != "CreateTarError" { + t.Errorf("An error was expected for CreateTarFile, but got different: %#v", ro) } } diff --git a/pkg/build/strategies/sti/usage.go b/pkg/build/strategies/sti/usage.go index f41c19944..570d8503d 100644 --- a/pkg/build/strategies/sti/usage.go +++ b/pkg/build/strategies/sti/usage.go @@ -21,7 +21,7 @@ type Usage struct { // NewUsage creates a new instance of the default Usage implementation func NewUsage(config *api.Config) (*Usage, error) { - b, err := New(config) + b, err := New(config, build.Overrides{}) if err != nil { return nil, err } diff --git a/pkg/build/strategies/strategies.go b/pkg/build/strategies/strategies.go index ba6de142c..899ace278 100644 --- a/pkg/build/strategies/strategies.go +++ b/pkg/build/strategies/strategies.go @@ -13,17 +13,24 @@ import ( ) // GetStrategy decides what build strategy will be used for the STI build. +// TODO: deprecated, use Strategy() instead func GetStrategy(config *api.Config) (build.Builder, error) { + return Strategy(config, build.Overrides{}) +} + +// Strategy creates the appropriate build strategy for the provided config, using +// the overrides provided. Not all strategies support all overrides. +func Strategy(config *api.Config, overrides build.Overrides) (build.Builder, error) { image, err := GetBuilderImage(config) if err != nil { return nil, err } if image.OnBuild { - return onbuild.New(config) + return onbuild.New(config, overrides) } - return sti.New(config) + return sti.New(config, overrides) } // GetBuilderImage processes the config and performs operations necessary to make diff --git a/pkg/tar/tar.go b/pkg/tar/tar.go index 671fc5056..bc5c2fd00 100644 --- a/pkg/tar/tar.go +++ b/pkg/tar/tar.go @@ -36,7 +36,8 @@ type Tar interface { CreateTarFile(base, dir string) (string, error) // CreateTarStream creates a tar from the given directory - // and streams it to the given writer + // and streams it to the given writer. + // An error is returned if an error occurs during streaming. CreateTarStream(dir string, includeDirInPath bool, writer io.Writer) error // ExtractTarStream extracts files from a given tar stream. @@ -87,6 +88,7 @@ func (t *stiTar) shouldExclude(path string) bool { // CreateTarStream creates a tar stream on the given writer from // the given directory while excluding files that match the given // exclusion pattern. +// TODO: this should encapsulate the goroutine that generates the stream. func (t *stiTar) CreateTarStream(dir string, includeDirInPath bool, writer io.Writer) error { tarWriter := tar.NewWriter(writer) defer tarWriter.Close() @@ -148,7 +150,7 @@ func (t *stiTar) writeTarHeader(tarWriter *tar.Writer, dir string, path string, prefix = filepath.Dir(prefix) } header.Name = filepath.ToSlash(path[1+len(prefix):]) - glog.V(3).Infof("Adding to tar: %s as %s", path, header.Name) + glog.V(5).Infof("Adding to tar: %s as %s", path, header.Name) if err = tarWriter.WriteHeader(header); err != nil { return err } diff --git a/pkg/test/tar.go b/pkg/test/tar.go index 04391c54d..33258bb1a 100644 --- a/pkg/test/tar.go +++ b/pkg/test/tar.go @@ -3,6 +3,7 @@ package test import ( "io" "regexp" + "sync" ) // FakeTar provides a fake UNIX tar interface @@ -15,10 +16,21 @@ type FakeTar struct { ExtractTarDir string ExtractTarReader io.Reader ExtractTarError error + + lock sync.Mutex +} + +func (f *FakeTar) Copy() *FakeTar { + f.lock.Lock() + defer f.lock.Unlock() + n := *f + return &n } // CreateTarFile creates a new fake UNIX tar file func (f *FakeTar) CreateTarFile(base, dir string) (string, error) { + f.lock.Lock() + defer f.lock.Unlock() f.CreateTarBase = base f.CreateTarDir = dir return f.CreateTarResult, f.CreateTarError @@ -26,6 +38,8 @@ func (f *FakeTar) CreateTarFile(base, dir string) (string, error) { // ExtractTarStream streams a content of fake tar func (f *FakeTar) ExtractTarStream(dir string, reader io.Reader) error { + f.lock.Lock() + defer f.lock.Unlock() f.ExtractTarDir = dir f.ExtractTarReader = reader return f.ExtractTarError @@ -35,6 +49,8 @@ func (f *FakeTar) SetExclusionPattern(*regexp.Regexp) { } func (f *FakeTar) CreateTarStream(dir string, includeDirInPath bool, writer io.Writer) error { + f.lock.Lock() + defer f.lock.Unlock() f.CreateTarDir = dir return f.CreateTarError }