change bundle resolver to use secret instead of service account

This commit fixes #7159. The bundle resolver's service account doesn't have the permission to fetch the service account which contains the credientials to pull bundle, and the error is also omitted. This commit changes to use secret directly, without granting SA read permissions to resolver. Signed-off-by: Yongxuan Zhang [email protected]
tektoncd · Nov 6, 2023 · e664cba · e664cba
1 parent a791d49
commit e664cba
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 45 deletions.
diff --git a/config/resolvers/bundleresolver-config.yaml b/config/resolvers/bundleresolver-config.yaml
@@ -22,7 +22,5 @@ metadata:
     app.kubernetes.io/instance: default
     app.kubernetes.io/part-of: tekton-pipelines
 data:
-  # the default service account name to use for bundle requests.
-  default-service-account: "default"
   # The default layer kind in the bundle image.
   default-kind: "task"
diff --git a/docs/bundle-resolver.md b/docs/bundle-resolver.md
@@ -15,7 +15,7 @@ This Resolver responds to type `bundles`.
 
 | Param Name       | Description                                                                   | Example Value                                              |
 |------------------|-------------------------------------------------------------------------------|------------------------------------------------------------|
-| `serviceAccount` | The name of the service account to use when constructing registry credentials | `default`                                                  |
+| `secret` | The name of the secret to use when constructing registry credentials | `default`                                                  |
 | `bundle`         | The bundle url pointing at the image to fetch                                 | `gcr.io/tekton-releases/catalog/upstream/golang-build:0.1` |
 | `name`           | The name of the resource to pull out of the bundle                            | `golang-build`                                             |
 | `kind`           | The resource kind to pull out of the bundle                                   | `task`                                                     |
@@ -24,7 +24,7 @@ This Resolver responds to type `bundles`.
 
 - A cluster running Tekton Pipeline v0.41.0 or later.
 - The [built-in remote resolvers installed](./install.md#installing-and-configuring-remote-task-and-pipeline-resolution).
-- The `enable-bundles-resolver` feature flag in the `resolvers-feature-flags` ConfigMap 
+- The `enable-bundles-resolver` feature flag in the `resolvers-feature-flags` ConfigMap
   in the `tekton-pipelines-resolvers` namespace set to `true`.
 - [Beta features](./additional-configs.md#beta-features) enabled.
 
@@ -38,7 +38,6 @@ for the name, namespace and defaults that the resolver ships with.
 
 | Option Name               | Description                                                  | Example Values        |
 |---------------------------|--------------------------------------------------------------|-----------------------|
-| `default-service-account` | The default service account name to use for bundle requests. | `default`, `someuser` |
 | `default-kind`            | The default layer kind in the bundle image.                  | `task`, `pipeline`    |
 
 ## Usage

diff --git a/pkg/resolution/resolver/bundle/bundle.go b/pkg/resolution/resolver/bundle/bundle.go
@@ -37,10 +37,10 @@ const (
 // RequestOptions are the options used to request a resource from
 // a remote bundle.
 type RequestOptions struct {
-	ServiceAccount string
-	Bundle         string
-	EntryName      string
-	Kind           string
+	ImagePullSecret string
+	Bundle          string
+	EntryName       string
+	Kind            string
 }
 
 // ResolvedResource wraps the content of a matched entry in a bundle.

diff --git a/pkg/resolution/resolver/bundle/config.go b/pkg/resolution/resolver/bundle/config.go
@@ -16,9 +16,6 @@ package bundle
 const (
 	// ConfigMapName is the bundle resolver's config map
 	ConfigMapName = "bundleresolver-config"
-	// ConfigServiceAccount is the configuration field name for controlling
-	// the Service Account name to use for bundle requests.
-	ConfigServiceAccount = "default-service-account"
 	// ConfigKind is the configuration field name for controlling
 	// what the layer name in the bundle image is.
 	ConfigKind = "default-kind"

diff --git a/pkg/resolution/resolver/bundle/params.go b/pkg/resolution/resolver/bundle/params.go
@@ -22,9 +22,9 @@ import (
 	"github.com/tektoncd/pipeline/pkg/resolution/resolver/framework"
 )
 
-// ParamServiceAccount is the parameter defining what service
-// account name to use for bundle requests.
-const ParamServiceAccount = "serviceAccount"
+// ParamImagePullSecret is the parameter defining what secret
+// name to use for bundle requests.
+const ParamImagePullSecret = "secret"
 
 // ParamBundle is the parameter defining what the bundle image url is.
 const ParamBundle = "bundle"
@@ -48,18 +48,6 @@ func OptionsFromParams(ctx context.Context, params []pipelinev1.Param) (RequestO
 		paramsMap[p.Name] = p.Value
 	}
 
-	saVal, ok := paramsMap[ParamServiceAccount]
-	sa := ""
-	if !ok || saVal.StringVal == "" {
-		if saString, ok := conf[ConfigServiceAccount]; ok {
-			sa = saString
-		} else {
-			return opts, fmt.Errorf("default Service Account was not set during installation of the bundle resolver")
-		}
-	} else {
-		sa = saVal.StringVal
-	}
-
 	bundleVal, ok := paramsMap[ParamBundle]
 	if !ok || bundleVal.StringVal == "" {
 		return opts, fmt.Errorf("parameter %q required", ParamBundle)
@@ -85,7 +73,7 @@ func OptionsFromParams(ctx context.Context, params []pipelinev1.Param) (RequestO
 		kind = kindVal.StringVal
 	}
 
-	opts.ServiceAccount = sa
+	opts.ImagePullSecret = paramsMap[ParamImagePullSecret].StringVal
 	opts.Bundle = bundleVal.StringVal
 	opts.EntryName = nameVal.StringVal
 	opts.Kind = kind

diff --git a/pkg/resolution/resolver/bundle/resolver.go b/pkg/resolution/resolver/bundle/resolver.go
@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/google/go-containerregistry/pkg/authn/k8schain"
+	kauth "github.com/google/go-containerregistry/pkg/authn/kubernetes"
 	resolverconfig "github.com/tektoncd/pipeline/pkg/apis/config/resolver"
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"github.com/tektoncd/pipeline/pkg/resolution/common"
@@ -93,11 +94,19 @@ func (r *Resolver) Resolve(ctx context.Context, params []pipelinev1.Param) (fram
 	if err != nil {
 		return nil, err
 	}
+	var imagePullSecrets []string
+	if opts.ImagePullSecret != "" {
+		imagePullSecrets = append(imagePullSecrets, opts.ImagePullSecret)
+	}
 	namespace := common.RequestNamespace(ctx)
-	kc, _ := k8schain.New(ctx, r.kubeClientSet, k8schain.Options{
+	kc, err := k8schain.New(ctx, r.kubeClientSet, k8schain.Options{
 		Namespace:          namespace,
-		ServiceAccountName: opts.ServiceAccount,
+		ImagePullSecrets:   imagePullSecrets,
+		ServiceAccountName: kauth.NoServiceAccount,
 	})
+	if err != nil {
+		return nil, err
+	}
 	ctx, cancelFn := context.WithTimeout(ctx, timeoutDuration)
 	defer cancelFn()
 	return GetEntry(ctx, kc, opts)

diff --git a/pkg/resolution/resolver/bundle/resolver_test.go b/pkg/resolution/resolver/bundle/resolver_test.go
@@ -18,6 +18,7 @@ package bundle_test
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http/httptest"
 	"net/url"
@@ -39,8 +40,10 @@ import (
 	"github.com/tektoncd/pipeline/test"
 	"github.com/tektoncd/pipeline/test/diff"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	ktesting "k8s.io/client-go/testing"
 	"knative.dev/pkg/system"
 	_ "knative.dev/pkg/system/testing" // Setup system.Namespace()
 	"sigs.k8s.io/yaml"
@@ -73,7 +76,7 @@ func TestValidateParams(t *testing.T) {
 		Name:  bundle.ParamBundle,
 		Value: *pipelinev1.NewStructuredValues("bar"),
 	}, {
-		Name:  bundle.ParamServiceAccount,
+		Name:  bundle.ParamImagePullSecret,
 		Value: *pipelinev1.NewStructuredValues("baz"),
 	}}
 
@@ -91,7 +94,7 @@ func TestValidateParams(t *testing.T) {
 		Name:  bundle.ParamBundle,
 		Value: *pipelinev1.NewStructuredValues("bar"),
 	}, {
-		Name:  bundle.ParamServiceAccount,
+		Name:  bundle.ParamImagePullSecret,
 		Value: *pipelinev1.NewStructuredValues("baz"),
 	}}
 	if err := resolver.ValidateParams(context.Background(), paramsWithPipeline); err != nil {
@@ -114,7 +117,7 @@ func TestValidateParamsDisabled(t *testing.T) {
 		Name:  bundle.ParamBundle,
 		Value: *pipelinev1.NewStructuredValues("bar"),
 	}, {
-		Name:  bundle.ParamServiceAccount,
+		Name:  bundle.ParamImagePullSecret,
 		Value: *pipelinev1.NewStructuredValues("baz"),
 	}}
 	err = resolver.ValidateParams(resolverDisabledContext(), params)
@@ -139,7 +142,7 @@ func TestValidateParamsMissing(t *testing.T) {
 		Name:  bundle.ParamName,
 		Value: *pipelinev1.NewStructuredValues("foo"),
 	}, {
-		Name:  bundle.ParamServiceAccount,
+		Name:  bundle.ParamImagePullSecret,
 		Value: *pipelinev1.NewStructuredValues("baz"),
 	}}
 	err = resolver.ValidateParams(context.Background(), paramsMissingBundle)
@@ -154,7 +157,7 @@ func TestValidateParamsMissing(t *testing.T) {
 		Name:  bundle.ParamBundle,
 		Value: *pipelinev1.NewStructuredValues("bar"),
 	}, {
-		Name:  bundle.ParamServiceAccount,
+		Name:  bundle.ParamImagePullSecret,
 		Value: *pipelinev1.NewStructuredValues("baz"),
 	}}
 	err = resolver.ValidateParams(context.Background(), paramsMissingName)
@@ -178,7 +181,7 @@ func TestResolveDisabled(t *testing.T) {
 		Name:  bundle.ParamBundle,
 		Value: *pipelinev1.NewStructuredValues("bar"),
 	}, {
-		Name:  bundle.ParamServiceAccount,
+		Name:  bundle.ParamImagePullSecret,
 		Value: *pipelinev1.NewStructuredValues("baz"),
 	}}
 	_, err = resolver.Resolve(resolverDisabledContext(), params)
@@ -191,11 +194,55 @@ func TestResolveDisabled(t *testing.T) {
 	}
 }
 
+func TestResolve_KeyChainError(t *testing.T) {
+	resolver := &bundle.Resolver{}
+	params := &params{
+		bundle: "foo",
+		name:   "example-task",
+		kind:   "task",
+		secret: "bar",
+	}
+
+	ctx, _ := ttesting.SetupFakeContext(t)
+	request := createRequest(params)
+
+	d := test.Data{
+		ResolutionRequests: []*v1beta1.ResolutionRequest{request},
+		ConfigMaps: []*corev1.ConfigMap{{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      bundle.ConfigMapName,
+				Namespace: resolverconfig.ResolversNamespace(system.Namespace()),
+			},
+			Data: map[string]string{
+				bundle.ConfigKind: "task",
+			},
+		}},
+	}
+
+	testAssets, cancel := frtesting.GetResolverFrameworkController(ctx, t, d, resolver)
+	defer cancel()
+
+	expectedErr := apierrors.NewBadRequest("bad request")
+	// return error when getting secrets from kube client
+	testAssets.Clients.Kube.Fake.PrependReactor("get", "secrets", func(action ktesting.Action) (bool, runtime.Object, error) {
+		return true, nil, expectedErr
+	})
+
+	err := testAssets.Controller.Reconciler.Reconcile(testAssets.Ctx, strings.Join([]string{request.Namespace, request.Name}, "/"))
+	if err == nil {
+		t.Fatalf("expected to get error but got nothing")
+	}
+
+	if !errors.Is(err, expectedErr) {
+		t.Fatalf("expected to get error %v, but got %v", expectedErr, err)
+	}
+}
+
 type params struct {
-	serviceAccount string
-	bundle         string
-	name           string
-	kind           string
+	secret string
+	bundle string
+	name   string
+	kind   string
 }
 
 func TestResolve(t *testing.T) {
@@ -397,9 +444,6 @@ func TestResolve(t *testing.T) {
 	resolver := &bundle.Resolver{}
 	confMap := map[string]string{
 		bundle.ConfigKind: "task",
-		// service account is not used in testing, but we have to set this since
-		// param validation will check if the service account is set either from param or from configmap.
-		bundle.ConfigServiceAccount: "placeholder",
 	}
 
 	for _, tc := range testcases {
@@ -491,8 +535,8 @@ func createRequest(p *params) *v1beta1.ResolutionRequest {
 				Name:  bundle.ParamKind,
 				Value: *pipelinev1.NewStructuredValues(p.kind),
 			}, {
-				Name:  bundle.ParamServiceAccount,
-				Value: *pipelinev1.NewStructuredValues(p.serviceAccount),
+				Name:  bundle.ParamImagePullSecret,
+				Value: *pipelinev1.NewStructuredValues(p.secret),
 			}},
 		},
 	}