Merge pull request #139 from tesshuflower/viper_env_vars

Use viper to allow both cmd line and env vars for mover container imgs

Author: openshift-merge-robot

controllers/mover/rclone/builder.go

@@ -22,32 +22,67 @@ import (
 	"fmt"
 
 	"github.com/go-logr/logr"
+	"github.com/spf13/viper"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	volsyncv1alpha1 "github.com/backube/volsync/api/v1alpha1"
 	"github.com/backube/volsync/controllers/mover"
 	"github.com/backube/volsync/controllers/volumehandler"
 )
 
-// defaultRcloneContainerImage is the default container image for the rclone
-// data mover
-const defaultRcloneContainerImage = "quay.io/backube/volsync-mover-rclone:latest"
-
-// rcloneContainerImage is the container image name of the rclone data mover
-var rcloneContainerImage string
+const (
+	// defaultRcloneContainerImage is the default container image for the rclone
+	// data mover
+	defaultRcloneContainerImage = "quay.io/backube/volsync-mover-rclone:latest"
+	// Command line flag will be checked first
+	// If command line flag not set, the RELATED_IMAGE_ env var will be used
+	rcloneContainerImageFlag   = "rclone-container-image"
+	rcloneContainerImageEnvVar = "RELATED_IMAGE_RCLONE_CONTAINER"
+)
 
-type Builder struct{}
+type Builder struct {
+	viper *viper.Viper  // For unit tests to be able to override - global viper will be used by default in Register()
+	flags *flag.FlagSet // For unit tests to be able to override - global flags will be used by default in Register()
+}
 
 var _ mover.Builder = &Builder{}
 
-func Register() {
-	flag.StringVar(&rcloneContainerImage, "rclone-container-image",
-		defaultRcloneContainerImage, "The container image for the rclone data mover")
-	mover.Register(&Builder{})
+func Register() error {
+	// Use global viper & command line flags
+	b, err := newBuilder(viper.GetViper(), flag.CommandLine)
+	if err != nil {
+		return err
+	}
+
+	mover.Register(b)
+	return nil
+}
+
+func newBuilder(viper *viper.Viper, flags *flag.FlagSet) (*Builder, error) {
+	b := &Builder{
+		viper: viper,
+		flags: flags,
+	}
+
+	// Set default rclone container image - will be used if both command line flag and env var are not set
+	b.viper.SetDefault(rcloneContainerImageFlag, defaultRcloneContainerImage)
+
+	// Setup command line flag for the rclone container image
+	b.flags.String(rcloneContainerImageFlag, defaultRcloneContainerImage,
+		"The container image for the rclone data mover")
+	// Viper will check for command line flag first, then fallback to the env var
+	err := b.viper.BindEnv(rcloneContainerImageFlag, rcloneContainerImageEnvVar)
+
+	return b, err
 }
 
 func (rb *Builder) VersionInfo() string {
-	return fmt.Sprintf("Rclone container: %s", rcloneContainerImage)
+	return fmt.Sprintf("Rclone container: %s", rb.getRcloneContainerImage())
+}
+
+// rcloneContainerImage is the container image name of the rclone data mover
+func (rb *Builder) getRcloneContainerImage() string {
+	return rb.viper.GetString(rcloneContainerImageFlag)
 }
 
 func (rb *Builder) FromSource(client client.Client, logger logr.Logger,
@@ -71,6 +106,7 @@ func (rb *Builder) FromSource(client client.Client, logger logr.Logger,
 		logger:              logger.WithValues("method", "Rclone"),
 		owner:               source,
 		vh:                  vh,
+		containerImage:      rb.getRcloneContainerImage(),
 		rcloneConfigSection: source.Spec.Rclone.RcloneConfigSection,
 		rcloneDestPath:      source.Spec.Rclone.RcloneDestPath,
 		rcloneConfig:        source.Spec.Rclone.RcloneConfig,
@@ -101,6 +137,7 @@ func (rb *Builder) FromDestination(client client.Client, logger logr.Logger,
 		logger:              logger.WithValues("method", "Rclone"),
 		owner:               destination,
 		vh:                  vh,
+		containerImage:      rb.getRcloneContainerImage(),
 		rcloneConfigSection: destination.Spec.Rclone.RcloneConfigSection,
 		rcloneDestPath:      destination.Spec.Rclone.RcloneDestPath,
 		rcloneConfig:        destination.Spec.Rclone.RcloneConfig,

controllers/mover/rclone/mover.go

@@ -47,6 +47,7 @@ type Mover struct {
 	logger              logr.Logger
 	owner               metav1.Object
 	vh                  *volumehandler.VolumeHandler
+	containerImage      string
 	rcloneConfigSection *string
 	rcloneDestPath      *string
 	rcloneConfig        *string
@@ -236,7 +237,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
 				{Name: "RCLONE_CONFIG_SECTION", Value: *m.rcloneConfigSection},
 			},
 			Command: []string{"/bin/bash", "-c", "./active.sh"},
-			Image:   rcloneContainerImage,
+			Image:   m.containerImage,
 			SecurityContext: &corev1.SecurityContext{
 				RunAsUser: &runAsUser,
 			},

controllers/mover/rclone/rclone_test.go

@@ -18,9 +18,14 @@ along with this program.  If not, see <https://www.gnu.org/licenses/>.
 package rclone
 
 import (
+	"flag"
+	"os"
+
 	snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1beta1"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/spf13/pflag"
+	"github.com/spf13/viper"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -49,8 +54,9 @@ var emptyString = ""
 var _ = Describe("Rclone properly registers", func() {
 	When("Rclone's registration function is called", func() {
 		BeforeEach(func() {
-			Register()
+			Expect(Register()).To(Succeed())
 		})
+
 		It("is added to the mover catalog", func() {
 			found := false
 			for _, v := range mover.Catalog {
@@ -63,6 +69,115 @@ var _ = Describe("Rclone properly registers", func() {
 	})
 })
 
+var _ = Describe("Rclone init flags and env vars", func() {
+	logger := zap.New(zap.UseDevMode(true), zap.WriteTo(GinkgoWriter))
+	When("Rclone builder inits flags", func() {
+		var builderForInitTests *Builder
+		var testPflagSet *pflag.FlagSet
+		BeforeEach(func() {
+			os.Unsetenv(rcloneContainerImageEnvVar)
+
+			// Instantiate new viper instance and flagset instance just for this test
+			testViper := viper.New()
+			testFlagSet := flag.NewFlagSet("testflagsetrclone", flag.ExitOnError)
+
+			// New Builder for this test - use testViper and testFlagSet so we can modify
+			// flags for these tests without modifying global flags and potentially affecting other tests
+			var err error
+			builderForInitTests, err = newBuilder(testViper, testFlagSet)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(builderForInitTests).NotTo(BeNil())
+
+			// code here (see main.go) for viper to bind cmd line flags (including those
+			// defined in the mover Register() func)
+			// Bind viper to a new set of flags so each of these tests can get their own
+			testPflagSet = pflag.NewFlagSet("testpflagsetrclone", pflag.ExitOnError)
+			testPflagSet.AddGoFlagSet(testFlagSet)
+			Expect(testViper.BindPFlags(testPflagSet)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			os.Unsetenv(rcloneContainerImageEnvVar)
+		})
+
+		JustBeforeEach(func() {
+			// Common checks - make sure if we instantiate a source/dest mover, it uses the container image that
+			// was picked up by flags/command line etc from the builder
+			var err error
+
+			rs := &volsyncv1alpha1.ReplicationSource{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "testrscr",
+					Namespace: "testing",
+				},
+				Spec: volsyncv1alpha1.ReplicationSourceSpec{
+					Rclone: &volsyncv1alpha1.ReplicationSourceRcloneSpec{},
+				},
+				Status: &volsyncv1alpha1.ReplicationSourceStatus{}, // Controller sets status to non-nil
+			}
+			sourceMover, err := builderForInitTests.FromSource(k8sClient, logger, rs)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(sourceMover).NotTo(BeNil())
+			sourceRcloneMover, _ := sourceMover.(*Mover)
+			Expect(sourceRcloneMover.containerImage).To(Equal(builderForInitTests.getRcloneContainerImage()))
+
+			rd := &volsyncv1alpha1.ReplicationDestination{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "rd",
+					Namespace: "testing",
+				},
+				Spec: volsyncv1alpha1.ReplicationDestinationSpec{
+					Trigger: &volsyncv1alpha1.ReplicationDestinationTriggerSpec{},
+					Rclone:  &volsyncv1alpha1.ReplicationDestinationRcloneSpec{},
+				},
+				Status: &volsyncv1alpha1.ReplicationDestinationStatus{}, // Controller sets status to non-nil
+			}
+			destMover, err := builderForInitTests.FromDestination(k8sClient, logger, rd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(destMover).NotTo(BeNil())
+			destRcloneMover, _ := destMover.(*Mover)
+			Expect(destRcloneMover.containerImage).To(Equal(builderForInitTests.getRcloneContainerImage()))
+		})
+
+		Context("When no command line flag or ENV var is specified", func() {
+			It("Should use the default rclone container image", func() {
+				Expect(builderForInitTests.getRcloneContainerImage()).To(Equal(defaultRcloneContainerImage))
+			})
+		})
+
+		Context("When rclone container image command line flag is specified", func() {
+			const cmdLineOverrideImageName = "test-rclone-image-name:cmdlineoverride"
+			BeforeEach(func() {
+				// Manually set the value of the command line flag
+				Expect(testPflagSet.Set("rclone-container-image", cmdLineOverrideImageName)).To(Succeed())
+			})
+			It("Should use the rclone container image set by the cmd line flag", func() {
+				Expect(builderForInitTests.getRcloneContainerImage()).To(Equal(cmdLineOverrideImageName))
+			})
+
+			Context("And env var is set", func() {
+				const envVarOverrideShouldBeIgnored = "test-rclone-image-name:donotuseme"
+				BeforeEach(func() {
+					os.Setenv(rcloneContainerImageEnvVar, envVarOverrideShouldBeIgnored)
+				})
+				It("Should still use the cmd line flag instead of the env var", func() {
+					Expect(builderForInitTests.getRcloneContainerImage()).To(Equal(cmdLineOverrideImageName))
+				})
+			})
+		})
+
+		Context("When rclone container image cmd line flag is not set and env var is", func() {
+			const envVarOverrideImageName = "test-rclone-image-name:setbyenvvar"
+			BeforeEach(func() {
+				os.Setenv(rcloneContainerImageEnvVar, envVarOverrideImageName)
+			})
+			It("Should use the value from the env var", func() {
+				Expect(builderForInitTests.getRcloneContainerImage()).To(Equal(envVarOverrideImageName))
+			})
+		})
+	})
+})
+
 var _ = Describe("Rclone ignores other movers", func() {
 	logger := zap.New(zap.UseDevMode(true), zap.WriteTo(GinkgoWriter))
 	When("An RS isn't for rclone", func() {
@@ -76,8 +191,7 @@ var _ = Describe("Rclone ignores other movers", func() {
 					Rclone: nil,
 				},
 			}
-			builder := Builder{}
-			m, e := builder.FromSource(k8sClient, logger, rs)
+			m, e := commonBuilderForTestSuite.FromSource(k8sClient, logger, rs)
 			Expect(m).To(BeNil())
 			Expect(e).NotTo(HaveOccurred())
 		})
@@ -93,8 +207,7 @@ var _ = Describe("Rclone ignores other movers", func() {
 					Rclone: nil,
 				},
 			}
-			builder := Builder{}
-			m, e := builder.FromDestination(k8sClient, logger, rd)
+			m, e := commonBuilderForTestSuite.FromDestination(k8sClient, logger, rd)
 			Expect(m).To(BeNil())
 			Expect(e).NotTo(HaveOccurred())
 		})
@@ -168,9 +281,7 @@ var _ = Describe("Rclone as a source", func() {
 			// Controller sets status to non-nil
 			rs.Status = &volsyncv1alpha1.ReplicationSourceStatus{}
 			// Instantiate a rclone mover for the tests
-			b := Builder{}
-			var err error
-			m, err := b.FromSource(k8sClient, logger, rs)
+			m, err := commonBuilderForTestSuite.FromSource(k8sClient, logger, rs)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(m).NotTo(BeNil())
 			mover, _ = m.(*Mover)
@@ -401,7 +512,6 @@ var _ = Describe("Rclone as a source", func() {
 				}
 			})
 			JustBeforeEach(func() {
-				rcloneContainerImage = "therclonecontainerimage"
 				Expect(k8sClient.Create(ctx, sa)).To(Succeed())
 				Expect(k8sClient.Create(ctx, rcloneConfigSecret)).To(Succeed())
 			})
@@ -431,7 +541,7 @@ var _ = Describe("Rclone as a source", func() {
 						return err
 					}).Should(Succeed())
 					Expect(len(job.Spec.Template.Spec.Containers)).To(BeNumerically(">", 0))
-					Expect(job.Spec.Template.Spec.Containers[0].Image).To(Equal(rcloneContainerImage))
+					Expect(job.Spec.Template.Spec.Containers[0].Image).To(Equal(defaultRcloneContainerImage))
 				})
 
 				It("should use the specified service account", func() {
@@ -640,9 +750,7 @@ var _ = Describe("Rclone as a destination", func() {
 			// Controller sets status to non-nil
 			rd.Status = &volsyncv1alpha1.ReplicationDestinationStatus{}
 			// Instantiate a restic mover for the tests
-			b := Builder{}
-			var err error
-			m, err := b.FromDestination(k8sClient, logger, rd)
+			m, err := commonBuilderForTestSuite.FromDestination(k8sClient, logger, rd)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(m).NotTo(BeNil())
 			mover, _ = m.(*Mover)
@@ -769,7 +877,6 @@ var _ = Describe("Rclone as a destination", func() {
 				}
 			})
 			JustBeforeEach(func() {
-				rcloneContainerImage = "therclonecontainerimage"
 				Expect(k8sClient.Create(ctx, dPVC)).To(Succeed())
 				Expect(k8sClient.Create(ctx, sa)).To(Succeed())
 				Expect(k8sClient.Create(ctx, rcloneConfigSecret)).To(Succeed())

controllers/mover/rclone/suite_test.go

@@ -19,12 +19,14 @@ package rclone
 
 import (
 	"context"
+	"flag"
 	"path/filepath"
 	"testing"
 
 	snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1beta1"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/spf13/viper"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -44,6 +46,7 @@ import (
 var cfg *rest.Config
 var k8sClient client.Client
 var testEnv *envtest.Environment
+var commonBuilderForTestSuite *Builder
 var cancel context.CancelFunc
 var ctx context.Context
 
@@ -114,6 +117,12 @@ var _ = BeforeSuite(func() {
 		k8sClient = k8sManager.GetClient()
 		return k8sClient
 	}, "60s", "1s").Should(Not(BeNil()))
+
+	// Instantiate common rsync builder to use for tests in this test suite
+	commonBuilderForTestSuite, err = newBuilder(viper.New(), flag.NewFlagSet("testfsetrclonetests", flag.ExitOnError))
+	Expect(err).NotTo(HaveOccurred())
+	Expect(commonBuilderForTestSuite).NotTo(BeNil())
+
 })
 
 var _ = AfterSuite(func() {