From 4f957266205094f44d81e00f243431f08e004e82 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 20 Jan 2021 13:23:01 -0600 Subject: [PATCH] Do not panic when setting driver config Use the go, Luke. Return errors instead of throwing a panic. Signed-off-by: Carolyn Van Slyck --- driver/debug/debug.go | 3 +- driver/docker/docker.go | 6 +-- driver/docker/docker_test.go | 50 +++++++++++++++++++ driver/driver.go | 2 +- driver/kubernetes/kubernetes.go | 13 ++--- .../kubernetes/kubernetes_integration_test.go | 15 +++++- driver/kubernetes/kubernetes_test.go | 13 +++++ 7 files changed, 88 insertions(+), 14 deletions(-) diff --git a/driver/debug/debug.go b/driver/debug/debug.go index 2baef63f..85a3c54b 100644 --- a/driver/debug/debug.go +++ b/driver/debug/debug.go @@ -42,6 +42,7 @@ func (d *Driver) Config() map[string]string { } // SetConfig sets configuration for this driver -func (d *Driver) SetConfig(settings map[string]string) { +func (d *Driver) SetConfig(settings map[string]string) error { d.config = settings + return nil } diff --git a/driver/docker/docker.go b/driver/docker/docker.go index b081d5f7..3c2024ad 100644 --- a/driver/docker/docker.go +++ b/driver/docker/docker.go @@ -19,7 +19,6 @@ import ( "github.com/docker/docker/pkg/stdcopy" "github.com/docker/docker/registry" "github.com/mitchellh/copystructure" - "github.com/pkg/errors" "github.com/cnabio/cnab-go/bundle" @@ -98,16 +97,17 @@ func (d *Driver) Config() map[string]string { } // SetConfig sets Docker driver configuration -func (d *Driver) SetConfig(settings map[string]string) { +func (d *Driver) SetConfig(settings map[string]string) error { // Set default and provide feedback on acceptable input values. value, ok := settings["CLEANUP_CONTAINERS"] if !ok { settings["CLEANUP_CONTAINERS"] = "true" } else if value != "true" && value != "false" { - fmt.Printf("CLEANUP_CONTAINERS environment variable has unexpected value %q. Supported values are 'true', 'false', or unset.", value) + return fmt.Errorf("environment variable CLEANUP_CONTAINERS has unexpected value %q. Supported values are 'true', 'false', or unset", value) } d.config = settings + return nil } // SetDockerCli makes the driver use an already initialized cli diff --git a/driver/docker/docker_test.go b/driver/docker/docker_test.go index 50719f0c..730b7bae 100644 --- a/driver/docker/docker_test.go +++ b/driver/docker/docker_test.go @@ -87,6 +87,56 @@ func TestDriver_GetConfigurationOptions(t *testing.T) { }) } +func TestDriver_SetConfig(t *testing.T) { + testcases := []struct { + name string + settings map[string]string + wantError string + }{ + { + name: "valid settings", + settings: map[string]string{ + "VERBOSE": "true", + }, + wantError: "", + }, + { + name: "cleanup containers: true", + settings: map[string]string{ + "CLEANUP_CONTAINERS": "true", + }, + wantError: "", + }, + { + name: "cleanup containers: false", + settings: map[string]string{ + "CLEANUP_CONTAINERS": "false", + }, + wantError: "", + }, + { + name: "cleanup containers - invalid", + settings: map[string]string{ + "CLEANUP_CONTAINERS": "1", + }, + wantError: "environment variable CLEANUP_CONTAINERS has unexpected value", + }, + } + + for _, tc := range testcases { + d := Driver{} + err := d.SetConfig(tc.settings) + + if tc.wantError == "" { + require.NoError(t, err, "expected SetConfig to succeed") + assert.Equal(t, tc.settings, d.config, "expected all of the specified settings to be copied") + } else { + require.Error(t, err, "expected SetConfig to fail") + assert.Contains(t, err.Error(), tc.wantError) + } + } +} + func TestDriver_ValidateImageDigest(t *testing.T) { // Mimic the digests created when a bundle is pushed with cnab-to-oci // there is one for the original invocation image and another diff --git a/driver/driver.go b/driver/driver.go index 2cab0501..2daad0a1 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -103,5 +103,5 @@ type Configurable interface { Config() map[string]string // SetConfig allows setting configuration, where name corresponds to the key in Config, and value is // the value to be set. - SetConfig(map[string]string) + SetConfig(map[string]string) error } diff --git a/driver/kubernetes/kubernetes.go b/driver/kubernetes/kubernetes.go index ba8ff126..b7797d4f 100644 --- a/driver/kubernetes/kubernetes.go +++ b/driver/kubernetes/kubernetes.go @@ -87,7 +87,7 @@ func (k *Driver) Config() map[string]string { } // SetConfig sets Kubernetes driver configuration. -func (k *Driver) SetConfig(settings map[string]string) { +func (k *Driver) SetConfig(settings map[string]string) error { k.setDefaults() k.Namespace = settings["KUBE_NAMESPACE"] k.ServiceAccountName = settings["SERVICE_ACCOUNT"] @@ -101,12 +101,9 @@ func (k *Driver) SetConfig(settings map[string]string) { conf, err := clientcmd.BuildConfigFromFlags(settings["MASTER_URL"], kubeconfig) if err != nil { - panic(err) - } - err = k.setClient(conf) - if err != nil { - panic(err) + return errors.Wrapf(err, "error retrieving external kubernetes configuration using configuration:\n%v", settings) } + return k.setClient(conf) } func (k *Driver) setDefaults() { @@ -120,11 +117,11 @@ func (k *Driver) setDefaults() { func (k *Driver) setClient(conf *rest.Config) error { coreClient, err := coreclientv1.NewForConfig(conf) if err != nil { - return err + return errors.Wrap(err, "error creating CoreClient for Kubernetes Driver") } batchClient, err := batchclientv1.NewForConfig(conf) if err != nil { - return err + return errors.Wrap(err, "error creating BatchClient for Kubernetes Driver") } k.jobs = batchClient.Jobs(k.Namespace) k.secrets = coreClient.Secrets(k.Namespace) diff --git a/driver/kubernetes/kubernetes_integration_test.go b/driver/kubernetes/kubernetes_integration_test.go index a20a4203..80b4fd88 100644 --- a/driver/kubernetes/kubernetes_integration_test.go +++ b/driver/kubernetes/kubernetes_integration_test.go @@ -7,9 +7,11 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/cnabio/cnab-go/bundle" "github.com/cnabio/cnab-go/driver" - "github.com/stretchr/testify/assert" ) func TestDriver_Run_Integration(t *testing.T) { @@ -85,3 +87,14 @@ func TestDriver_Run_Integration(t *testing.T) { }) } } + +func TestDriver_SetConfig(t *testing.T) { + t.Run("kubeconfig", func(t *testing.T) { + + d := Driver{} + err := d.SetConfig(map[string]string{ + "KUBECONFIG": os.Getenv("KUBECONFIG"), + }) + require.NoError(t, err) + }) +} diff --git a/driver/kubernetes/kubernetes_test.go b/driver/kubernetes/kubernetes_test.go index 2fd9cd4f..310a9d57 100644 --- a/driver/kubernetes/kubernetes_test.go +++ b/driver/kubernetes/kubernetes_test.go @@ -162,3 +162,16 @@ func TestGenerateNameTemplate(t *testing.T) { }) } } + +func TestDriver_SetConfig_Fails(t *testing.T) { + t.Run("kubeconfig invalid", func(t *testing.T) { + + d := Driver{} + err := d.SetConfig(map[string]string{ + "KUBECONFIG": "invalid", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "error retrieving external kubernetes configuration using configuration") + }) + +}