fix: premature Close() of docker client (#1066)

* Fix docker client lifecycle

Avoid premature Close() of docker client.

Signed-off-by: Matej Vasek <mvasek@redhat.com>

* Guard for usage of docker client after close

Signed-off-by: Matej Vasek <mvasek@redhat.com>

.codecov.yaml

@@ -2,3 +2,5 @@ ignore:
   - third_party
   - vendor
   - testdata
+  - docker/zz_close_guarding_client_generated.go
+

.gitattributes

@@ -2,3 +2,5 @@ testdata/repository-a.git/objects/*/* ignore-lint=true
 testdata/repository.git/objects/*/* ignore-lint=true
 version.txt linguist-generated=true
 zz_filesystem_generated.go linguist-generated=true
+docker/zz_close_guarding_client_generated.go linguist-generated=true
+

buildpacks/builder.go

@@ -9,14 +9,13 @@ import (
 	"runtime"
 	"strings"
 
+	"github.com/Masterminds/semver"
+	pack "github.com/buildpacks/pack/pkg/client"
+	"github.com/buildpacks/pack/pkg/logging"
 	"github.com/docker/docker/client"
 
 	fn "knative.dev/kn-plugin-func"
 	"knative.dev/kn-plugin-func/docker"
-
-	"github.com/Masterminds/semver"
-	pack "github.com/buildpacks/pack/pkg/client"
-	"github.com/buildpacks/pack/pkg/logging"
 )
 
 var (
@@ -109,16 +108,28 @@ func (b *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 		opts.ContainerConfig.Network = "host"
 	}
 
+	var impl = b.impl
 	// Instantate the pack build client implementation
 	// (and update build opts as necessary)
-	if b.impl == nil {
-		if b.impl, err = newImpl(ctx, &opts, b.logger); err != nil {
-			return
+	if impl == nil {
+		var (
+			cli        client.CommonAPIClient
+			dockerHost string
+		)
+
+		cli, dockerHost, err = docker.NewClient(client.DefaultDockerHost)
+		if err != nil {
+			return fmt.Errorf("cannot craete docker client: %w", err)
+		}
+		defer cli.Close()
+
+		if impl, err = newImpl(ctx, cli, dockerHost, &opts, b.logger); err != nil {
+			return fmt.Errorf("cannot create pack client: %w", err)
 		}
 	}
 
 	// Perform the build
-	if err = b.impl.Build(ctx, opts); err != nil {
+	if err = impl.Build(ctx, opts); err != nil {
 		if ctx.Err() != nil {
 			return // SIGINT
 		} else if !b.verbose {
@@ -130,13 +141,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 
 // newImpl returns an instance of the builder implementatoin.  Note that this
 // also mutates the provided options' DockerHost and TrustBuilder.
-func newImpl(ctx context.Context, opts *pack.BuildOptions, logger io.Writer) (impl Impl, err error) {
-	cli, dockerHost, err := docker.NewClient(client.DefaultDockerHost)
-	if err != nil {
-		return
-	}
-	defer cli.Close()
-
+func newImpl(ctx context.Context, cli client.CommonAPIClient, dockerHost string, opts *pack.BuildOptions, logger io.Writer) (impl Impl, err error) {
 	opts.DockerHost = dockerHost
 
 	daemonIsPodmanPreV330, err := podmanPreV330(ctx, cli)

docker/close_guarding_client.go

@@ -0,0 +1,21 @@
+package docker
+
+import (
+	"sync"
+
+	"github.com/docker/docker/client"
+)
+
+// Client that panics when used after Close()
+type closeGuardingClient struct {
+	pimpl  client.CommonAPIClient
+	m      sync.RWMutex
+	closed bool
+}
+
+func (c *closeGuardingClient) Close() error {
+	c.m.Lock()
+	defer c.m.Unlock()
+	c.closed = true
+	return c.pimpl.Close()
+}

docker/docker_client.go

@@ -44,6 +44,7 @@ func NewClient(defaultHost string) (dockerClient client.CommonAPIClient, dockerH
 			return
 		case os.IsNotExist(err):
 			dockerClient, dockerHost, err = newClientWithPodmanService()
+			dockerClient = &closeGuardingClient{pimpl: dockerClient}
 			return
 		}
 	}
@@ -61,6 +62,7 @@ func NewClient(defaultHost string) (dockerClient client.CommonAPIClient, dockerH
 
 	if !isSSH {
 		dockerClient, err = client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
+		dockerClient = &closeGuardingClient{pimpl: dockerClient}
 		return
 	}
 
@@ -98,6 +100,7 @@ func NewClient(defaultHost string) (dockerClient client.CommonAPIClient, dockerH
 		}
 	}
 
+	dockerClient = &closeGuardingClient{pimpl: dockerClient}
 	return dockerClient, dockerHost, err
 }
 

go.mod

@@ -22,6 +22,7 @@ require (
 	github.com/google/uuid v1.3.0
 	github.com/hinshun/vt10x v0.0.0-20180809195222-d55458df857c
 	github.com/mitchellh/go-homedir v1.1.0
+	github.com/opencontainers/image-spec v1.0.3-0.20220114050600-8b9d41f48198
 	github.com/openshift/source-to-image v1.3.1
 	github.com/ory/viper v1.7.5
 	github.com/pkg/errors v0.9.1

s2i/builder.go

@@ -118,14 +118,18 @@ func (b *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 
 	cfg.AsDockerfile = filepath.Join(tmp, "Dockerfile")
 
-	if b.cli == nil {
-		b.cli, _, err = docker.NewClient(dockerClient.DefaultDockerHost)
+	var client = b.cli
+	if client == nil {
+		var c dockerClient.CommonAPIClient
+		c, _, err = docker.NewClient(dockerClient.DefaultDockerHost)
 		if err != nil {
 			return fmt.Errorf("cannot create docker client: %w", err)
 		}
+		defer c.Close()
+		client = c
 	}
 
-	scriptURL, err := s2iScriptURL(ctx, b.cli, cfg.BuilderImage)
+	scriptURL, err := s2iScriptURL(ctx, client, cfg.BuilderImage)
 	if err != nil {
 		return fmt.Errorf("cannot get s2i script url: %w", err)
 	}
@@ -157,16 +161,17 @@ func (b *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 		return errors.New("Unable to build via the s2i builder.")
 	}
 
+	var impl = b.impl
 	// Create the S2I builder instance if not overridden
-	if b.impl == nil {
-		b.impl, _, err = strategies.Strategy(nil, cfg, build.Overrides{})
+	if impl == nil {
+		impl, _, err = strategies.Strategy(nil, cfg, build.Overrides{})
 		if err != nil {
 			return fmt.Errorf("cannot create s2i builder: %w", err)
 		}
 	}
 
 	// Perform the build
-	result, err := b.impl.Build(cfg)
+	result, err := impl.Build(cfg)
 	if err != nil {
 		return
 	}
@@ -223,7 +228,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 		Tags: []string{f.Image},
 	}
 
-	resp, err := b.cli.ImageBuild(ctx, pr, opts)
+	resp, err := client.ImageBuild(ctx, pr, opts)
 	if err != nil {
 		return fmt.Errorf("cannot build the app image: %w", err)
 	}

vendor/modules.txt

@@ -608,6 +608,7 @@ github.com/morikuni/aec
 # github.com/opencontainers/go-digest v1.0.0
 github.com/opencontainers/go-digest
 # github.com/opencontainers/image-spec v1.0.3-0.20220114050600-8b9d41f48198
+## explicit
 github.com/opencontainers/image-spec/specs-go
 github.com/opencontainers/image-spec/specs-go/v1
 # github.com/opencontainers/runc v1.1.0