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

@@ -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
+}

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

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,13 +50,24 @@ 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,
+		s,
+		&ignore.DockerIgnorer{},
 	}
-	config.Source = sourceUrl
 
 	b.source = onBuildSourceHandler{downloader, s, &ignore.DockerIgnorer{}}
 	b.garbage = &build.DefaultCleaner{b.fs, b.docker}

pkg/build/strategies/sti/sti.go

@@ -66,7 +66,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 +99,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 +173,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 +381,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 +405,24 @@ func (b *STI) Execute(command string, config *api.Config) error {
 		Env:             buildEnv,
 		PostExec:        b.postExecutor,
 	}
+
 	if !config.LayeredBuild {
-		opts.Stdin = tarFile
+		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)
+				}
+			}()
+			err = b.tar.CreateTarStream(uploadDir, false, w)
+		}()
+
+		opts.Stdin = r
 	}
 
 	go func(reader io.Reader) {

pkg/build/strategies/sti/sti_test.go

@@ -3,6 +3,7 @@ package sti
 import (
 	"errors"
 	"fmt"
+	"io"
 	"reflect"
 	"testing"
 
@@ -681,36 +682,37 @@ 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" {
+	if th.CreateTarDir != "" {
 		t.Errorf("Unexpected tar directory: %s", th.CreateTarDir)
 	}
 	fh, ok := rh.fs.(*test.FakeFileSystem)
 	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 +727,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)
 	}
 }
 

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
 	}

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

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.
@@ -148,7 +149,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
 	}

pkg/test/tar.go

@@ -3,6 +3,7 @@ package test
 import (
 	"io"
 	"regexp"
+	"sync"
 )
 
 // FakeTar provides a fake UNIX tar interface
@@ -15,17 +16,30 @@ 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
 }
 
 // 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
 }