Remove the need to validate DriverType (#1374)

* Remove the need to validate DriverType

* Return nil on error for storageForDriver fn

cmd/kubeops/internal/handler/handler.go

@@ -23,12 +23,12 @@ type dependentHandler func(cfg agent.Config, w http.ResponseWriter, req *http.Re
 // WithAgentConfig takes a dependentHandler and creates a regular (WithParams) handler that,
 // for every request, will create an agent config for itself.
 // Written in a curried fashion for convenient usage; see cmd/kubeops/main.go.
-func WithAgentConfig(driverType agent.DriverType, options agent.Options) func(f dependentHandler) handlerutil.WithParams {
+func WithAgentConfig(storageForDriver agent.StorageForDriver, options agent.Options) func(f dependentHandler) handlerutil.WithParams {
 	return func(f dependentHandler) handlerutil.WithParams {
 		return func(w http.ResponseWriter, req *http.Request, params handlerutil.Params) {
 			namespace := params[namespaceParam]
 			token := auth.ExtractToken(req.Header.Get(authHeader))
-			actionConfig, err := agent.NewActionConfig(driverType, token, namespace)
+			actionConfig, err := agent.NewActionConfig(storageForDriver, token, namespace)
 			if err != nil {
 				// TODO log details rather than return potentially sensitive details in error.
 				http.Error(w, err.Error(), http.StatusInternalServerError)

cmd/kubeops/main.go

@@ -20,8 +20,6 @@ import (
 	"k8s.io/helm/pkg/helm/environment"
 )
 
-const defaultHelmDriver agent.DriverType = agent.Secret
-
 var (
 	settings         environment.EnvSettings
 	chartsvcURL      string
@@ -50,15 +48,15 @@ func main() {
 		Timeout:   timeout,
 	}
 
-	driverType := defaultHelmDriver
+	storageForDriver := agent.StorageForSecrets
 	if helmDriverArg != "" {
-		d, err := agent.ParseDriverType(helmDriverArg)
+		var err error
+		storageForDriver, err = agent.ParseDriverType(helmDriverArg)
 		if err != nil {
 			panic(err)
 		}
-		driverType = d // Necessary detour to please typechecker.
 	}
-	withAgentConfig := handler.WithAgentConfig(driverType, options)
+	withAgentConfig := handler.WithAgentConfig(storageForDriver, options)
 	r := mux.NewRouter()
 
 	// Routes

pkg/agent/agent.go

@@ -2,7 +2,6 @@ package agent
 
 import (
 	"errors"
-	"fmt"
 	"strconv"
 
 	"github.com/kubeapps/kubeapps/pkg/proxy"
@@ -16,13 +15,28 @@ import (
 	"k8s.io/klog"
 )
 
-type DriverType string
+// StorageForDriver is a function type which returns a specific storage.
+type StorageForDriver func(namespace string, clientset *kubernetes.Clientset) *storage.Storage
 
-const (
-	Secret    DriverType = "SECRET"
-	ConfigMap DriverType = "CONFIGMAP"
-	Memory    DriverType = "MEMORY"
-)
+// StorageForSecrets returns a storage using the Secret driver.
+func StorageForSecrets(namespace string, clientset *kubernetes.Clientset) *storage.Storage {
+	d := driver.NewSecrets(clientset.CoreV1().Secrets(namespace))
+	d.Log = klog.Infof
+	return storage.Init(d)
+}
+
+// StorageForConfigMaps returns a storage using the ConfigMap driver.
+func StorageForConfigMaps(namespace string, clientset *kubernetes.Clientset) *storage.Storage {
+	d := driver.NewConfigMaps(clientset.CoreV1().ConfigMaps(namespace))
+	d.Log = klog.Infof
+	return storage.Init(d)
+}
+
+// StorageForMemory returns a storage using the Memory driver.
+func StorageForMemory(_ string, _ *kubernetes.Clientset) *storage.Storage {
+	d := driver.NewMemory()
+	return storage.Init(d)
+}
 
 type Options struct {
 	ListLimit int
@@ -54,7 +68,7 @@ func ListReleases(actionConfig *action.Configuration, namespace string, listLimi
 	return appOverviews, nil
 }
 
-func NewActionConfig(driver DriverType, token, namespace string) (*action.Configuration, error) {
+func NewActionConfig(storageForDriver StorageForDriver, token, namespace string) (*action.Configuration, error) {
 	actionConfig := new(action.Configuration)
 	config, err := rest.InClusterConfig()
 	if err != nil {
@@ -63,47 +77,24 @@ func NewActionConfig(driver DriverType, token, namespace string) (*action.Config
 	config.BearerToken = token
 	config.BearerTokenFile = ""
 	clientset, err := kubernetes.NewForConfig(config)
-	store, err := createStorage(driver, namespace, clientset)
-	if err != nil {
-		return nil, err
-	}
+	store := storageForDriver(namespace, clientset)
 	actionConfig.RESTClientGetter = nil     // TODO replace nil with meaningful value
 	actionConfig.KubeClient = kube.New(nil) // TODO replace nil with meaningful value
 	actionConfig.Releases = store
 	actionConfig.Log = klog.Infof
 	return actionConfig, nil
 }
 
-func createStorage(driverType DriverType, namespace string, clientset *kubernetes.Clientset) (*storage.Storage, error) {
-	var store *storage.Storage
-	switch driverType {
-	case Secret:
-		d := driver.NewSecrets(clientset.CoreV1().Secrets(namespace))
-		d.Log = klog.Infof
-		store = storage.Init(d)
-	case ConfigMap:
-		d := driver.NewConfigMaps(clientset.CoreV1().ConfigMaps(namespace))
-		d.Log = klog.Infof
-		store = storage.Init(d)
-	case Memory:
-		d := driver.NewMemory()
-		store = storage.Init(d)
-	default:
-		return nil, fmt.Errorf("Invalid Helm drive type: %q", driverType)
-	}
-	return store, nil
-}
-
-func ParseDriverType(raw string) (DriverType, error) {
+func ParseDriverType(raw string) (StorageForDriver, error) {
 	switch raw {
 	case "secret", "secrets":
-		return Secret, nil
+		return StorageForSecrets, nil
 	case "configmap", "configmaps":
-		return ConfigMap, nil
+		return StorageForConfigMaps, nil
 	case "memory":
-		return Memory, nil
+		return StorageForMemory, nil
 	default:
-		return Memory, errors.New("Invalid Helm driver type: " + raw)
+		return nil, errors.New("Invalid Helm driver type: " + raw)
 	}
 }
 

pkg/agent/agent_test.go

@@ -12,6 +12,7 @@ import (
 	"helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/storage"
 	"helm.sh/helm/v3/pkg/storage/driver"
+	"k8s.io/client-go/kubernetes"
 
 	"github.com/kubeapps/kubeapps/pkg/proxy"
 )
@@ -208,47 +209,52 @@ func TestListReleases(t *testing.T) {
 
 func TestParseDriverType(t *testing.T) {
 	validTestCases := []struct {
-		input  string
-		output DriverType
+		input      string
+		driverName string
 	}{
 		{
-			input:  "secret",
-			output: Secret,
+			input:      "secret",
+			driverName: "Secret",
 		},
 		{
-			input:  "secrets",
-			output: Secret,
+			input:      "secrets",
+			driverName: "Secret",
 		},
 		{
-			input:  "configmap",
-			output: ConfigMap,
+			input:      "configmap",
+			driverName: "ConfigMap",
 		},
 		{
-			input:  "configmaps",
-			output: ConfigMap,
+			input:      "configmaps",
+			driverName: "ConfigMap",
 		},
 		{
-			input:  "memory",
-			output: Memory,
+			input:      "memory",
+			driverName: "Memory",
 		},
 	}
 
 	for _, tc := range validTestCases {
 		t.Run(tc.input, func(t *testing.T) {
-			driverType, err := ParseDriverType(tc.input)
+			storageForDriver, err := ParseDriverType(tc.input)
 			if err != nil {
-				t.Errorf("%v", err)
-			} else if driverType != tc.output {
-				t.Errorf("expected: %s, actual: %s", tc.output, driverType)
+				t.Fatalf("%v", err)
+			}
+			storage := storageForDriver("default", &kubernetes.Clientset{})
+			if got, want := storage.Name(), tc.driverName; got != want {
+				t.Errorf("expected: %s, actual: %s", want, got)
 			}
 		})
 	}
 
 	invalidTestCase := "andresmgot"
 	t.Run(invalidTestCase, func(t *testing.T) {
-		driverType, err := ParseDriverType(invalidTestCase)
+		storageForDriver, err := ParseDriverType(invalidTestCase)
 		if err == nil {
-			t.Errorf("Expected \"%s\" to be an invalid driver type, but it was parsed as %v", invalidTestCase, driverType)
+			t.Errorf("Expected \"%s\" to be an invalid driver type, but it was parsed as %v", invalidTestCase, storageForDriver)
+		}
+		if storageForDriver != nil {
+			t.Errorf("got: %#v, want: nil", storageForDriver)
 		}
 	})
 }