Replace privateRegistry with imagePullSecrets for tasks

By using the standard k8s imagePullSecrets property on eirini task
instead of providing literal username and password in the
privateRegistry object, we can delete some code and we stay more secure
by not exposing secret values.

Issue: cloudfoundry/korifi#1387

deployment/helm/templates/core/task-crd.yml

@@ -174,6 +174,18 @@ spec:
                 type: array
               image:
                 type: string
+              imagePullSecrets:
+                items:
+                  description: LocalObjectReference contains enough information to
+                    let you locate the referenced object inside the same namespace.
+                  properties:
+                    name:
+                      description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
+                        TODO: Add other useful fields. apiVersion, kind, uid?'
+                      type: string
+                  type: object
+                  x-kubernetes-map-type: atomic
+                type: array
               memoryMB:
                 format: int64
                 type: integer
@@ -183,13 +195,6 @@ spec:
                 type: string
               orgName:
                 type: string
-              privateRegistry:
-                properties:
-                  password:
-                    type: string
-                  username:
-                    type: string
-                type: object
               spaceGUID:
                 type: string
               spaceName:

k8s/jobs/desire.go

@@ -3,28 +3,23 @@ package jobs
 import (
 	"context"
 
-	"code.cloudfoundry.org/eirini-controller/k8s/utils/dockerutils"
 	eiriniv1 "code.cloudfoundry.org/eirini-controller/pkg/apis/eirini/v1"
-	"code.cloudfoundry.org/eirini-controller/util"
 	"code.cloudfoundry.org/lager"
-	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 //counterfeiter:generate . TaskToJobConverter
 //counterfeiter:generate . JobCreator
 //counterfeiter:generate . SecretsClient
 
 type TaskToJobConverter interface {
-	Convert(*eiriniv1.Task, *corev1.Secret) *batchv1.Job
+	Convert(*eiriniv1.Task) *batchv1.Job
 }
 
 type JobCreator interface {
@@ -61,88 +56,19 @@ func NewDesirer(
 func (d *Desirer) Desire(ctx context.Context, task *eiriniv1.Task) (*batchv1.Job, error) {
 	logger := d.logger.Session("desire-task", lager.Data{"guid": task.Spec.GUID, "name": task.Name, "namespace": task.Namespace})
 
-	var (
-		err                   error
-		privateRegistrySecret *corev1.Secret
-	)
-
-	if imageInPrivateRegistry(task) {
-		privateRegistrySecret, err = d.createPrivateRegistrySecret(ctx, task.Namespace, task)
-		if err != nil {
-			return nil, errors.Wrap(err, "failed to create task secret")
-		}
-	}
-
-	job := d.taskToJobConverter.Convert(task, privateRegistrySecret)
+	job := d.taskToJobConverter.Convert(task)
 
 	job.Namespace = task.Namespace
 
-	if err = ctrl.SetControllerReference(task, job, d.scheme); err != nil {
+	if err := ctrl.SetControllerReference(task, job, d.scheme); err != nil {
 		return nil, errors.Wrap(err, "failed to set controller reference")
 	}
 
-	err = d.client.Create(ctx, job)
-	if err != nil {
+	if err := d.client.Create(ctx, job); err != nil {
 		logger.Error("failed-to-create-job", err)
 
-		return nil, d.cleanupAndError(ctx, err, privateRegistrySecret)
-	}
-
-	if privateRegistrySecret != nil {
-		originalSecret := privateRegistrySecret.DeepCopy()
-
-		if err := controllerutil.SetOwnerReference(job, privateRegistrySecret, scheme.Scheme); err != nil {
-			return nil, errors.Wrap(err, "secret-client-set-owner-ref-failed")
-		}
-
-		if err := d.client.Patch(ctx, privateRegistrySecret, client.MergeFrom(originalSecret)); err != nil {
-			return nil, errors.Wrap(err, "failed-to-set-secret-ownership")
-		}
+		return nil, err
 	}
 
 	return job, nil
 }
-
-func imageInPrivateRegistry(task *eiriniv1.Task) bool {
-	return task.Spec.PrivateRegistry != nil && task.Spec.PrivateRegistry.Username != "" && task.Spec.PrivateRegistry.Password != ""
-}
-
-func (d *Desirer) createPrivateRegistrySecret(ctx context.Context, namespace string, task *eiriniv1.Task) (*corev1.Secret, error) {
-	secret := &corev1.Secret{}
-
-	secret.GenerateName = PrivateRegistrySecretGenerateName
-	secret.Namespace = namespace
-	secret.Type = corev1.SecretTypeDockerConfigJson
-
-	dockerConfig := dockerutils.NewDockerConfig(
-		util.ParseImageRegistryHost(task.Spec.Image),
-		task.Spec.PrivateRegistry.Username,
-		task.Spec.PrivateRegistry.Password,
-	)
-
-	dockerConfigJSON, err := dockerConfig.JSON()
-	if err != nil {
-		return nil, errors.Wrap(err, "failed-to-get-docker-config")
-	}
-
-	secret.StringData = map[string]string{
-		dockerutils.DockerConfigKey: dockerConfigJSON,
-	}
-
-	err = d.client.Create(ctx, secret)
-
-	return secret, err
-}
-
-func (d *Desirer) cleanupAndError(ctx context.Context, jobCreationError error, privateRegistrySecret *corev1.Secret) error {
-	resultError := multierror.Append(nil, jobCreationError)
-
-	if privateRegistrySecret != nil {
-		err := d.client.Delete(ctx, privateRegistrySecret)
-		if err != nil {
-			resultError = multierror.Append(resultError, errors.Wrap(err, "failed to cleanup registry secret"))
-		}
-	}
-
-	return resultError
-}

k8s/jobs/desire_test.go

@@ -1,10 +1,6 @@
 package jobs_test
 
 import (
-	"context"
-	"encoding/base64"
-	"fmt"
-
 	"code.cloudfoundry.org/eirini-controller/k8s/jobs"
 	"code.cloudfoundry.org/eirini-controller/k8s/jobs/jobsfakes"
 	"code.cloudfoundry.org/eirini-controller/k8s/k8sfakes"
@@ -17,7 +13,6 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var _ = Describe("Desire", func() {
@@ -55,7 +50,10 @@ var _ = Describe("Desire", func() {
 				Namespace: "app-namespace",
 			},
 			Spec: eiriniv1.TaskSpec{
-				Image:     image,
+				Image: image,
+				ImagePullSecrets: []corev1.LocalObjectReference{{
+					Name: "my-registry-secret",
+				}},
 				Command:   []string{"/lifecycle/launch"},
 				AppName:   "my-app",
 				Name:      "task-name",
@@ -112,122 +110,4 @@ var _ = Describe("Desire", func() {
 	It("sets the job namespace", func() {
 		Expect(job.Namespace).To(Equal("app-namespace"))
 	})
-
-	When("the task uses a private registry", func() {
-		var (
-			createSecretError error
-			createJobError    error
-		)
-
-		BeforeEach(func() {
-			createSecretError = nil
-			createJobError = nil
-			task.Spec.PrivateRegistry = &eiriniv1.PrivateRegistry{
-				Username: "username",
-				Password: "password",
-			}
-
-			client.CreateStub = func(_ context.Context, object k8sclient.Object, _ ...k8sclient.CreateOption) error {
-				secret, ok := object.(*corev1.Secret)
-				if ok {
-					if createSecretError != nil {
-						return createSecretError
-					}
-					secret.Name = secret.GenerateName + "1234"
-				}
-
-				_, ok = object.(*batchv1.Job)
-				if ok {
-					return createJobError
-				}
-
-				return nil
-			}
-		})
-
-		It("creates a secret with the registry credentials", func() {
-			Expect(client.CreateCallCount()).To(Equal(2))
-			_, actualObject, _ := client.CreateArgsForCall(0)
-			actualSecret, ok := actualObject.(*corev1.Secret)
-			Expect(ok).To(BeTrue())
-
-			Expect(actualSecret.GenerateName).To(Equal("private-registry-"))
-			Expect(actualSecret.Type).To(Equal(corev1.SecretTypeDockerConfigJson))
-			Expect(actualSecret.StringData).To(
-				HaveKeyWithValue(
-					".dockerconfigjson",
-					fmt.Sprintf(
-						`{"auths":{"gcr.io":{"username":"username","password":"password","auth":"%s"}}}`,
-						base64.StdEncoding.EncodeToString([]byte("username:password")),
-					),
-				),
-			)
-		})
-
-		It("converts the task using the private registry secret", func() {
-			_, actualSecret := taskToJobConverter.ConvertArgsForCall(0)
-			Expect(actualSecret.Name).To(Equal("private-registry-1234"))
-		})
-
-		It("sets the ownership of the secret to the job", func() {
-			Expect(client.PatchCallCount()).To(Equal(1))
-			_, obj, _, _ := client.PatchArgsForCall(0)
-			Expect(obj).To(BeAssignableToTypeOf(&corev1.Secret{}))
-
-			patchedSecret, ok := obj.(*corev1.Secret)
-			Expect(ok).To(BeTrue())
-
-			Expect(patchedSecret.OwnerReferences).To(HaveLen(1))
-			Expect(patchedSecret.OwnerReferences[0].Kind).To(Equal("Job"))
-			Expect(patchedSecret.OwnerReferences[0].Name).To(Equal(job.Name))
-		})
-
-		When("creating the secret fails", func() {
-			BeforeEach(func() {
-				createSecretError = errors.New("create-secret-err")
-			})
-
-			It("returns an error", func() {
-				Expect(desireErr).To(MatchError(ContainSubstring("create-secret-err")))
-			})
-		})
-
-		When("creating the job fails", func() {
-			BeforeEach(func() {
-				createJobError = errors.New("create-failed")
-			})
-
-			It("returns an error", func() {
-				Expect(desireErr).To(MatchError(ContainSubstring("create-failed")))
-			})
-
-			It("deletes the secret", func() {
-				Expect(client.DeleteCallCount()).To(Equal(1))
-				_, obj, _ := client.DeleteArgsForCall(0)
-				deletedSecret, ok := obj.(*corev1.Secret)
-				Expect(ok).To(BeTrue())
-				Expect(deletedSecret.Name).To(Equal("private-registry-1234"))
-			})
-
-			When("deleting the secret fails", func() {
-				BeforeEach(func() {
-					client.DeleteReturns(errors.New("delete-secret-failed"))
-				})
-
-				It("returns a job creation error and a note that the secret is not cleaned up", func() {
-					Expect(desireErr).To(MatchError(And(ContainSubstring("create-failed"), ContainSubstring("delete-secret-failed"))))
-				})
-			})
-		})
-
-		When("setting the ownership of the secret fails", func() {
-			BeforeEach(func() {
-				client.PatchReturns(errors.New("potato"))
-			})
-
-			It("returns an error", func() {
-				Expect(desireErr).To(MatchError(ContainSubstring("potato")))
-			})
-		})
-	})
 })

k8s/jobs/task_to_job.go

@@ -34,7 +34,7 @@ func NewTaskToJobConverter(
 	}
 }
 
-func (m *Converter) Convert(task *eiriniv1.Task, privateRegistrySecret *corev1.Secret) *batch.Job {
+func (m *Converter) Convert(task *eiriniv1.Task) *batch.Job {
 	job := m.toJob(task)
 	job.Spec.Template.Spec.ServiceAccountName = m.serviceAccountName
 	job.Labels[LabelSourceType] = TaskSourceType
@@ -66,16 +66,8 @@ func (m *Converter) Convert(task *eiriniv1.Task, privateRegistrySecret *corev1.S
 		},
 	}
 
-	job.Spec.Template.Spec.ImagePullSecrets = []corev1.LocalObjectReference{
-		{
-			Name: m.registrySecretName,
-		},
-	}
-
-	if privateRegistrySecret != nil {
-		job.Spec.Template.Spec.ImagePullSecrets = append(job.Spec.Template.Spec.ImagePullSecrets,
-			corev1.LocalObjectReference{Name: privateRegistrySecret.Name})
-	}
+	job.Spec.Template.Spec.ImagePullSecrets = []corev1.LocalObjectReference{{Name: m.registrySecretName}}
+	job.Spec.Template.Spec.ImagePullSecrets = append(job.Spec.Template.Spec.ImagePullSecrets, task.Spec.ImagePullSecrets...)
 
 	job.Spec.Template.Spec.Containers = containers