2144 pull secrets in jobs fix (#2156)

* Add 'repo-sync-image-pullsecrets' param in apprepository-controller

It accepts a comma separated values list for passing the names of the Secrets to be used when pulling images (i.e., from a private repo)

* Add 'repo-sync-image-pullsecrets'  argument generation from chart

If a values.global.imagePullSecrets is passed, it is also forwarded to the apprepository-controller.
Added a helper function to generate the string.

* Add if to avoid nil pointer in template

Since lazy evaluation is not possible, we should to split up the if sentence

* Remove global flag variables. Adapt flag parsing for working in tests

* Add unit tests for command line arguments

* Param generation without comma separated lst

* Change back namespace to kubeappsNamespace. Change flag pck to pflag

* Fix wrong namespace utilization

config.KubeappsNamespace was wrongly being used in some parts. To avoid confusion, now the whole apprepo is being passed

* Minor fixes after PR review

chart/kubeapps/templates/apprepository-deployment.yaml

@@ -43,6 +43,13 @@ spec:
           args:
             - --user-agent-comment=kubeapps/{{ .Chart.AppVersion }}
             - --repo-sync-image={{ template "kubeapps.image" (list .Values.apprepository.syncImage .Values.global) }}
+           {{- if .Values.global }}
+             {{- if.Values.global.imagePullSecrets }}
+               {{- range $key, $value := .Values.global.imagePullSecrets }}
+            -  --repo-sync-image-pullsecrets={{ $value | quote }}
+               {{- end }}
+             {{- end }}
+           {{- end }}
             - --repo-sync-cmd=/asset-syncer
             - --namespace={{ .Release.Namespace }}
             - --database-secret-name={{ .Values.postgresql.existingSecret }}

cmd/apprepository-controller/controller.go

@@ -92,7 +92,7 @@ type Controller struct {
 	// Kubernetes API.
 	recorder record.EventRecorder
 
-	kubeappsNamespace string
+	conf Config
 }
 
 // NewController returns a new sample controller
@@ -101,7 +101,7 @@ func NewController(
 	apprepoclientset clientset.Interface,
 	kubeInformerFactory kubeinformers.SharedInformerFactory,
 	apprepoInformerFactory informers.SharedInformerFactory,
-	kubeappsNamespace string) *Controller {
+	conf *Config) *Controller {
 
 	// obtain references to shared index informers for the CronJob and
 	// AppRepository types.
@@ -119,15 +119,15 @@ func NewController(
 	recorder := eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: controllerAgentName})
 
 	controller := &Controller{
-		kubeclientset:     kubeclientset,
-		apprepoclientset:  apprepoclientset,
-		cronjobsLister:    cronjobInformer.Lister(),
-		cronjobsSynced:    cronjobInformer.Informer().HasSynced,
-		appreposLister:    apprepoInformer.Lister(),
-		appreposSynced:    apprepoInformer.Informer().HasSynced,
-		workqueue:         workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "AppRepositories"),
-		recorder:          recorder,
-		kubeappsNamespace: kubeappsNamespace,
+		kubeclientset:    kubeclientset,
+		apprepoclientset: apprepoclientset,
+		cronjobsLister:   cronjobInformer.Lister(),
+		cronjobsSynced:   cronjobInformer.Informer().HasSynced,
+		appreposLister:   apprepoInformer.Lister(),
+		appreposSynced:   apprepoInformer.Informer().HasSynced,
+		workqueue:        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "AppRepositories"),
+		recorder:         recorder,
+		conf:             *conf,
 	}
 
 	log.Info("Setting up event handlers")
@@ -273,35 +273,35 @@ func (c *Controller) syncHandler(key string) error {
 		if errors.IsNotFound(err) {
 			log.Infof("AppRepository '%s' no longer exists so performing cleanup of charts from the DB", key)
 			// Trigger a Job to perfrom the cleanup of the charts in the DB corresponding to deleted AppRepository
-			_, err = c.kubeclientset.BatchV1().Jobs(namespace).Create(context.TODO(), newCleanupJob(name, namespace, c.kubeappsNamespace), metav1.CreateOptions{})
+			_, err = c.kubeclientset.BatchV1().Jobs(namespace).Create(context.TODO(), newCleanupJob(apprepo, c.conf), metav1.CreateOptions{})
 			return nil
 		}
 		return fmt.Errorf("Error fetching object with key %s from store: %v", key, err)
 	}
 
 	// Get the cronjob with the same name as AppRepository
 	cronjobName := cronJobName(apprepo)
-	cronjob, err := c.cronjobsLister.CronJobs(c.kubeappsNamespace).Get(cronjobName)
+	cronjob, err := c.cronjobsLister.CronJobs(c.conf.KubeappsNamespace).Get(cronjobName)
 	// If the resource doesn't exist, we'll create it
 	if errors.IsNotFound(err) {
 		log.Infof("Creating CronJob %q for AppRepository %q", cronjobName, apprepo.GetName())
-		cronjob, err = c.kubeclientset.BatchV1beta1().CronJobs(c.kubeappsNamespace).Create(context.TODO(), newCronJob(apprepo, c.kubeappsNamespace), metav1.CreateOptions{})
+		cronjob, err = c.kubeclientset.BatchV1beta1().CronJobs(c.conf.KubeappsNamespace).Create(context.TODO(), newCronJob(apprepo, c.conf), metav1.CreateOptions{})
 		if err != nil {
 			return err
 		}
 
 		// Trigger a manual Job for the initial sync
-		_, err = c.kubeclientset.BatchV1().Jobs(c.kubeappsNamespace).Create(context.TODO(), newSyncJob(apprepo, c.kubeappsNamespace), metav1.CreateOptions{})
+		_, err = c.kubeclientset.BatchV1().Jobs(c.conf.KubeappsNamespace).Create(context.TODO(), newSyncJob(apprepo, c.conf), metav1.CreateOptions{})
 	} else if err == nil {
 		// If the resource already exists, we'll update it
-		log.Infof("Updating CronJob %q in namespace %q for AppRepository %q in namespace %q", cronjobName, c.kubeappsNamespace, apprepo.GetName(), apprepo.GetNamespace())
-		cronjob, err = c.kubeclientset.BatchV1beta1().CronJobs(c.kubeappsNamespace).Update(context.TODO(), newCronJob(apprepo, c.kubeappsNamespace), metav1.UpdateOptions{})
+		log.Infof("Updating CronJob %q in namespace %q for AppRepository %q in namespace %q", cronjobName, c.conf.KubeappsNamespace, apprepo.GetName(), apprepo.GetNamespace())
+		cronjob, err = c.kubeclientset.BatchV1beta1().CronJobs(c.conf.KubeappsNamespace).Update(context.TODO(), newCronJob(apprepo, c.conf), metav1.UpdateOptions{})
 		if err != nil {
 			return err
 		}
 
 		// The AppRepository has changed, launch a manual Job
-		_, err = c.kubeclientset.BatchV1().Jobs(c.kubeappsNamespace).Create(context.TODO(), newSyncJob(apprepo, c.kubeappsNamespace), metav1.CreateOptions{})
+		_, err = c.kubeclientset.BatchV1().Jobs(c.conf.KubeappsNamespace).Create(context.TODO(), newSyncJob(apprepo, c.conf), metav1.CreateOptions{})
 	}
 
 	// If an error occurs during Get/Create, we'll requeue the item so we can
@@ -320,7 +320,7 @@ func (c *Controller) syncHandler(key string) error {
 		return fmt.Errorf(msg)
 	}
 
-	if apprepo.GetNamespace() == c.kubeappsNamespace {
+	if apprepo.GetNamespace() == c.conf.KubeappsNamespace {
 		c.recorder.Event(apprepo, corev1.EventTypeNormal, SuccessSynced, MessageResourceSynced)
 	}
 	return nil
@@ -409,48 +409,48 @@ func ownerReferencesForAppRepo(apprepo *apprepov1alpha1.AppRepository, childName
 // newCronJob creates a new CronJob for a AppRepository resource. It also sets
 // the appropriate OwnerReferences on the resource so handleObject can discover
 // the AppRepository resource that 'owns' it.
-func newCronJob(apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace string) *batchv1beta1.CronJob {
+func newCronJob(apprepo *apprepov1alpha1.AppRepository, config Config) *batchv1beta1.CronJob {
 	return &batchv1beta1.CronJob{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            cronJobName(apprepo),
-			OwnerReferences: ownerReferencesForAppRepo(apprepo, kubeappsNamespace),
+			OwnerReferences: ownerReferencesForAppRepo(apprepo, config.KubeappsNamespace),
 			Labels:          jobLabels(apprepo),
 		},
 		Spec: batchv1beta1.CronJobSpec{
-			Schedule: crontab,
+			Schedule: config.Crontab,
 			// Set to replace as short-circuit in k8s <1.12
 			// TODO re-evaluate ConcurrentPolicy when 1.12+ is mainstream (i.e 1.14)
 			// https://github.com/kubernetes/kubernetes/issues/54870
 			ConcurrencyPolicy: "Replace",
 			JobTemplate: batchv1beta1.JobTemplateSpec{
-				Spec: syncJobSpec(apprepo, kubeappsNamespace),
+				Spec: syncJobSpec(apprepo, config),
 			},
 		},
 	}
 }
 
 // newSyncJob triggers a job for the AppRepository resource. It also sets the
 // appropriate OwnerReferences on the resource
-func newSyncJob(apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace string) *batchv1.Job {
+func newSyncJob(apprepo *apprepov1alpha1.AppRepository, config Config) *batchv1.Job {
 	return &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName:    cronJobName(apprepo) + "-",
-			OwnerReferences: ownerReferencesForAppRepo(apprepo, kubeappsNamespace),
+			OwnerReferences: ownerReferencesForAppRepo(apprepo, config.KubeappsNamespace),
 		},
-		Spec: syncJobSpec(apprepo, kubeappsNamespace),
+		Spec: syncJobSpec(apprepo, config),
 	}
 }
 
 // jobSpec returns a batchv1.JobSpec for running the chart-repo sync job
-func syncJobSpec(apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace string) batchv1.JobSpec {
+func syncJobSpec(apprepo *apprepov1alpha1.AppRepository, config Config) batchv1.JobSpec {
 	volumes := []corev1.Volume{}
 	volumeMounts := []corev1.VolumeMount{}
 	if apprepo.Spec.Auth.CustomCA != nil {
 		volumes = append(volumes, corev1.Volume{
 			Name: apprepo.Spec.Auth.CustomCA.SecretKeyRef.Name,
 			VolumeSource: corev1.VolumeSource{
 				Secret: &corev1.SecretVolumeSource{
-					SecretName: secretKeyRefForRepo(apprepo.Spec.Auth.CustomCA.SecretKeyRef, apprepo, kubeappsNamespace).Name,
+					SecretName: secretKeyRefForRepo(apprepo.Spec.Auth.CustomCA.SecretKeyRef, apprepo, config).Name,
 					Items: []corev1.KeyToPath{
 						{Key: apprepo.Spec.Auth.CustomCA.SecretKeyRef.Key, Path: "ca.crt"},
 					},
@@ -480,12 +480,15 @@ func syncJobSpec(apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace strin
 	if len(podTemplateSpec.Spec.Containers) == 0 {
 		podTemplateSpec.Spec.Containers = []corev1.Container{{}}
 	}
+	// Populate ImagePullSecrets spec
+	podTemplateSpec.Spec.ImagePullSecrets = append(podTemplateSpec.Spec.ImagePullSecrets, config.ImagePullSecretsRefs...)
+
 	podTemplateSpec.Spec.Containers[0].Name = "sync"
-	podTemplateSpec.Spec.Containers[0].Image = repoSyncImage
+	podTemplateSpec.Spec.Containers[0].Image = config.RepoSyncImage
 	podTemplateSpec.Spec.Containers[0].ImagePullPolicy = "IfNotPresent"
-	podTemplateSpec.Spec.Containers[0].Command = []string{repoSyncCommand}
-	podTemplateSpec.Spec.Containers[0].Args = apprepoSyncJobArgs(apprepo)
-	podTemplateSpec.Spec.Containers[0].Env = append(podTemplateSpec.Spec.Containers[0].Env, apprepoSyncJobEnvVars(apprepo, kubeappsNamespace)...)
+	podTemplateSpec.Spec.Containers[0].Command = []string{config.RepoSyncCommand}
+	podTemplateSpec.Spec.Containers[0].Args = apprepoSyncJobArgs(apprepo, config)
+	podTemplateSpec.Spec.Containers[0].Env = append(podTemplateSpec.Spec.Containers[0].Env, apprepoSyncJobEnvVars(apprepo, config)...)
 	podTemplateSpec.Spec.Containers[0].VolumeMounts = append(podTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMounts...)
 	// Add volumes
 	podTemplateSpec.Spec.Volumes = append(podTemplateSpec.Spec.Volumes, volumes...)
@@ -497,37 +500,39 @@ func syncJobSpec(apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace strin
 
 // newCleanupJob triggers a job for the AppRepository resource. It also sets the
 // appropriate OwnerReferences on the resource
-func newCleanupJob(reponame, namespace, kubeappsNamespace string) *batchv1.Job {
+func newCleanupJob(apprepo *apprepov1alpha1.AppRepository, config Config) *batchv1.Job {
 	return &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: deleteJobName(reponame, namespace) + "-",
-			Namespace:    kubeappsNamespace,
+			GenerateName: deleteJobName(apprepo) + "-",
+			Namespace:    apprepo.GetNamespace(),
 		},
-		Spec: cleanupJobSpec(reponame, namespace),
+		Spec: cleanupJobSpec(apprepo, config),
 	}
 }
 
 // cleanupJobSpec returns a batchv1.JobSpec for running the chart-repo delete job
-func cleanupJobSpec(repoName, repoNamespace string) batchv1.JobSpec {
+func cleanupJobSpec(apprepo *apprepov1alpha1.AppRepository, config Config) batchv1.JobSpec {
+	log.Info("Creating cleaunp")
 	return batchv1.JobSpec{
 		Template: corev1.PodTemplateSpec{
 			Spec: corev1.PodSpec{
 				// If there's an issue, delay till the next cron
-				RestartPolicy: "Never",
+				RestartPolicy:    "Never",
+				ImagePullSecrets: config.ImagePullSecretsRefs,
 				Containers: []corev1.Container{
 					{
 						Name:            "delete",
-						Image:           repoSyncImage,
+						Image:           config.RepoSyncImage,
 						ImagePullPolicy: "IfNotPresent",
-						Command:         []string{repoSyncCommand},
-						Args:            apprepoCleanupJobArgs(repoName, repoNamespace),
+						Command:         []string{config.RepoSyncCommand},
+						Args:            apprepoCleanupJobArgs(apprepo, config),
 						Env: []corev1.EnvVar{
 							{
 								Name: "DB_PASSWORD",
 								ValueFrom: &corev1.EnvVarSource{
 									SecretKeyRef: &corev1.SecretKeySelector{
-										LocalObjectReference: corev1.LocalObjectReference{Name: dbSecretName},
-										Key:                  dbSecretKey,
+										LocalObjectReference: corev1.LocalObjectReference{Name: config.DBSecretName},
+										Key:                  config.DBSecretKey,
 									},
 								},
 							},
@@ -553,38 +558,38 @@ func cronJobName(apprepo *apprepov1alpha1.AppRepository) string {
 }
 
 // deleteJobName returns a unique name for the Job to cleanup AppRepository
-func deleteJobName(reponame, reponamespace string) string {
-	return fmt.Sprintf("apprepo-%s-cleanup-%s", reponamespace, reponame)
+func deleteJobName(apprepo *apprepov1alpha1.AppRepository) string {
+	return fmt.Sprintf("apprepo-%s-cleanup-%s", apprepo.GetNamespace(), apprepo.GetName())
 }
 
 // apprepoSyncJobArgs returns a list of args for the sync container
-func apprepoSyncJobArgs(apprepo *apprepov1alpha1.AppRepository) []string {
-	args := append([]string{"sync"}, dbFlags()...)
+func apprepoSyncJobArgs(apprepo *apprepov1alpha1.AppRepository, config Config) []string {
+	args := append([]string{"sync"}, dbFlags(config)...)
 
-	if userAgentComment != "" {
-		args = append(args, "--user-agent-comment="+userAgentComment)
+	if config.UserAgentComment != "" {
+		args = append(args, "--user-agent-comment="+config.UserAgentComment)
 	}
 
 	return append(args, "--namespace="+apprepo.GetNamespace(), apprepo.GetName(), apprepo.Spec.URL)
 }
 
 // apprepoSyncJobEnvVars returns a list of env variables for the sync container
-func apprepoSyncJobEnvVars(apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace string) []corev1.EnvVar {
+func apprepoSyncJobEnvVars(apprepo *apprepov1alpha1.AppRepository, config Config) []corev1.EnvVar {
 	var envVars []corev1.EnvVar
 	envVars = append(envVars, corev1.EnvVar{
 		Name: "DB_PASSWORD",
 		ValueFrom: &corev1.EnvVarSource{
 			SecretKeyRef: &corev1.SecretKeySelector{
-				LocalObjectReference: corev1.LocalObjectReference{Name: dbSecretName},
-				Key:                  dbSecretKey,
+				LocalObjectReference: corev1.LocalObjectReference{Name: config.DBSecretName},
+				Key:                  config.DBSecretKey,
 			},
 		},
 	})
 	if apprepo.Spec.Auth.Header != nil {
 		envVars = append(envVars, corev1.EnvVar{
 			Name: "AUTHORIZATION_HEADER",
 			ValueFrom: &corev1.EnvVarSource{
-				SecretKeyRef: secretKeyRefForRepo(apprepo.Spec.Auth.Header.SecretKeyRef, apprepo, kubeappsNamespace),
+				SecretKeyRef: secretKeyRefForRepo(apprepo.Spec.Auth.Header.SecretKeyRef, apprepo, config),
 			},
 		})
 	}
@@ -595,27 +600,27 @@ func apprepoSyncJobEnvVars(apprepo *apprepov1alpha1.AppRepository, kubeappsNames
 // this repo is in the kubeapps namespace or not. If the repo is not in the
 // kubeapps namespace, then the secret will have been copied from another namespace
 // into the kubeapps namespace and have a slightly different name.
-func secretKeyRefForRepo(keyRef corev1.SecretKeySelector, apprepo *apprepov1alpha1.AppRepository, kubeappsNamespace string) *corev1.SecretKeySelector {
-	if apprepo.ObjectMeta.Namespace == kubeappsNamespace {
+func secretKeyRefForRepo(keyRef corev1.SecretKeySelector, apprepo *apprepov1alpha1.AppRepository, config Config) *corev1.SecretKeySelector {
+	if apprepo.ObjectMeta.Namespace == config.KubeappsNamespace {
 		return &keyRef
 	}
 	keyRef.LocalObjectReference.Name = kube.KubeappsSecretNameForRepo(apprepo.ObjectMeta.Name, apprepo.ObjectMeta.Namespace)
 	return &keyRef
 }
 
 // apprepoCleanupJobArgs returns a list of args for the repo cleanup container
-func apprepoCleanupJobArgs(repoName, repoNamespace string) []string {
+func apprepoCleanupJobArgs(apprepo *apprepov1alpha1.AppRepository, config Config) []string {
 	return append([]string{
 		"delete",
-		repoName,
-		"--namespace=" + repoNamespace,
-	}, dbFlags()...)
+		apprepo.GetName(),
+		"--namespace=" + apprepo.GetNamespace(),
+	}, dbFlags(config)...)
 }
 
-func dbFlags() []string {
+func dbFlags(config Config) []string {
 	return []string{
-		"--database-url=" + dbURL,
-		"--database-user=" + dbUser,
-		"--database-name=" + dbName,
+		"--database-url=" + config.DBURL,
+		"--database-user=" + config.DBUser,
+		"--database-name=" + config.DBName,
 	}
 }