Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Commit

Permalink
Replace privateRegistry with imagePullSecrets for tasks
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Kieron Browne committed Jul 29, 2022
1 parent 155f71a commit e54a445
Show file tree
Hide file tree
Showing 12 changed files with 69 additions and 317 deletions.
19 changes: 12 additions & 7 deletions deployment/helm/templates/core/task-crd.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -183,13 +195,6 @@ spec:
type: string
orgName:
type: string
privateRegistry:
properties:
password:
type: string
username:
type: string
type: object
spaceGUID:
type: string
spaceName:
Expand Down
84 changes: 5 additions & 79 deletions k8s/jobs/desire.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
128 changes: 4 additions & 124 deletions k8s/jobs/desire_test.go
View file
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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() {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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")))
})
})
})
})
19 changes: 8 additions & 11 deletions k8s/jobs/jobsfakes/fake_task_to_job_converter.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 3 additions & 11 deletions k8s/jobs/task_to_job.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
Loading

0 comments on commit e54a445

Please sign in to comment.