Merge pull request #1748 from nrb/fix-1743

Use custom namespace when creating Velero clients

pkg/client/client.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 the Velero contributors.
+Copyright 2017, 2019 the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -29,12 +29,20 @@ import (
 
 // Config returns a *rest.Config, using either the kubeconfig (if specified) or an in-cluster
 // configuration.
-func Config(kubeconfig, kubecontext, baseName string) (*rest.Config, error) {
+func Config(kubeconfig, kubecontext, baseName string, qps float32, burst int) (*rest.Config, error) {
 	loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
 	loadingRules.ExplicitPath = kubeconfig
 	configOverrides := &clientcmd.ConfigOverrides{CurrentContext: kubecontext}
 	kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides)
 	clientConfig, err := kubeConfig.ClientConfig()
+	if qps > 0.0 {
+		clientConfig.QPS = qps
+	}
+
+	if burst > 0 {
+		clientConfig.Burst = burst
+	}
+
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}

pkg/client/factory.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 the Velero contributors.
+Copyright 2017, 2019 the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -24,6 +24,7 @@ import (
 	"github.com/spf13/pflag"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
 
 	v1 "github.com/heptio/velero/pkg/apis/velero/v1"
 	clientset "github.com/heptio/velero/pkg/generated/clientset/versioned"
@@ -42,6 +43,17 @@ type Factory interface {
 	// DynamicClient returns a Kubernetes dynamic client. It uses the following priority to specify the cluster
 	// configuration: --kubeconfig flag, KUBECONFIG environment variable, in-cluster configuration.
 	DynamicClient() (dynamic.Interface, error)
+	// SetBasename changes the basename for an already-constructed client.
+	// This is useful for generating clients that require a different user-agent string below the root `velero`
+	// command, such as the server subcommand.
+	SetBasename(string)
+	// SetClientQPS sets the Queries Per Second for a client.
+	SetClientQPS(float32)
+	// SetClientBurst sets the Burst for a client.
+	SetClientBurst(int)
+	// ClientConfig returns a rest.Config struct used for client-go clients.
+	ClientConfig() (*rest.Config, error)
+	// Namespace returns the namespace which the Factory will create clients for.
 	Namespace() string
 }
 
@@ -51,6 +63,8 @@ type factory struct {
 	kubecontext string
 	baseName    string
 	namespace   string
+	clientQPS   float32
+	clientBurst int
 }
 
 // NewFactory returns a Factory.
@@ -60,12 +74,19 @@ func NewFactory(baseName string) Factory {
 		baseName: baseName,
 	}
 
+	f.namespace = os.Getenv("VELERO_NAMESPACE")
+
 	if config, err := LoadConfig(); err == nil {
-		f.namespace = config[ConfigKeyNamespace]
+		// Only override the namespace if the config key is set
+		if _, ok := config[ConfigKeyNamespace]; ok {
+			f.namespace = config[ConfigKeyNamespace]
+		}
 	} else {
 		fmt.Fprintf(os.Stderr, "WARNING: error retrieving namespace from config file: %v\n", err)
 	}
 
+	// We didn't get the namespace via env var or config file, so use the default.
+	// Command line flags will override when BindFlags is called.
 	if f.namespace == "" {
 		f.namespace = v1.DefaultNamespace
 	}
@@ -81,8 +102,12 @@ func (f *factory) BindFlags(flags *pflag.FlagSet) {
 	flags.AddFlagSet(f.flags)
 }
 
+func (f *factory) ClientConfig() (*rest.Config, error) {
+	return Config(f.kubeconfig, f.kubecontext, f.baseName, f.clientQPS, f.clientBurst)
+}
+
 func (f *factory) Client() (clientset.Interface, error) {
-	clientConfig, err := Config(f.kubeconfig, f.kubecontext, f.baseName)
+	clientConfig, err := f.ClientConfig()
 	if err != nil {
 		return nil, err
 	}
@@ -95,7 +120,7 @@ func (f *factory) Client() (clientset.Interface, error) {
 }
 
 func (f *factory) KubeClient() (kubernetes.Interface, error) {
-	clientConfig, err := Config(f.kubeconfig, f.kubecontext, f.baseName)
+	clientConfig, err := f.ClientConfig()
 	if err != nil {
 		return nil, err
 	}
@@ -108,11 +133,10 @@ func (f *factory) KubeClient() (kubernetes.Interface, error) {
 }
 
 func (f *factory) DynamicClient() (dynamic.Interface, error) {
-	clientConfig, err := Config(f.kubeconfig, f.kubecontext, f.baseName)
+	clientConfig, err := f.ClientConfig()
 	if err != nil {
 		return nil, err
 	}
-
 	dynamicClient, err := dynamic.NewForConfig(clientConfig)
 	if err != nil {
 		return nil, errors.WithStack(err)
@@ -120,6 +144,18 @@ func (f *factory) DynamicClient() (dynamic.Interface, error) {
 	return dynamicClient, nil
 }
 
+func (f *factory) SetBasename(name string) {
+	f.baseName = name
+}
+
+func (f *factory) SetClientQPS(qps float32) {
+	f.clientQPS = qps
+}
+
+func (f *factory) SetClientBurst(burst int) {
+	f.clientBurst = burst
+}
+
 func (f *factory) Namespace() string {
 	return f.namespace
 }

pkg/client/factory_test.go

@@ -0,0 +1,61 @@
+/*
+Copyright 2019 the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package client
+
+import (
+	"os"
+	"testing"
+
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/assert"
+)
+
+// TestFactory tests the client.Factory interface.
+func TestFactory(t *testing.T) {
+	// Velero client configuration is currently omitted due to requiring a
+	// test filesystem in pkg/test. This causes an import cycle as pkg/test
+	// uses pkg/client's interfaces to implement fakes
+
+	// Env variable should set the namespace if no config or argument are used
+	os.Setenv("VELERO_NAMESPACE", "env-velero")
+	f := NewFactory("velero")
+
+	assert.Equal(t, "env-velero", f.Namespace())
+
+	os.Unsetenv("VELERO_NAMESPACE")
+
+	// Argument should change the namespace
+	f = NewFactory("velero")
+	s := "flag-velero"
+	flags := new(pflag.FlagSet)
+
+	f.BindFlags(flags)
+
+	flags.Parse([]string{"--namespace", s})
+
+	assert.Equal(t, s, f.Namespace())
+
+	// An arugment overrides the env variable if both are set.
+	os.Setenv("VELERO_NAMESPACE", "env-velero")
+	f = NewFactory("velero")
+	flags = new(pflag.FlagSet)
+
+	f.BindFlags(flags)
+	flags.Parse([]string{"--namespace", s})
+	assert.Equal(t, s, f.Namespace())
+
+	os.Unsetenv("VELERO_NAMESPACE")
+}

pkg/cmd/cli/restic/server.go

@@ -61,7 +61,8 @@ func NewServerCommand(f client.Factory) *cobra.Command {
 			logger := logging.DefaultLogger(logLevel, formatFlag.Parse())
 			logger.Infof("Starting Velero restic server %s (%s)", buildinfo.Version, buildinfo.FormattedGitSHA())
 
-			s, err := newResticServer(logger, fmt.Sprintf("%s-%s", c.Parent().Name(), c.Name()))
+			f.SetBasename(fmt.Sprintf("%s-%s", c.Parent().Name(), c.Name()))
+			s, err := newResticServer(logger, f)
 			cmd.CheckError(err)
 
 			s.run()
@@ -87,20 +88,16 @@ type resticServer struct {
 	fileSystem            filesystem.Interface
 }
 
-func newResticServer(logger logrus.FieldLogger, baseName string) (*resticServer, error) {
-	clientConfig, err := client.Config("", "", baseName)
+func newResticServer(logger logrus.FieldLogger, factory client.Factory) (*resticServer, error) {
+
+	kubeClient, err := factory.KubeClient()
 	if err != nil {
 		return nil, err
 	}
 
-	kubeClient, err := kubernetes.NewForConfig(clientConfig)
+	veleroClient, err := factory.Client()
 	if err != nil {
-		return nil, errors.WithStack(err)
-	}
-
-	veleroClient, err := clientset.NewForConfig(clientConfig)
-	if err != nil {
-		return nil, errors.WithStack(err)
+		return nil, err
 	}
 
 	// use a stand-alone pod informer because we want to use a field selector to
@@ -123,7 +120,7 @@ func newResticServer(logger logrus.FieldLogger, baseName string) (*resticServer,
 	// to fully-encrypted backups and have unique keys per repository.
 	secretInformer := corev1informers.NewFilteredSecretInformer(
 		kubeClient,
-		os.Getenv("VELERO_NAMESPACE"),
+		factory.Namespace(),
 		0,
 		cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc},
 		func(opts *metav1.ListOptions) {
@@ -136,7 +133,7 @@ func newResticServer(logger logrus.FieldLogger, baseName string) (*resticServer,
 	s := &resticServer{
 		kubeClient:            kubeClient,
 		veleroClient:          veleroClient,
-		veleroInformerFactory: informers.NewFilteredSharedInformerFactory(veleroClient, 0, os.Getenv("VELERO_NAMESPACE"), nil),
+		veleroInformerFactory: informers.NewFilteredSharedInformerFactory(veleroClient, 0, factory.Namespace(), nil),
 		kubeInformerFactory:   kubeinformers.NewSharedInformerFactory(kubeClient, 0),
 		podInformer:           podInformer,
 		secretInformer:        secretInformer,

pkg/cmd/server/server.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 the Velero contributors.
+Copyright 2017, 2019 the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -19,7 +19,6 @@ package server
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
 	"log"
 	"net/http"
 	"net/http/pprof"
@@ -32,7 +31,6 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kubeerrs "k8s.io/apimachinery/pkg/util/errors"
@@ -127,7 +125,7 @@ type controllerRunInfo struct {
 	numWorkers int
 }
 
-func NewCommand() *cobra.Command {
+func NewCommand(f client.Factory) *cobra.Command {
 	var (
 		volumeSnapshotLocations = flag.NewMap().WithKeyValueDelimiter(":")
 		logLevelFlag            = logging.LogLevelFlag(logrus.InfoLevel)
@@ -171,25 +169,13 @@ func NewCommand() *cobra.Command {
 
 			logger.Infof("Starting Velero server %s (%s)", buildinfo.Version, buildinfo.FormattedGitSHA())
 
-			// NOTE: the namespace flag is bound to velero's persistent flags when the root velero command
-			// creates the client Factory and binds the Factory's flags. We're not using a Factory here in
-			// the server because the Factory gets its basename set at creation time, and the basename is
-			// used to construct the user-agent for clients. Also, the Factory's Namespace() method uses
-			// the client config file to determine the appropriate namespace to use, and that isn't
-			// applicable to the server (it uses the method directly below instead). We could potentially
-			// add a SetBasename() method to the Factory, and tweak how Namespace() works, if we wanted to
-			// have the server use the Factory.
-			namespaceFlag := c.Flag("namespace")
-			if namespaceFlag == nil {
-				cmd.CheckError(errors.New("unable to look up namespace flag"))
-			}
-			namespace := getServerNamespace(namespaceFlag)
-
 			if volumeSnapshotLocations.Data() != nil {
 				config.defaultVolumeSnapshotLocations = volumeSnapshotLocations.Data()
 			}
 
-			s, err := newServer(namespace, fmt.Sprintf("%s-%s", c.Parent().Name(), c.Name()), config, logger)
+			f.SetBasename(fmt.Sprintf("%s-%s", c.Parent().Name(), c.Name()))
+
+			s, err := newServer(f, config, logger)
 			cmd.CheckError(err)
 
 			cmd.CheckError(s.run())
@@ -216,20 +202,6 @@ func NewCommand() *cobra.Command {
 	return command
 }
 
-func getServerNamespace(namespaceFlag *pflag.Flag) string {
-	if namespaceFlag.Changed {
-		return namespaceFlag.Value.String()
-	}
-
-	if data, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
-		if ns := strings.TrimSpace(string(data)); len(ns) > 0 {
-			return ns
-		}
-	}
-
-	return api.DefaultNamespace
-}
-
 type server struct {
 	namespace             string
 	metricsAddress        string
@@ -251,29 +223,30 @@ type server struct {
 	config                serverConfig
 }
 
-func newServer(namespace, baseName string, config serverConfig, logger *logrus.Logger) (*server, error) {
-	clientConfig, err := client.Config("", "", baseName)
-	if err != nil {
-		return nil, err
-	}
+func newServer(f client.Factory, config serverConfig, logger *logrus.Logger) (*server, error) {
 	if config.clientQPS < 0.0 {
 		return nil, errors.New("client-qps must be positive")
 	}
-	clientConfig.QPS = config.clientQPS
+	f.SetClientQPS(config.clientQPS)
 
 	if config.clientBurst <= 0 {
 		return nil, errors.New("client-burst must be positive")
 	}
-	clientConfig.Burst = config.clientBurst
+	f.SetClientBurst(config.clientBurst)
 
-	kubeClient, err := kubernetes.NewForConfig(clientConfig)
+	kubeClient, err := f.KubeClient()
 	if err != nil {
-		return nil, errors.WithStack(err)
+		return nil, err
 	}
 
-	veleroClient, err := clientset.NewForConfig(clientConfig)
+	veleroClient, err := f.Client()
 	if err != nil {
-		return nil, errors.WithStack(err)
+		return nil, err
+	}
+
+	dynamicClient, err := f.DynamicClient()
+	if err != nil {
+		return nil, err
 	}
 
 	pluginRegistry := clientmgmt.NewRegistry(config.pluginDir, logger, logger.Level)
@@ -285,22 +258,22 @@ func newServer(namespace, baseName string, config serverConfig, logger *logrus.L
 		return nil, err
 	}
 
-	dynamicClient, err := dynamic.NewForConfig(clientConfig)
+	ctx, cancelFunc := context.WithCancel(context.Background())
+
+	clientConfig, err := f.ClientConfig()
 	if err != nil {
 		return nil, err
 	}
 
-	ctx, cancelFunc := context.WithCancel(context.Background())
-
 	s := &server{
-		namespace:             namespace,
+		namespace:             f.Namespace(),
 		metricsAddress:        config.metricsAddress,
 		kubeClientConfig:      clientConfig,
 		kubeClient:            kubeClient,
 		veleroClient:          veleroClient,
 		discoveryClient:       veleroClient.Discovery(),
 		dynamicClient:         dynamicClient,
-		sharedInformerFactory: informers.NewSharedInformerFactoryWithOptions(veleroClient, 0, informers.WithNamespace(namespace)),
+		sharedInformerFactory: informers.NewSharedInformerFactoryWithOptions(veleroClient, 0, informers.WithNamespace(f.Namespace())),
 		ctx:                   ctx,
 		cancelFunc:            cancelFunc,
 		logger:                logger,

pkg/cmd/velero/velero.go

@@ -63,7 +63,7 @@ operations can also be performed as 'velero backup get' and 'velero schedule cre
 		backup.NewCommand(f),
 		schedule.NewCommand(f),
 		restore.NewCommand(f),
-		server.NewCommand(),
+		server.NewCommand(f),
 		version.NewCommand(f),
 		get.NewCommand(f),
 		install.NewCommand(f),

pkg/install/deployment.go

@@ -144,6 +144,14 @@ func Deployment(namespace string, opts ...podTemplateOption) *appsv1.Deployment
 									Name:  "VELERO_SCRATCH_DIR",
 									Value: "/scratch",
 								},
+								{
+									Name: "VELERO_NAMESPACE",
+									ValueFrom: &corev1.EnvVarSource{
+										FieldRef: &corev1.ObjectFieldSelector{
+											FieldPath: "metadata.namespace",
+										},
+									},
+								},
 							},
 							Resources: c.resources,
 						},

pkg/install/deployment_test.go

@@ -32,7 +32,7 @@ func TestDeployment(t *testing.T) {
 	assert.Equal(t, "--restore-only", deploy.Spec.Template.Spec.Containers[0].Args[1])
 
 	deploy = Deployment("velero", WithEnvFromSecretKey("my-var", "my-secret", "my-key"))
-	envSecret := deploy.Spec.Template.Spec.Containers[0].Env[1]
+	envSecret := deploy.Spec.Template.Spec.Containers[0].Env[2]
 	assert.Equal(t, "my-var", envSecret.Name)
 	assert.Equal(t, "my-secret", envSecret.ValueFrom.SecretKeyRef.LocalObjectReference.Name)
 	assert.Equal(t, "my-key", envSecret.ValueFrom.SecretKeyRef.Key)
@@ -42,6 +42,6 @@ func TestDeployment(t *testing.T) {
 	assert.Equal(t, corev1.PullIfNotPresent, deploy.Spec.Template.Spec.Containers[0].ImagePullPolicy)
 
 	deploy = Deployment("velero", WithSecret(true))
-	assert.Equal(t, 4, len(deploy.Spec.Template.Spec.Containers[0].Env))
+	assert.Equal(t, 5, len(deploy.Spec.Template.Spec.Containers[0].Env))
 	assert.Equal(t, 3, len(deploy.Spec.Template.Spec.Volumes))
 }