Abstract client rest.Config behind an interface

Too bad discovery.NewDiscoveryClientForConfigOrDie() and
dynamic.NewDynamicClientPool() expects a concrete *rest.Config
struct rather than a kubernetes.Interface (clientset).

We can still wrap this rest.Config behind an interface. That's
a first step before we can, progressively, get rid of Config
(and run.go), which would make pkg/ content more like reusable
packages rather than a frameworkish bunch of files.

Author: bpineau

cmd/execute.go

@@ -8,26 +8,37 @@ import (
 	"github.com/spf13/viper"
 
 	"github.com/bpineau/katafygio/config"
+	"github.com/bpineau/katafygio/pkg/client"
 	"github.com/bpineau/katafygio/pkg/log"
 	"github.com/bpineau/katafygio/pkg/run"
 )
 
 const appName = "katafygio"
 
 var (
+	restcfg client.Interface
+
 	// RootCmd is our main entry point, launching pkg/run.Run()
 	RootCmd = &cobra.Command{
 		Use:   appName,
 		Short: "Backup Kubernetes cluster as yaml files",
 		Long: "Backup Kubernetes cluster as yaml files in a git repository.\n" +
 			"--exclude-kind (-x) and --exclude-object (-y) may be specified several times.",
 
-		RunE: func(cmd *cobra.Command, args []string) error {
+		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			resync := time.Duration(viper.GetInt("resync-interval")) * time.Second
 			logger := log.New(viper.GetString("log.level"),
 				viper.GetString("log.server"),
 				viper.GetString("log.output"))
 
+			if restcfg == nil {
+				restcfg, err = client.New(viper.GetString("api-server"),
+					viper.GetString("kube-config"))
+				if err != nil {
+					return fmt.Errorf("failed to create a client: %v", err)
+				}
+			}
+
 			conf := &config.KfConfig{
 				DryRun:        viper.GetBool("dry-run"),
 				DumpMode:      viper.GetBool("dump-only"),
@@ -38,14 +49,10 @@ var (
 				ExcludeKind:   viper.GetStringSlice("exclude-kind"),
 				ExcludeObject: viper.GetStringSlice("exclude-object"),
 				HealthPort:    viper.GetInt("healthcheck-port"),
+				Client:        restcfg,
 				ResyncIntv:    resync,
 			}
 
-			err := conf.Init(viper.GetString("api-server"), viper.GetString("kube-config"))
-			if err != nil {
-				return fmt.Errorf("Failed to initialize the configuration: %v", err)
-			}
-
 			run.Run(conf) // <- this is where things happen
 			return nil
 		},

cmd/execute_test.go

@@ -0,0 +1,51 @@
+package cmd
+
+import (
+	"bytes"
+	"testing"
+
+	"k8s.io/client-go/rest"
+)
+
+type mockClient struct{}
+
+func (m *mockClient) GetRestConfig() *rest.Config {
+	return &rest.Config{}
+}
+
+func TestRootCmd(t *testing.T) {
+	restcfg = new(mockClient)
+	RootCmd.SetOutput(new(bytes.Buffer))
+	RootCmd.SetArgs([]string{
+		"--config",
+		"/dev/null",
+		"--kube-config",
+		"/dev/null",
+		"--dry-run",
+		"--dump-only",
+		"--api-server",
+		"http://192.0.2.1", // RFC 5737 reserved/unroutable
+		"--log-level",
+		"warning",
+		"--log-output",
+		"test",
+		"--healthcheck-port",
+		"0",
+		"--filter",
+		"foo=bar,spam=egg",
+		"--resync-interval",
+		"1",
+	})
+
+	if err := Execute(); err != nil {
+		t.Errorf("version subcommand shouldn't fail: %+v", err)
+	}
+}
+
+func TestVersionCmd(t *testing.T) {
+	RootCmd.SetOutput(new(bytes.Buffer))
+	RootCmd.SetArgs([]string{"version"})
+	if err := RootCmd.Execute(); err != nil {
+		t.Errorf("version subcommand shouldn't fail: %+v", err)
+	}
+}

config/config.go

@@ -7,7 +7,6 @@ import (
 	"github.com/bpineau/katafygio/pkg/client"
 
 	"github.com/sirupsen/logrus"
-	"k8s.io/client-go/rest"
 )
 
 // KfConfig holds the configuration options passed at launch time (and the rest client)
@@ -22,7 +21,7 @@ type KfConfig struct {
 	Logger *logrus.Logger
 
 	// Client represents a connection to a Kubernetes cluster
-	Client *rest.Config
+	Client client.Interface
 
 	// GitURL is the address of a remote git repository
 	GitURL string
@@ -48,9 +47,9 @@ type KfConfig struct {
 
 // Init initialize the config
 func (c *KfConfig) Init(apiserver string, kubeconfig string) (err error) {
-	c.Client, err = client.NewRestConfig(apiserver, kubeconfig)
+	c.Client, err = client.New(apiserver, kubeconfig)
 	if err != nil {
-		return fmt.Errorf("Failed init Kubernetes clientset: %+v", err)
+		return fmt.Errorf("Failed init Kubernetes client: %+v", err)
 	}
 	return nil
 }

pkg/client/client.go

@@ -1,11 +1,11 @@
-// Package client initialize a Kubernete's client-go rest.Config or clientset
+// Package client initialize and wrap a Kubernete's client-go rest.Config client
 package client
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 
-	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	"k8s.io/client-go/util/homedir"
@@ -14,13 +14,40 @@ import (
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
 )
 
-// NewRestConfig create a *rest.Config, trying to mimic kubectl behavior:
+// Interface abstracts access to a concrete Kubernetes rest.Client
+type Interface interface {
+	GetRestConfig() *rest.Config
+}
+
+// RestClient holds a Kubernetes rest client configuration
+type RestClient struct {
+	cfg *rest.Config
+}
+
+// New create a new RestClient
+func New(apiserver, kubeconfig string) (*RestClient, error) {
+	cfg, err := newRestConfig(apiserver, kubeconfig)
+	if err != nil {
+		return nil, fmt.Errorf("failed to build a restconfig: %v", err)
+	}
+
+	return &RestClient{
+		cfg: cfg,
+	}, nil
+}
+
+// GetRestConfig returns the current rest.Config
+func (r *RestClient) GetRestConfig() *rest.Config {
+	return r.cfg
+}
+
+// newRestConfig create a *rest.Config, trying to mimic kubectl behavior:
 // - Explicit user provided api-server (and/or kubeconfig path) have higher priorities
 // - Else, use the config file path in KUBECONFIG environment variable (if any)
 // - Else, use the config file in ~/.kube/config, if any
 // - Else, consider we're running in cluster (in a pod), and use the pod's service
 //   account and cluster's kubernetes.default service.
-func NewRestConfig(apiserver string, kubeconfig string) (*rest.Config, error) {
+func newRestConfig(apiserver string, kubeconfig string) (*rest.Config, error) {
 	// if not passed as an argument, kubeconfig can be provided as env var
 	if kubeconfig == "" {
 		kubeconfig = os.Getenv("KUBECONFIG")
@@ -43,13 +70,3 @@ func NewRestConfig(apiserver string, kubeconfig string) (*rest.Config, error) {
 	// else assume we're running in a pod, in cluster
 	return rest.InClusterConfig()
 }
-
-// NewClientSet create a clientset (a client connection to a Kubernetes cluster).
-func NewClientSet(apiserver string, kubeconfig string) (*kubernetes.Clientset, error) {
-	config, err := NewRestConfig(apiserver, kubeconfig)
-	if err != nil {
-		return nil, err
-	}
-
-	return kubernetes.NewForConfig(config)
-}

pkg/client/client_test.go

@@ -6,33 +6,33 @@ import (
 	"testing"
 )
 
-const nonExistentPath = "\\/hopefully/non/existent/path"
+const nonExistentPath = "\\/non / existent / $path$"
 
 func TestClientSet(t *testing.T) {
 	here, _ := os.Getwd()
 	_ = os.Setenv("HOME", here+"/../../assets")
-	cs, err := NewClientSet("", "")
+	cs, err := New("", "")
 	if err != nil {
 		t.Fatal(err)
 	}
-	if fmt.Sprintf("%T", cs) != "*kubernetes.Clientset" {
-		t.Errorf("NewClientSet() didn't return a *kubernetes.Clientset: %T", cs)
+	if fmt.Sprintf("%T", cs.GetRestConfig()) != "*rest.Config" {
+		t.Errorf("GetRestConfig() didn't return a *rest.Config: %T", cs)
 	}
 
-	cs, _ = NewClientSet("http://127.0.0.1", "/dev/null")
-	if fmt.Sprintf("%T", cs) != "*kubernetes.Clientset" {
-		t.Errorf("NewClientSet(server) didn't return a *kubernetes.Clientset: %T", cs)
+	cs, _ = New("http://127.0.0.1", "/dev/null")
+	if fmt.Sprintf("%T", cs.GetRestConfig()) != "*rest.Config" {
+		t.Errorf("New(server) didn't return a *rest.Config: %T", cs)
 	}
 
-	_, err = NewClientSet("http://127.0.0.1", nonExistentPath)
+	_, err = New("http://127.0.0.1", nonExistentPath)
 	if err == nil {
-		t.Fatal("NewClientSet() should fail on non existent kubeconfig path")
+		t.Fatal("New() should fail on non existent kubeconfig path")
 	}
 
 	_ = os.Unsetenv("KUBERNETES_SERVICE_HOST")
 	_ = os.Setenv("HOME", nonExistentPath)
-	_, err = NewClientSet("", "")
+	_, err = New("", "")
 	if err == nil {
-		t.Fatal("NewClientSet() should fail to load InClusterConfig without kube address env")
+		t.Fatal("New() should fail to load InClusterConfig without kube address env")
 	}
 }

pkg/observer/observer.go

@@ -56,8 +56,8 @@ func New(config *config.KfConfig, notif event.Notifier, factory ControllerFactor
 	return &Observer{
 		config:    config,
 		notifier:  notif,
-		discovery: discovery.NewDiscoveryClientForConfigOrDie(config.Client),
-		cpool:     dynamic.NewDynamicClientPool(config.Client),
+		discovery: discovery.NewDiscoveryClientForConfigOrDie(config.Client.GetRestConfig()),
+		cpool:     dynamic.NewDynamicClientPool(config.Client.GetRestConfig()),
 		ctrls:     make(controllerCollection),
 		factory:   factory,
 	}

pkg/observer/observer_test.go

@@ -46,6 +46,12 @@ func (m *mockFactory) NewController(client cache.ListerWatcher, notifier event.N
 	return &mockCtrl{}
 }
 
+type mockClient struct{}
+
+func (m *mockClient) GetRestConfig() *restclient.Config {
+	return &restclient.Config{}
+}
+
 var stdVerbs = []string{"list", "get", "watch"}
 var emptyExclude = make([]string, 0)
 
@@ -171,7 +177,7 @@ var resourcesTests = []resTest{
 func TestObserver(t *testing.T) {
 	for _, tt := range resourcesTests {
 		conf := &config.KfConfig{
-			Client:      &restclient.Config{},
+			Client:      new(mockClient),
 			Logger:      log.New("info", "", "test"),
 			ExcludeKind: tt.exclude,
 		}
@@ -212,7 +218,7 @@ var duplicatesTest = []*metav1.APIResourceList{
 
 func TestObserverDuplicas(t *testing.T) {
 	conf := &config.KfConfig{
-		Client:        &restclient.Config{},
+		Client:        new(mockClient),
 		Logger:        log.New("info", "", "test"),
 		ExcludeKind:   make([]string, 0),
 		ExcludeObject: make([]string, 0),
@@ -242,7 +248,7 @@ func TestObserverDuplicas(t *testing.T) {
 
 func TestObserverRecoverFromDicoveryFailure(t *testing.T) {
 	conf := &config.KfConfig{
-		Client:        &restclient.Config{},
+		Client:        new(mockClient),
 		Logger:        log.New("info", "", "test"),
 		ExcludeKind:   make([]string, 0),
 		ExcludeObject: make([]string, 0),