Add authentication for private index images

anik120 · anik120 · commit f2ad0ee54f22 · 2020-12-02T16:11:13.000-05:00
Fixes Issue #1536 #1505 This PR attaches the secrets in the spec.Secrets field of the CatalogSource to the default service account in the CatalogSource's namespace, to allow for pulling of index images that are backed by authentication.
diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
@@ -1424,7 +1424,7 @@ func toManifest(t *testing.T, obj runtime.Object) string {
 }
 
 func pod(s v1alpha1.CatalogSource) *corev1.Pod {
-	pod := reconciler.Pod(&s, "registry-server", s.Spec.Image, s.GetLabels(), 5, 10)
+	pod := reconciler.Pod(&s, "registry-server", s.Spec.Image, s.GetName(), s.GetLabels(), 5, 10)
 	ownerutil.AddOwner(pod, &s, false, false)
 	return pod
 }
diff --git a/pkg/controller/registry/reconciler/configmap.go b/pkg/controller/registry/reconciler/configmap.go
@@ -95,7 +95,7 @@ func (s *configMapCatalogSourceDecorator) Service() *v1.Service {
 }
 
 func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod {
-	pod := Pod(s.CatalogSource, "configmap-registry-server", image, s.Labels(), 5, 2)
+	pod := Pod(s.CatalogSource, "configmap-registry-server", image, "", s.Labels(), 5, 2)
 	pod.Spec.ServiceAccountName = s.GetName() + ConfigMapServerPostfix
 	pod.Spec.Containers[0].Command = []string{"configmap-server", "-c", s.Spec.ConfigMap, "-n", s.GetNamespace()}
 	ownerutil.AddOwner(pod, s.CatalogSource, false, false)
diff --git a/pkg/controller/registry/reconciler/configmap_test.go b/pkg/controller/registry/reconciler/configmap_test.go
@@ -204,7 +204,7 @@ func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object {
 		if catsrc.Spec.Image != "" {
 			decorated := grpcCatalogSourceDecorator{catsrc}
 			objs = clientfake.AddSimpleGeneratedNames(
-				decorated.Pod(),
+				decorated.Pod(""),
 				decorated.Service(),
 			)
 		}
diff --git a/pkg/controller/registry/reconciler/grpc.go b/pkg/controller/registry/reconciler/grpc.go
@@ -16,6 +16,8 @@ import (
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
+	k8serror "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -86,8 +88,34 @@ func (s *grpcCatalogSourceDecorator) Service() *corev1.Service {
 	return svc
 }
 
-func (s *grpcCatalogSourceDecorator) Pod() *corev1.Pod {
-	pod := Pod(s.CatalogSource, "registry-server", s.Spec.Image, s.Labels(), 5, 10)
+func (s *grpcCatalogSourceDecorator) ServiceAccount() *corev1.ServiceAccount {
+	var secrets []corev1.LocalObjectReference
+	blockOwnerDeletion := true
+	isController := true
+	for _, secretName := range s.CatalogSource.Spec.Secrets {
+		secrets = append(secrets, corev1.LocalObjectReference{Name: secretName})
+	}
+	return &corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      s.GetName(),
+			Namespace: s.GetNamespace(),
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					Name:               s.GetName(),
+					Kind:               v1alpha1.CatalogSourceKind,
+					APIVersion:         v1alpha1.CatalogSourceCRDAPIVersion,
+					UID:                s.GetUID(),
+					Controller:         &isController,
+					BlockOwnerDeletion: &blockOwnerDeletion,
+				},
+			},
+		},
+		ImagePullSecrets: secrets,
+	}
+}
+
+func (s *grpcCatalogSourceDecorator) Pod(saName string) *corev1.Pod {
+	pod := Pod(s.CatalogSource, "registry-server", s.Spec.Image, saName, s.Labels(), 5, 10)
 	ownerutil.AddOwner(pod, s.CatalogSource, false, false)
 	return pod
 }
@@ -160,14 +188,18 @@ func (c *GrpcRegistryReconciler) EnsureRegistryServer(catalogSource *v1alpha1.Ca
 	overwritePod := overwrite || len(c.currentPodsWithCorrectImage(source)) == 0
 
 	//TODO: if any of these error out, we should write a status back (possibly set RegistryServiceStatus to nil so they get recreated)
-	if err := c.ensurePod(source, overwritePod); err != nil {
-		return errors.Wrapf(err, "error ensuring pod: %s", source.Pod().GetName())
+	sa, err := c.ensureSA(source)
+	if err != nil && !k8serror.IsAlreadyExists(err) {
+		return errors.Wrapf(err, "error ensuring service account: %s", source.GetName())
+	}
+	if err := c.ensurePod(source, sa.GetName(), overwritePod); err != nil {
+		return errors.Wrapf(err, "error ensuring pod: %s", source.Pod(sa.Name).GetName())
 	}
-	if err := c.ensureUpdatePod(source); err != nil {
+	if err := c.ensureUpdatePod(source, sa.Name); err != nil {
 		if _, ok := err.(UpdateNotReadyErr); ok {
 			return err
 		}
-		return errors.Wrapf(err, "error ensuring updated catalog source pod: %s", source.Pod().GetName())
+		return errors.Wrapf(err, "error ensuring updated catalog source pod: %s", source.Pod(sa.Name).GetName())
 	}
 	if err := c.ensureService(source, overwrite); err != nil {
 		return errors.Wrapf(err, "error ensuring service: %s", source.Service().GetName())
@@ -186,7 +218,7 @@ func (c *GrpcRegistryReconciler) EnsureRegistryServer(catalogSource *v1alpha1.Ca
 	return nil
 }
 
-func (c *GrpcRegistryReconciler) ensurePod(source grpcCatalogSourceDecorator, overwrite bool) error {
+func (c *GrpcRegistryReconciler) ensurePod(source grpcCatalogSourceDecorator, saName string, overwrite bool) error {
 	// currentLivePods refers to the currently live instances of the catalog source
 	currentLivePods := c.currentPods(source)
 	if len(currentLivePods) > 0 {
@@ -199,16 +231,16 @@ func (c *GrpcRegistryReconciler) ensurePod(source grpcCatalogSourceDecorator, ov
 			}
 		}
 	}
-	_, err := c.OpClient.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).Create(context.TODO(), source.Pod(), metav1.CreateOptions{})
+	_, err := c.OpClient.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).Create(context.TODO(), source.Pod(saName), metav1.CreateOptions{})
 	if err != nil {
-		return errors.Wrapf(err, "error creating new pod: %s", source.Pod().GetGenerateName())
+		return errors.Wrapf(err, "error creating new pod: %s", source.Pod(saName).GetGenerateName())
 	}
 
 	return nil
 }
 
 // ensureUpdatePod checks that for the same catalog source version the same container imageID is running
-func (c *GrpcRegistryReconciler) ensureUpdatePod(source grpcCatalogSourceDecorator) error {
+func (c *GrpcRegistryReconciler) ensureUpdatePod(source grpcCatalogSourceDecorator, saName string) error {
 	if !source.Poll() {
 		return nil
 	}
@@ -218,7 +250,7 @@ func (c *GrpcRegistryReconciler) ensureUpdatePod(source grpcCatalogSourceDecorat
 
 	if source.Update() && len(currentUpdatePods) == 0 {
 		logrus.WithField("CatalogSource", source.GetName()).Infof("catalog update required at %s", time.Now().String())
-		pod, err := c.createUpdatePod(source)
+		pod, err := c.createUpdatePod(source, saName)
 		if err != nil {
 			return errors.Wrapf(err, "creating update catalog source pod")
 		}
@@ -283,6 +315,14 @@ func (c *GrpcRegistryReconciler) ensureService(source grpcCatalogSourceDecorator
 	return err
 }
 
+func (c *GrpcRegistryReconciler) ensureSA(source grpcCatalogSourceDecorator) (*v1.ServiceAccount, error) {
+	sa := source.ServiceAccount()
+	if _, err := c.OpClient.CreateServiceAccount(sa); err != nil {
+		return sa, err
+	}
+	return sa, nil
+}
+
 // ServiceHashMatch will check the hash info in existing Service to ensure its
 // hash info matches the desired Service's hash.
 func ServiceHashMatch(existing, new *corev1.Service) bool {
@@ -317,14 +357,14 @@ func HashServiceSpec(spec corev1.ServiceSpec) string {
 }
 
 // createUpdatePod is an internal method that creates a pod using the latest catalog source.
-func (c *GrpcRegistryReconciler) createUpdatePod(source grpcCatalogSourceDecorator) (*corev1.Pod, error) {
+func (c *GrpcRegistryReconciler) createUpdatePod(source grpcCatalogSourceDecorator, saName string) (*corev1.Pod, error) {
 	// remove label from pod to ensure service does not accidentally route traffic to the pod
-	p := source.Pod()
+	p := source.Pod(saName)
 	p = swapLabels(p, "", source.Name)
 
 	pod, err := c.OpClient.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).Create(context.TODO(), p, metav1.CreateOptions{})
 	if err != nil {
-		logrus.WithField("pod", source.Pod().GetName()).Warn("couldn't create new catalogsource pod")
+		logrus.WithField("pod", source.Pod(saName).GetName()).Warn("couldn't create new catalogsource pod")
 		return nil, err
 	}
 
diff --git a/pkg/controller/registry/reconciler/grpc_test.go b/pkg/controller/registry/reconciler/grpc_test.go
@@ -33,8 +33,27 @@ func validGrpcCatalogSource(image, address string) *v1alpha1.CatalogSource {
 	}
 }
 
+func grpcCatalogSourceWithSecret(secretName string) *v1alpha1.CatalogSource {
+	return &v1alpha1.CatalogSource{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "private-catalog",
+			Namespace: testNamespace,
+			UID:       types.UID("catalog-uid"),
+			Labels:    map[string]string{"olm.catalogSource": "img-catalog"},
+		},
+		Spec: v1alpha1.CatalogSourceSpec{
+			Image:      "private-image",
+			Address:    "",
+			SourceType: v1alpha1.SourceTypeGrpc,
+			Secrets:    []string{secretName},
+		},
+	}
+}
+
 func TestGrpcRegistryReconciler(t *testing.T) {
 	now := func() metav1.Time { return metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC) }
+	blockOwnerDeletion := true
+	isController := true
 
 	type cluster struct {
 		k8sObjs []runtime.Object
@@ -44,8 +63,9 @@ func TestGrpcRegistryReconciler(t *testing.T) {
 		catsrc  *v1alpha1.CatalogSource
 	}
 	type out struct {
-		status *v1alpha1.RegistryServiceStatus
-		err    error
+		status  *v1alpha1.RegistryServiceStatus
+		cluster cluster
+		err     error
 	}
 	tests := []struct {
 		testName string
@@ -186,6 +206,56 @@ func TestGrpcRegistryReconciler(t *testing.T) {
 				},
 			},
 		},
+		{
+			testName: "Grpc/PrivateRegistry/SAHasSecrets",
+			in: in{
+				cluster: cluster{
+					k8sObjs: []runtime.Object{
+						&corev1.Secret{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      "test-secret",
+								Namespace: testNamespace,
+							},
+						},
+					},
+				},
+				catsrc: grpcCatalogSourceWithSecret("test-secret"),
+			},
+			out: out{
+				status: &v1alpha1.RegistryServiceStatus{
+					CreatedAt:        now(),
+					Protocol:         "grpc",
+					ServiceName:      "private-catalog",
+					ServiceNamespace: testNamespace,
+					Port:             "50051",
+				},
+				cluster: cluster{
+					k8sObjs: []runtime.Object{
+						&corev1.ServiceAccount{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      "private-catalog",
+								Namespace: testNamespace,
+								OwnerReferences: []metav1.OwnerReference{
+									{
+										Name:               "private-catalog",
+										UID:                types.UID("catalog-uid"),
+										Kind:               v1alpha1.CatalogSourceKind,
+										APIVersion:         v1alpha1.CatalogSourceCRDAPIVersion,
+										BlockOwnerDeletion: &blockOwnerDeletion,
+										Controller:         &isController,
+									},
+								},
+							},
+							ImagePullSecrets: []corev1.LocalObjectReference{
+								{
+									Name: "test-secret",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.testName, func(t *testing.T) {
@@ -206,12 +276,13 @@ func TestGrpcRegistryReconciler(t *testing.T) {
 
 			// Check for resource existence
 			decorated := grpcCatalogSourceDecorator{tt.in.catsrc}
-			pod := decorated.Pod()
+			pod := decorated.Pod(tt.in.catsrc.GetName())
 			service := decorated.Service()
+			sa := decorated.ServiceAccount()
 			listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()}
 			outPods, podErr := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(context.TODO(), listOptions)
 			outService, serviceErr := client.KubernetesInterface().CoreV1().Services(service.GetNamespace()).Get(context.TODO(), service.GetName(), metav1.GetOptions{})
-
+			outsa, saerr := client.KubernetesInterface().CoreV1().ServiceAccounts(sa.GetNamespace()).Get(context.TODO(), sa.GetName(), metav1.GetOptions{})
 			switch rec.(type) {
 			case *GrpcRegistryReconciler:
 				// Should be created by a GrpcRegistryReconciler
@@ -223,6 +294,10 @@ func TestGrpcRegistryReconciler(t *testing.T) {
 				require.Equal(t, pod.Spec, outPod.Spec)
 				require.NoError(t, serviceErr)
 				require.Equal(t, service, outService)
+				require.NoError(t, saerr)
+				if len(tt.in.catsrc.Spec.Secrets) > 0 {
+					require.Equal(t, tt.out.cluster.k8sObjs[0], outsa)
+				}
 			case *GrpcAddressRegistryReconciler:
 				// Should not be created by a GrpcAddressRegistryReconciler
 				require.NoError(t, podErr)
diff --git a/pkg/controller/registry/reconciler/reconciler.go b/pkg/controller/registry/reconciler/reconciler.go
@@ -91,7 +91,7 @@ func NewRegistryReconcilerFactory(lister operatorlister.OperatorLister, opClient
 	}
 }
 
-func Pod(source *v1alpha1.CatalogSource, name string, image string, labels map[string]string, readinessDelay int32, livenessDelay int32) *v1.Pod {
+func Pod(source *v1alpha1.CatalogSource, name string, image string, saName string, labels map[string]string, readinessDelay int32, livenessDelay int32) *v1.Pod {
 	// Ensure the catalog image is always pulled if the image is not based on a digest, measured by whether an "@" is included.
 	// See https://github.com/docker/distribution/blob/master/reference/reference.go for more info.
 	// This means recreating non-digest based catalog pods will result in the latest version of the catalog content being delivered on-cluster.
@@ -148,6 +148,7 @@ func Pod(source *v1alpha1.CatalogSource, name string, image string, labels map[s
 			NodeSelector: map[string]string{
 				"kubernetes.io/os": "linux",
 			},
+			ServiceAccountName: saName,
 		},
 	}
 	return pod
diff --git a/pkg/controller/registry/reconciler/reconciler_test.go b/pkg/controller/registry/reconciler/reconciler_test.go
@@ -19,7 +19,7 @@ func TestPodNodeSelector(t *testing.T) {
 	key := "kubernetes.io/os"
 	value := "linux"
 
-	gotCatSrcPod := Pod(catsrc, "hello", "busybox", map[string]string{}, int32(0), int32(0))
+	gotCatSrcPod := Pod(catsrc, "hello", "busybox", "", map[string]string{}, int32(0), int32(0))
 	gotCatSrcPodSelector := gotCatSrcPod.Spec.NodeSelector
 
 	if gotCatSrcPodSelector[key] != value {
@@ -67,7 +67,7 @@ func TestPullPolicy(t *testing.T) {
 	}
 
 	for _, tt := range table {
-		p := Pod(source, "catalog", tt.image, nil, int32(0), int32(0))
+		p := Pod(source, "catalog", tt.image, "", nil, int32(0), int32(0))
 		policy := p.Spec.Containers[0].ImagePullPolicy
 		if policy != tt.policy {
 			t.Fatalf("expected pull policy %s for image  %s", tt.policy, tt.image)

Original file line number	Diff line number	Diff line change
`@@ -1424,7 +1424,7 @@ func toManifest(t *testing.T, obj runtime.Object) string {`
`1424`	`1424`	`}`
`1425`	`1425`
`1426`	`1426`	`func pod(s v1alpha1.CatalogSource) *corev1.Pod {`
`1427`		`- pod := reconciler.Pod(&s, "registry-server", s.Spec.Image, s.GetLabels(), 5, 10)`
	`1427`	`+ pod := reconciler.Pod(&s, "registry-server", s.Spec.Image, s.GetName(), s.GetLabels(), 5, 10)`
`1428`	`1428`	`ownerutil.AddOwner(pod, &s, false, false)`
`1429`	`1429`	`return pod`
`1430`	`1430`	`}`
Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ func (s configMapCatalogSourceDecorator) Service() v1.Service {`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`func (s configMapCatalogSourceDecorator) Pod(image string) v1.Pod {`
`98`		`- pod := Pod(s.CatalogSource, "configmap-registry-server", image, s.Labels(), 5, 2)`
	`98`	`+ pod := Pod(s.CatalogSource, "configmap-registry-server", image, "", s.Labels(), 5, 2)`
`99`	`99`	`pod.Spec.ServiceAccountName = s.GetName() + ConfigMapServerPostfix`
`100`	`100`	`pod.Spec.Containers[0].Command = []string{"configmap-server", "-c", s.Spec.ConfigMap, "-n", s.GetNamespace()}`
`101`	`101`	`ownerutil.AddOwner(pod, s.CatalogSource, false, false)`
Original file line number	Diff line number	Diff line change
`@@ -204,7 +204,7 @@ func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object {`
`204`	`204`	`if catsrc.Spec.Image != "" {`
`205`	`205`	`decorated := grpcCatalogSourceDecorator{catsrc}`
`206`	`206`	`objs = clientfake.AddSimpleGeneratedNames(`
`207`		`- decorated.Pod(),`
	`207`	`+ decorated.Pod(""),`
`208`	`208`	`decorated.Service(),`
`209`	`209`	`)`
`210`	`210`	`}`
Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@ func NewRegistryReconcilerFactory(lister operatorlister.OperatorLister, opClient`
`91`	`91`	`}`
`92`	`92`	`}`
`93`	`93`
`94`		`-func Pod(source v1alpha1.CatalogSource, name string, image string, labels map[string]string, readinessDelay int32, livenessDelay int32) v1.Pod {`
	`94`	`+func Pod(source v1alpha1.CatalogSource, name string, image string, saName string, labels map[string]string, readinessDelay int32, livenessDelay int32) v1.Pod {`
`95`	`95`	`// Ensure the catalog image is always pulled if the image is not based on a digest, measured by whether an "@" is included.`
`96`	`96`	`// See https://github.com/docker/distribution/blob/master/reference/reference.go for more info.`
`97`	`97`	`// This means recreating non-digest based catalog pods will result in the latest version of the catalog content being delivered on-cluster.`
`@@ -148,6 +148,7 @@ func Pod(source *v1alpha1.CatalogSource, name string, image string, labels map[s`
`148`	`148`	`NodeSelector: map[string]string{`
`149`	`149`	`"kubernetes.io/os": "linux",`
`150`	`150`	`},`
	`151`	`+ ServiceAccountName: saName,`
`151`	`152`	`},`
`152`	`153`	`}`
`153`	`154`	`return pod`