Merge pull request #1101 from tesshuflower/rclone_env_vars

Rclone env vars

controllers/mover/rclone/mover.go

@@ -246,14 +246,22 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
 		}
 		job.Spec.Parallelism = &parallelism
 
-		envVars := []corev1.EnvVar{
+		envVars := []corev1.EnvVar{}
+		// Rclone env vars if they are in the secret
+		envVars = utils.AppendRCloneEnvVars(rcloneConfigSecret, envVars)
+
+		defaultEnvVars := []corev1.EnvVar{
 			{Name: "RCLONE_CONFIG", Value: "/rclone-config/rclone.conf"},
 			{Name: "RCLONE_DEST_PATH", Value: *m.rcloneDestPath},
 			{Name: "DIRECTION", Value: direction},
 			{Name: "MOUNT_PATH", Value: mountPath},
 			{Name: "RCLONE_CONFIG_SECTION", Value: *m.rcloneConfigSection},
 		}
 
+		// Add our defaults after RCLONE_ env vars so any duplicates will be
+		// overridden by the defaults
+		envVars = append(envVars, defaultEnvVars...)
+
 		// Cluster-wide proxy settings
 		envVars = utils.AppendEnvVarsForClusterWideProxy(envVars)
 

controllers/mover/rclone/rclone_test.go

@@ -1323,6 +1323,126 @@ var _ = Describe("Rclone as a source", func() {
 					})
 				})
 
+				When("RCLONE_ env vars are in the rclone secret", func() {
+					BeforeEach(func() {
+						rcloneConfigSecret.StringData = map[string]string{
+							"rclone.config":                        "datahere",
+							"RCLONE_CONFIG_MYS3_TYPE":              "mys3",
+							"RCLONE_CONFIG_MYS3_ACCESS_KEY_ID":     "9999",
+							"RCLONE_CONFIG_MYS3_SECRET_ACCESS_KEY": "111222",
+							"RCLONE_BWLIMIT":                       "5M",
+							"othervar":                             "abc123",
+							"RCLONE_CONFIG":                        "bad", // Should not override what we set
+						}
+					})
+
+					It("Should set the env vars in the mover job pod", func() {
+						j, e := mover.ensureJob(ctx, sPVC, sa, rcloneConfigSecret, nil) // Using sPVC as dataPVC (i.e. direct)
+						Expect(e).NotTo(HaveOccurred())
+						Expect(j).To(BeNil()) // hasn't completed
+						nsn := types.NamespacedName{Name: jobName, Namespace: ns.Name}
+						job = &batchv1.Job{}
+						Expect(k8sClient.Get(ctx, nsn, job)).To(Succeed())
+
+						env := job.Spec.Template.Spec.Containers[0].Env
+
+						// First validate all our normal env vars should be set
+						validateJobEnvVars(env, true)
+
+						t := true
+
+						// Validate our RCLONE_ env vars got set
+						Expect(env).To(ContainElement(corev1.EnvVar{
+							Name: "RCLONE_CONFIG_MYS3_TYPE",
+							ValueFrom: &corev1.EnvVarSource{
+								SecretKeyRef: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: rcloneConfigSecret.GetName(),
+									},
+									Key:      "RCLONE_CONFIG_MYS3_TYPE",
+									Optional: &t,
+								},
+							},
+						}))
+						Expect(env).To(ContainElement(corev1.EnvVar{
+							Name: "RCLONE_CONFIG_MYS3_ACCESS_KEY_ID",
+							ValueFrom: &corev1.EnvVarSource{
+								SecretKeyRef: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: rcloneConfigSecret.GetName(),
+									},
+									Key:      "RCLONE_CONFIG_MYS3_ACCESS_KEY_ID",
+									Optional: &t,
+								},
+							},
+						}))
+						Expect(env).To(ContainElement(corev1.EnvVar{
+							Name: "RCLONE_CONFIG_MYS3_SECRET_ACCESS_KEY",
+							ValueFrom: &corev1.EnvVarSource{
+								SecretKeyRef: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: rcloneConfigSecret.GetName(),
+									},
+									Key:      "RCLONE_CONFIG_MYS3_SECRET_ACCESS_KEY",
+									Optional: &t,
+								},
+							},
+						}))
+						Expect(env).To(ContainElement(corev1.EnvVar{
+							Name: "RCLONE_BWLIMIT",
+							ValueFrom: &corev1.EnvVarSource{
+								SecretKeyRef: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: rcloneConfigSecret.GetName(),
+									},
+									Key:      "RCLONE_BWLIMIT",
+									Optional: &t,
+								},
+							},
+						}))
+						// This one should still get set
+						Expect(env).To(ContainElement(corev1.EnvVar{
+							Name: "RCLONE_CONFIG",
+							ValueFrom: &corev1.EnvVarSource{
+								SecretKeyRef: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: rcloneConfigSecret.GetName(),
+									},
+									Key:      "RCLONE_CONFIG",
+									Optional: &t,
+								},
+							},
+						}))
+
+						// Confirm the RCLONE_CONFIG env var we set is after
+						// the one the user tried to override in the rclone secret
+						// to avoid users accidentally overriding something we need in
+						// the mover job
+						rcloneConfigEnvVars := []corev1.EnvVar{}
+						for _, envVar := range env {
+							if envVar.Name == "RCLONE_CONFIG" {
+								rcloneConfigEnvVars = append(rcloneConfigEnvVars, envVar)
+							}
+						}
+						Expect(len(rcloneConfigEnvVars)).To(Equal(2))
+						first := rcloneConfigEnvVars[0]
+						second := rcloneConfigEnvVars[1]
+						// First one in the env array should be the one from the secret
+						Expect(first.ValueFrom).To(Equal(&corev1.EnvVarSource{
+							SecretKeyRef: &corev1.SecretKeySelector{
+								LocalObjectReference: corev1.LocalObjectReference{
+									Name: rcloneConfigSecret.GetName(),
+								},
+								Key:      "RCLONE_CONFIG",
+								Optional: &t,
+							},
+						}))
+						// Second one should be the one we set for the rclone mover
+						// (this one will take precedence in the mover pod)
+						Expect(second.Value).To(Equal("/rclone-config/rclone.conf"))
+					})
+				})
+
 				It("Should have correct volume mounts", func() {
 					j, e := mover.ensureJob(ctx, sPVC, sa, rcloneConfigSecret, nil) // Using sPVC as dataPVC (i.e. direct)
 					Expect(e).NotTo(HaveOccurred())
@@ -1963,10 +2083,13 @@ func validateJobEnvVars(env []corev1.EnvVar, isSource bool) {
 
 func validateEnvVar(env []corev1.EnvVar, envVarName, envVarExpectedValue string) {
 	found := false
-	for _, envVar := range env {
+	// Reverse over range, look for env vars from the end first
+	for i := len(env) - 1; i >= 0; i-- {
+		envVar := env[i]
 		if envVar.Name == envVarName {
 			found = true
 			Expect(envVar.Value).To(Equal(envVarExpectedValue))
+			break
 		}
 	}
 	Expect(found).To(BeTrue())

controllers/mover/restic/mover.go

@@ -22,9 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"path"
-	"sort"
 	"strconv"
-	"strings"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -409,7 +407,7 @@ func (m *Mover) ensureJob(ctx context.Context, cachePVC *corev1.PersistentVolume
 		}
 
 		// Rclone env vars for restic if they are in the secret
-		envVars = appendRCloneEnvVars(repo, envVars)
+		envVars = utils.AppendRCloneEnvVars(repo, envVars)
 
 		// Cluster-wide proxy settings
 		envVars = utils.AppendEnvVarsForClusterWideProxy(envVars)
@@ -636,24 +634,3 @@ func generateForgetOptions(policy *volsyncv1alpha1.ResticRetainPolicy) string {
 	}
 	return forget
 }
-
-// Append k/v from the restic secret if they start with RCLONE_
-func appendRCloneEnvVars(resticSecret *corev1.Secret, envVars []corev1.EnvVar) []corev1.EnvVar {
-	rcloneKeys := []string{}
-	for key := range resticSecret.Data {
-		if strings.HasPrefix(key, "RCLONE_") {
-			rcloneKeys = append(rcloneKeys, key)
-		}
-	}
-
-	if len(rcloneKeys) > 0 {
-		// Sort so we do not keep re-ordering env vars (range over map keys will give us inconsistent sorting)
-		sort.Strings(rcloneKeys)
-
-		for _, sortedKey := range rcloneKeys {
-			envVars = append(envVars, utils.EnvFromSecret(resticSecret.Name, sortedKey, true))
-		}
-	}
-
-	return envVars
-}

controllers/utils/utils.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"sort"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -187,6 +188,27 @@ func AppendEnvVarsForClusterWideProxy(envVars []corev1.EnvVar) []corev1.EnvVar {
 	return envVars
 }
 
+// Append k/v from the secret if they start with RCLONE_
+func AppendRCloneEnvVars(secret *corev1.Secret, envVars []corev1.EnvVar) []corev1.EnvVar {
+	rcloneKeys := []string{}
+	for key := range secret.Data {
+		if strings.HasPrefix(key, "RCLONE_") {
+			rcloneKeys = append(rcloneKeys, key)
+		}
+	}
+
+	if len(rcloneKeys) > 0 {
+		// Sort so we do not keep re-ordering env vars (range over map keys will give us inconsistent sorting)
+		sort.Strings(rcloneKeys)
+
+		for _, sortedKey := range rcloneKeys {
+			envVars = append(envVars, EnvFromSecret(secret.Name, sortedKey, true))
+		}
+	}
+
+	return envVars
+}
+
 // Updates to set the securityContext, podLabels on mover pod in the spec and resourceRequirements on the mover
 // containers based on what is set in the MoverConfig
 func UpdatePodTemplateSpecFromMoverConfig(podTemplateSpec *corev1.PodTemplateSpec,

controllers/utils/utils_test.go

@@ -225,6 +225,117 @@ var _ = Describe("utils tests", func() {
 		})
 	})
 
+	Describe("AppendRCloneEnvVars", func() {
+		envVarsOrig := []corev1.EnvVar{
+			{
+				Name:  "existingvar1",
+				Value: "value1",
+			},
+			{
+				Name:  "EXISTINGVAR2",
+				Value: "VALUE2",
+			},
+		}
+
+		var envVars []corev1.EnvVar
+		var secret *corev1.Secret
+
+		BeforeEach(func() {
+			// Reset envVars back to initial starting value for test
+			envVars = make([]corev1.EnvVar, len(envVarsOrig))
+			copy(envVars, envVarsOrig)
+
+			secret = &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-secret-withrclonevars",
+					Namespace: "test-secret-namespace",
+				},
+				Data: map[string][]byte{},
+			}
+		})
+
+		When("no secret env vars are set", func() {
+			It("Should not modify the existing env vars", func() {
+				envVars = utils.AppendRCloneEnvVars(secret, envVars)
+				Expect(envVars).To(Equal(envVarsOrig))
+			})
+		})
+
+		When("no env vars containing RCLONE_ are set in the secret", func() {
+			BeforeEach(func() {
+				secret.Data = map[string][]byte{
+					"testkey":        []byte("pineapples"),
+					"NOT_RCLONE_VAR": []byte("kiwis"),
+					"OTHER_VAR":      []byte("oranges"),
+				}
+			})
+			It("Should not modify the existing env vars", func() {
+				envVars = utils.AppendRCloneEnvVars(secret, envVars)
+				Expect(envVars).To(Equal(envVarsOrig))
+			})
+		})
+
+		When("RCLONE_ env vars are set", func() {
+			BeforeEach(func() {
+				secret.Data = map[string][]byte{
+					"RCLONE_TESTVAR1": []byte("veryimportant"),
+					"RCLONE_TESTVAR2": []byte("evenmoreimportant"),
+					"NOT_RCLONE_VAR":  []byte("shouldntbeset"),
+					"RCLONE_BWLIMIT":  []byte("5M:10M"),
+				}
+			})
+
+			It("Should set the appropriate env vars", func() {
+				envVars = utils.AppendRCloneEnvVars(secret, envVars)
+				for i := range envVarsOrig {
+					// Original env vars should still be set
+					origVar := envVarsOrig[i]
+					Expect(envVars).To(ContainElements(origVar))
+				}
+
+				t := true
+
+				// RCLONE_ env vars from secret should be set
+				Expect(envVars).To(ContainElement(corev1.EnvVar{
+					Name: "RCLONE_TESTVAR1",
+					ValueFrom: &corev1.EnvVarSource{
+						SecretKeyRef: &corev1.SecretKeySelector{
+							LocalObjectReference: corev1.LocalObjectReference{
+								Name: secret.GetName(),
+							},
+							Key:      "RCLONE_TESTVAR1",
+							Optional: &t,
+						},
+					},
+				}))
+				Expect(envVars).To(ContainElement(corev1.EnvVar{
+					Name: "RCLONE_TESTVAR2",
+					ValueFrom: &corev1.EnvVarSource{
+						SecretKeyRef: &corev1.SecretKeySelector{
+							LocalObjectReference: corev1.LocalObjectReference{
+								Name: secret.GetName(),
+							},
+							Key:      "RCLONE_TESTVAR2",
+							Optional: &t,
+						},
+					},
+				}))
+				Expect(envVars).To(ContainElement(corev1.EnvVar{
+					Name: "RCLONE_BWLIMIT",
+					ValueFrom: &corev1.EnvVarSource{
+						SecretKeyRef: &corev1.SecretKeySelector{
+							LocalObjectReference: corev1.LocalObjectReference{
+								Name: secret.GetName(),
+							},
+							Key:      "RCLONE_BWLIMIT",
+							Optional: &t,
+						},
+					},
+				}))
+			})
+		})
+	})
+
 	Describe("UpdatePodTemplateSpecFromMoverConfig", func() {
 		When("no pod template spec", func() {
 			It("should not fail", func() {