From af934e422a86858b721165887e21383eb5b2e608 Mon Sep 17 00:00:00 2001 From: Vaughn Dice Date: Wed, 22 Apr 2020 15:13:11 -0600 Subject: [PATCH 1/4] ref(docker.go): export ApplyConfigurationOptions Signed-off-by: Vaughn Dice --- driver/docker/docker.go | 5 +++-- driver/docker/docker_test.go | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/driver/docker/docker.go b/driver/docker/docker.go index 2a46c226..1ddde061 100644 --- a/driver/docker/docker.go +++ b/driver/docker/docker.go @@ -200,7 +200,7 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) { } d.containerHostCfg = &container.HostConfig{} - if err := d.applyConfigurationOptions(); err != nil { + if err := d.ApplyConfigurationOptions(); err != nil { return driver.OperationResult{}, err } @@ -293,7 +293,8 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) { return opResult, err } -func (d *Driver) applyConfigurationOptions() error { +// ApplyConfigurationOptions applies the configuration options set on the driver +func (d *Driver) ApplyConfigurationOptions() error { for _, opt := range d.dockerConfigurationOptions { if err := opt(d.containerCfg, d.containerHostCfg); err != nil { return err diff --git a/driver/docker/docker_test.go b/driver/docker/docker_test.go index 5e453c39..4a6ce62d 100644 --- a/driver/docker/docker_test.go +++ b/driver/docker/docker_test.go @@ -18,7 +18,7 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { d.containerCfg = &container.Config{} d.containerHostCfg = &container.HostConfig{} - err := d.applyConfigurationOptions() + err := d.ApplyConfigurationOptions() is.NoError(err) cfg, err := d.GetContainerConfig() @@ -39,7 +39,7 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { return nil }) - err := d.applyConfigurationOptions() + err := d.ApplyConfigurationOptions() is.NoError(err) expectedContainerCfg := container.Config{ From c46b74fc4e0cf61fef72a0e3bbb8bf420b45546a Mon Sep 17 00:00:00 2001 From: Vaughn Dice Date: Wed, 22 Apr 2020 15:16:11 -0600 Subject: [PATCH 2/4] ref(docker.go): container config fields on Driver no longer pointers Signed-off-by: Vaughn Dice --- driver/docker/docker.go | 44 +++++++++--------------------------- driver/docker/docker_test.go | 19 ++++------------ 2 files changed, 16 insertions(+), 47 deletions(-) diff --git a/driver/docker/docker.go b/driver/docker/docker.go index 1ddde061..d6df47cf 100644 --- a/driver/docker/docker.go +++ b/driver/docker/docker.go @@ -3,7 +3,6 @@ package docker import ( "archive/tar" "context" - "errors" "fmt" "io" "io/ioutil" @@ -21,7 +20,6 @@ import ( "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/pkg/stdcopy" "github.com/docker/docker/registry" - copystructure "github.com/mitchellh/copystructure" ) // Driver is capable of running Docker invocation images using Docker itself. @@ -33,8 +31,8 @@ type Driver struct { dockerConfigurationOptions []ConfigurationOption containerOut io.Writer containerErr io.Writer - containerHostCfg *container.HostConfig - containerCfg *container.Config + containerHostCfg container.HostConfig + containerCfg container.Config } // Run executes the Docker driver @@ -54,34 +52,14 @@ func (d *Driver) AddConfigurationOptions(opts ...ConfigurationOption) { // GetContainerConfig returns a copy of the container configuration // used by the driver during container exec -func (d *Driver) GetContainerConfig() (container.Config, error) { - cpy, err := copystructure.Copy(*d.containerCfg) - if err != nil { - return container.Config{}, err - } - - containerCfg, ok := cpy.(container.Config) - if !ok { - return container.Config{}, errors.New("unable to process container config") - } - - return containerCfg, nil +func (d *Driver) GetContainerConfig() container.Config { + return d.containerCfg } // GetContainerHostConfig returns a copy of the container host configuration // used by the driver during container exec -func (d *Driver) GetContainerHostConfig() (container.HostConfig, error) { - cpy, err := copystructure.Copy(*d.containerHostCfg) - if err != nil { - return container.HostConfig{}, err - } - - hostCfg, ok := cpy.(container.HostConfig) - if !ok { - return container.HostConfig{}, errors.New("unable to process container host config") - } - - return hostCfg, nil +func (d *Driver) GetContainerHostConfig() container.HostConfig { + return d.containerHostCfg } // Config returns the Docker driver configuration options @@ -191,7 +169,7 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) { env = append(env, fmt.Sprintf("%s=%v", k, v)) } - d.containerCfg = &container.Config{ + d.containerCfg = container.Config{ Image: op.Image.Image, Env: env, Entrypoint: strslice.StrSlice{"/cnab/app/run"}, @@ -199,19 +177,19 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) { AttachStdout: true, } - d.containerHostCfg = &container.HostConfig{} + d.containerHostCfg = container.HostConfig{} if err := d.ApplyConfigurationOptions(); err != nil { return driver.OperationResult{}, err } - resp, err := cli.Client().ContainerCreate(ctx, d.containerCfg, d.containerHostCfg, nil, "") + resp, err := cli.Client().ContainerCreate(ctx, &d.containerCfg, &d.containerHostCfg, nil, "") switch { case client.IsErrNotFound(err): fmt.Fprintf(cli.Err(), "Unable to find image '%s' locally\n", op.Image.Image) if err := pullImage(ctx, cli, op.Image.Image); err != nil { return driver.OperationResult{}, err } - if resp, err = cli.Client().ContainerCreate(ctx, d.containerCfg, d.containerHostCfg, nil, ""); err != nil { + if resp, err = cli.Client().ContainerCreate(ctx, &d.containerCfg, &d.containerHostCfg, nil, ""); err != nil { return driver.OperationResult{}, fmt.Errorf("cannot create container: %v", err) } case err != nil: @@ -296,7 +274,7 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) { // ApplyConfigurationOptions applies the configuration options set on the driver func (d *Driver) ApplyConfigurationOptions() error { for _, opt := range d.dockerConfigurationOptions { - if err := opt(d.containerCfg, d.containerHostCfg); err != nil { + if err := opt(&d.containerCfg, &d.containerHostCfg); err != nil { return err } } diff --git a/driver/docker/docker_test.go b/driver/docker/docker_test.go index 4a6ce62d..91bf7644 100644 --- a/driver/docker/docker_test.go +++ b/driver/docker/docker_test.go @@ -14,25 +14,18 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { is.NotNil(d) is.True(d.Handles(driver.ImageTypeDocker)) - t.Run("no configuration options", func(t *testing.T) { - d.containerCfg = &container.Config{} - d.containerHostCfg = &container.HostConfig{} - + t.Run("empty configuration options", func(t *testing.T) { err := d.ApplyConfigurationOptions() is.NoError(err) - cfg, err := d.GetContainerConfig() - is.NoError(err) + cfg := d.GetContainerConfig() is.Equal(container.Config{}, cfg) - hostCfg, err := d.GetContainerHostConfig() - is.NoError(err) + hostCfg := d.GetContainerHostConfig() is.Equal(container.HostConfig{}, hostCfg) }) t.Run("configuration options", func(t *testing.T) { - d.containerCfg = &container.Config{} - d.containerHostCfg = &container.HostConfig{} d.AddConfigurationOptions(func(cfg *container.Config, hostCfg *container.HostConfig) error { cfg.User = "cnabby" hostCfg.Privileged = true @@ -49,12 +42,10 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { Privileged: true, } - cfg, err := d.GetContainerConfig() - is.NoError(err) + cfg := d.GetContainerConfig() is.Equal(expectedContainerCfg, cfg) - hostCfg, err := d.GetContainerHostConfig() - is.NoError(err) + hostCfg := d.GetContainerHostConfig() is.Equal(expectedHostCfg, hostCfg) }) } From 9b6875f2cf938105a9c49fd60b66919f76fe6d03 Mon Sep 17 00:00:00 2001 From: Vaughn Dice Date: Fri, 24 Apr 2020 11:50:44 -0600 Subject: [PATCH 3/4] keep deep copy logic for container config accessors Signed-off-by: Vaughn Dice --- driver/docker/docker.go | 30 ++++++++++++++++++++++++++---- driver/docker/docker_test.go | 12 ++++++++---- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/driver/docker/docker.go b/driver/docker/docker.go index d6df47cf..61f981d9 100644 --- a/driver/docker/docker.go +++ b/driver/docker/docker.go @@ -3,6 +3,7 @@ package docker import ( "archive/tar" "context" + "errors" "fmt" "io" "io/ioutil" @@ -20,6 +21,7 @@ import ( "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/pkg/stdcopy" "github.com/docker/docker/registry" + "github.com/mitchellh/copystructure" ) // Driver is capable of running Docker invocation images using Docker itself. @@ -52,14 +54,34 @@ func (d *Driver) AddConfigurationOptions(opts ...ConfigurationOption) { // GetContainerConfig returns a copy of the container configuration // used by the driver during container exec -func (d *Driver) GetContainerConfig() container.Config { - return d.containerCfg +func (d *Driver) GetContainerConfig() (container.Config, error) { + cpy, err := copystructure.Copy(d.containerCfg) + if err != nil { + return container.Config{}, err + } + + cfg, ok := cpy.(container.Config) + if !ok { + return container.Config{}, errors.New("unable to process container config") + } + + return cfg, nil } // GetContainerHostConfig returns a copy of the container host configuration // used by the driver during container exec -func (d *Driver) GetContainerHostConfig() container.HostConfig { - return d.containerHostCfg +func (d *Driver) GetContainerHostConfig() (container.HostConfig, error) { + cpy, err := copystructure.Copy(d.containerHostCfg) + if err != nil { + return container.HostConfig{}, err + } + + cfg, ok := cpy.(container.HostConfig) + if !ok { + return container.HostConfig{}, errors.New("unable to process container host config") + } + + return cfg, nil } // Config returns the Docker driver configuration options diff --git a/driver/docker/docker_test.go b/driver/docker/docker_test.go index 91bf7644..c39eef82 100644 --- a/driver/docker/docker_test.go +++ b/driver/docker/docker_test.go @@ -18,10 +18,12 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { err := d.ApplyConfigurationOptions() is.NoError(err) - cfg := d.GetContainerConfig() + cfg, err := d.GetContainerConfig() + is.NoError(err) is.Equal(container.Config{}, cfg) - hostCfg := d.GetContainerHostConfig() + hostCfg, err := d.GetContainerHostConfig() + is.NoError(err) is.Equal(container.HostConfig{}, hostCfg) }) @@ -42,10 +44,12 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { Privileged: true, } - cfg := d.GetContainerConfig() + cfg, err := d.GetContainerConfig() + is.NoError(err) is.Equal(expectedContainerCfg, cfg) - hostCfg := d.GetContainerHostConfig() + hostCfg, err := d.GetContainerHostConfig() + is.NoError(err) is.Equal(expectedHostCfg, hostCfg) }) } From 5395191c27b9b2ccda76a6f4e6327e32215796ff Mon Sep 17 00:00:00 2001 From: Vaughn Dice Date: Fri, 24 Apr 2020 12:08:50 -0600 Subject: [PATCH 4/4] add config modification test Signed-off-by: Vaughn Dice --- driver/docker/docker_test.go | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/driver/docker/docker_test.go b/driver/docker/docker_test.go index c39eef82..5ef9a3ad 100644 --- a/driver/docker/docker_test.go +++ b/driver/docker/docker_test.go @@ -5,16 +5,18 @@ import ( "github.com/cnabio/cnab-go/driver" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/strslice" "github.com/stretchr/testify/assert" ) func TestDriver_GetConfigurationOptions(t *testing.T) { - d := &Driver{} is := assert.New(t) - is.NotNil(d) - is.True(d.Handles(driver.ImageTypeDocker)) t.Run("empty configuration options", func(t *testing.T) { + d := &Driver{} + is.NotNil(d) + is.True(d.Handles(driver.ImageTypeDocker)) + err := d.ApplyConfigurationOptions() is.NoError(err) @@ -28,6 +30,8 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { }) t.Run("configuration options", func(t *testing.T) { + d := &Driver{} + d.AddConfigurationOptions(func(cfg *container.Config, hostCfg *container.HostConfig) error { cfg.User = "cnabby" hostCfg.Privileged = true @@ -52,4 +56,30 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { is.NoError(err) is.Equal(expectedHostCfg, hostCfg) }) + + t.Run("configuration options - no unintentional modification", func(t *testing.T) { + d := &Driver{} + + d.AddConfigurationOptions(func(cfg *container.Config, hostCfg *container.HostConfig) error { + hostCfg.CapAdd = strslice.StrSlice{"SUPER_POWERS"} + return nil + }) + + err := d.ApplyConfigurationOptions() + is.NoError(err) + + expectedHostCfg := container.HostConfig{ + CapAdd: strslice.StrSlice{"SUPER_POWERS"}, + } + + hostCfg, err := d.GetContainerHostConfig() + is.NoError(err) + is.Equal(expectedHostCfg, hostCfg) + + hostCfg.CapAdd[0] = "NORMAL_POWERS" + + hostCfg, err = d.GetContainerHostConfig() + is.NoError(err) + is.Equal(expectedHostCfg, hostCfg) + }) }