Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use podTemplate ImagePullSecrets for Entrypoint Image Lookup #4496

Merged
merged 1 commit into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions docs/podtemplates.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,40 @@ Pod templates support fields listed in the table below.
</tbody>
</table>

## Use `imagePullSecrets` to lookup entrypoint

If no command is configured in `task` and `imagePullSecrets` is configured in `podTemplate`, the Tekton Controller will look up the entrypoint of image with `imagePullSecrets`. The Tekton controller's service account is given access to secrets by default. See [this](https://github.com/tektoncd/pipeline/blob/main/config/200-clusterrole.yaml) for reference. If the Tekton controller's service account is not granted the access to secrets in different namespace, you need to grant the access via `RoleBinding`:

```yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: creds-getter
namespace: my-ns
rules:
- apiGroups: [""]
resources: ["secrets"]
resourceNames: ["creds"]
verbs: ["get"]
```

```yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: creds-getter-binding
namespace: my-ns
subjects:
- kind: ServiceAccount
name: tekton-pipelines-controller
namespace: tekton-pipelines
apiGroup: rbac.authorization.k8s.io
roleRef:
kind: Role
name: creds-getter
apiGroup: rbac.authorization.k8s.io
```

---

Except as otherwise noted, the content of this page is licensed under the
Expand Down
6 changes: 3 additions & 3 deletions pkg/pod/entrypoint_lookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type EntrypointCache interface {
// the reference referred to an index, the returned digest will be the
// index's digest, not any platform-specific image contained by the
// index.
get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string) (*imageData, error)
get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string, imagePullSecrets []corev1.LocalObjectReference) (*imageData, error)
}

// imageData contains information looked up about an image or multi-platform image index.
Expand All @@ -50,7 +50,7 @@ type imageData struct {
//
// Images that are not specified by digest will be specified by digest after
// lookup in the resulting list of containers.
func resolveEntrypoints(ctx context.Context, cache EntrypointCache, namespace, serviceAccountName string, steps []corev1.Container) ([]corev1.Container, error) {
func resolveEntrypoints(ctx context.Context, cache EntrypointCache, namespace, serviceAccountName string, imagePullSecrets []corev1.LocalObjectReference, steps []corev1.Container) ([]corev1.Container, error) {
// Keep a local cache of name->imageData lookups, just for the scope of
// resolving this set of steps. If the image is pushed to before the
// next run, we need to resolve its digest and commands again, but we
Expand All @@ -72,7 +72,7 @@ func resolveEntrypoints(ctx context.Context, cache EntrypointCache, namespace, s
id = cid
} else {
// Look it up for real.
lid, err := cache.get(ctx, ref, namespace, serviceAccountName)
lid, err := cache.get(ctx, ref, namespace, serviceAccountName, imagePullSecrets)
if err != nil {
return nil, err
}
Expand Down
8 changes: 7 additions & 1 deletion pkg/pod/entrypoint_lookup_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/google/go-containerregistry/pkg/v1/remote"
lru "github.com/hashicorp/golang-lru"
specs "github.com/opencontainers/image-spec/specs-go/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/client-go/kubernetes"
)

Expand Down Expand Up @@ -56,18 +57,23 @@ func NewEntrypointCache(kubeclient kubernetes.Interface) (EntrypointCache, error
// It also returns the digest associated with the given reference. If the
// reference referred to an index, the returned digest will be the index's
// digest, not any platform-specific image contained by the index.
func (e *entrypointCache) get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string) (*imageData, error) {
func (e *entrypointCache) get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string, imagePullSecrets []corev1.LocalObjectReference) (*imageData, error) {
// If image is specified by digest, check the local cache.
if digest, ok := ref.(name.Digest); ok {
if id, ok := e.lru.Get(digest.String()); ok {
return id.(*imageData), nil
}
}

pullSecretsNames := make([]string, 0, len(imagePullSecrets))
for _, ps := range imagePullSecrets {
pullSecretsNames = append(pullSecretsNames, ps.Name)
}
// Consult the remote registry, using imagePullSecrets.
kc, err := k8schain.New(ctx, e.kubeclient, k8schain.Options{
Namespace: namespace,
ServiceAccountName: serviceAccountName,
ImagePullSecrets: pullSecretsNames,
})
if err != nil {
return nil, fmt.Errorf("error creating k8schain: %v", err)
Expand Down
179 changes: 179 additions & 0 deletions pkg/pod/entrypoint_lookup_impl_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
/*
Copyright 2022 The Tekton Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package pod
Yongxuanzhang marked this conversation as resolved.
Show resolved Hide resolved

import (
"context"
"encoding/base64"
"fmt"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"

"github.com/google/go-containerregistry/pkg/name"
"github.com/google/go-containerregistry/pkg/registry"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
remotetest "github.com/tektoncd/pipeline/test"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
fakeclient "k8s.io/client-go/kubernetes/fake"
)

const (
username = "foo"
password = "bar"
imagePullSecretsName = "secret"
nameSpace = "ns"
)

type fakeHTTP struct {
reg http.Handler
}

func newfakeHTTP() fakeHTTP {
reg := registry.New()
return fakeHTTP{
reg: reg,
}
}

func (f *fakeHTTP) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Request authentication for ping request.
// For further reference see https://docs.docker.com/registry/spec/api/#api-version-check.
if r.URL.Path == "/v2/" && r.Method == http.MethodGet {
w.Header().Add("WWW-Authenticate", "basic")
w.WriteHeader(http.StatusUnauthorized)
return
}
// Check auth if we've fetching the image.
if strings.HasPrefix(r.URL.Path, "/v2/task") && r.Method == "GET" {
u, p, ok := r.BasicAuth()
if !ok || username != u || password != p {
w.WriteHeader(http.StatusUnauthorized)
return
}
}
// Default to open.
f.reg.ServeHTTP(w, r)
}

func generateSecret(host string, username string, password string) *corev1.Secret {
return &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: imagePullSecretsName,
Namespace: nameSpace,
},
Type: corev1.SecretTypeDockercfg,
Data: map[string][]byte{
corev1.DockerConfigKey: []byte(
fmt.Sprintf(`{%q: {"auth": %q}}`,
host,
base64.StdEncoding.EncodeToString([]byte(username+":"+password)),
),
),
},
}
}

func TestGetImageWithImagePullSecrets(t *testing.T) {
ctx := context.Background()
ctx, cancel := context.WithCancel(ctx)
defer cancel()

ftp := newfakeHTTP()
s := httptest.NewServer(&ftp)
defer s.Close()

u, err := url.Parse(s.URL)
if err != nil {
t.Errorf("Parsing url with an error: %v", err)
}

task := &v1beta1.Task{
TypeMeta: metav1.TypeMeta{
APIVersion: "tekton.dev/v1beta1",
Kind: "Task"},
ObjectMeta: metav1.ObjectMeta{
Name: "test-create-image"},
}

ref, err := remotetest.CreateImageWithAnnotations(u.Host+"/task/test-create-image", remotetest.DefaultObjectAnnotationMapper, task)
if err != nil {
t.Errorf("uploading image failed unexpectedly with an error: %v", err)
}

imgRef, err := name.ParseReference(ref)
if err != nil {
t.Errorf("digest %s is not a valid reference: %v", ref, err)
}

for _, tc := range []struct {
name string
basicSecret *corev1.Secret
imagePullSecrets []corev1.LocalObjectReference
wantErr bool
}{{
name: "correct secret",
basicSecret: generateSecret(u.Host, username, password),
imagePullSecrets: []corev1.LocalObjectReference{{Name: imagePullSecretsName}},
wantErr: false,
}, {
name: "unauthorized secret",
basicSecret: generateSecret(u.Host, username, "wrong password"),
imagePullSecrets: []corev1.LocalObjectReference{{Name: imagePullSecretsName}},
wantErr: true,
}, {
name: "empty secret",
basicSecret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
imagePullSecrets: []corev1.LocalObjectReference{{Name: imagePullSecretsName}},
wantErr: true,
}, {
name: "no basic secret",
basicSecret: &corev1.Secret{},
Yongxuanzhang marked this conversation as resolved.
Show resolved Hide resolved
imagePullSecrets: []corev1.LocalObjectReference{{Name: imagePullSecretsName}},
wantErr: true,
}, {
name: "no imagePullSecrets",
basicSecret: generateSecret(u.Host, username, password),
imagePullSecrets: nil,
wantErr: true,
}} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another useful test I thought of - []corev1.LocalObjectReference{badsecret, goodsecret}

e.g. test that we iterate through the chain properly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we expecting err or not for this case? I tried with this case, but k8schain.New() will return err error creating k8schain: secrets "wrong secret" not found. This is a little bit anti my intuition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to know (also a good reason to add tests for these cases)!

I think it's reasonable to error if the secret doesn't exist. But if the bad secret exists but just isn't the right password, then I would assume the next secret would be tried (otherwise there isn't really a reason to provide multiple secrets).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see. Thanks!

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Feb 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • So I tested this and found out that {badsecret, goodsecret} cannot work but {goodsecret,badsecret} works. The direct reason is that only 1 auth is written into the request header and sent to the server. The others are not considered.

After taking a look at the go-containerregistry latest code, I found out that the function use a map to store all the auths, where k is the remote host and value is encoded auth. So for this test case. The 2 secrets have the same host, the first come auth will be stored in keychain and others will be ignored.

Related code see here:
https://github.com/google/go-containerregistry/blob/9c35968ef47ec2e4a14a82ea72167f8655c7c853/pkg/authn/kubernetes/keychain.go#L106-L133

  • I think this probably makes sense, meanwhile I'm wondering if this test case is valid or not. The secret is created from .docker/config.json, the file is written only when user has successfully login. So it is of low possibility that the secret is bad or incorrect. (e.g. user manually manipulates the file later.)

What do you think?

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Feb 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I prefer to keep it and document it in the comments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emmm, it may need more restructuring of the code, I will remove it for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secret is created from .docker/config.json, the file is written only when user has successfully login.

FWIW, this isn't technically true. You can edit that file to contain any username/password, and many tools implement "login" simply be editing that file without validating that the username/password is correct at all.

If there are improvements we can make to that keychain, especially if it helps Tekton users, we should consider them. I'll happily approve :)

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Feb 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this isn't technically true. You can edit that file to contain any username/password, and many tools implement "login" simply be editing that file without validating that the username/password is correct at all.

Oh I see, thank you! I thought the login will do authentication to make sure the auth is valid.

If there are improvements we can make to that keychain, especially if it helps Tekton users, we should consider them. I'll happily approve

So this is an interesting case and I'm not sure which makes more sense (Sorry again I'm still new to this but I'm willing to learn more 😄 ).
From the following code, Say if I have 2 secrets with the same remote registry, but one is incorrect and another is correct. If the bad secret comes first then the correct one will be ignored and not stored in map. Only bad secret will be written into header and sent to server.

This is just what I see from the test case, what I cannot see is what will be the real case, will this {bad secret, correct secret} happen in real use cases? If so, I think some work should be done from the go-containerregistry repo. If not, then I think there's nothing to worry about.

@wlynch @imjasonh

https://github.com/google/go-containerregistry/blob/9c35968ef47ec2e4a14a82ea72167f8655c7c853/pkg/authn/kubernetes/keychain.go#L106

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in that case, today, go-containerregistry will select the first matching secret and use it, not knowing whether it's bad or good. It will only be able to tell whether one is "bad" or "good" by using it to try to auth to the registry, and by that time it's already dropped the old possible alternatives, and will just fail with the "bad" credentials.

This is a bit of a known gap in this keychain logic, and we have some ideas about how to fix it, but it's not really something we see come up much in practice today, so it hasn't been a top priority.

t.Run(tc.name, func(t *testing.T) {
client := fakeclient.NewSimpleClientset(&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "default",
Namespace: nameSpace,
},
}, tc.basicSecret)

entrypointCache, err := NewEntrypointCache(client)
Yongxuanzhang marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Errorf("Creating entrypointCache with an error: %v", err)
}

i, err := entrypointCache.get(ctx, imgRef, nameSpace, "", tc.imagePullSecrets)
if (err != nil) != tc.wantErr {
t.Fatalf("get() = %+v, %v, wantErr %t", i, err, tc.wantErr)
}

})

}

}
4 changes: 2 additions & 2 deletions pkg/pod/entrypoint_lookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestResolveEntrypoints(t *testing.T) {
"reg.io/multi/arch:latest": &data{id: multi},
}

got, err := resolveEntrypoints(ctx, cache, "namespace", "serviceAccountName", []corev1.Container{{
got, err := resolveEntrypoints(ctx, cache, "namespace", "serviceAccountName", []corev1.LocalObjectReference{{Name: "imageSecret"}}, []corev1.Container{{
// This step specifies its command, so there's nothing to
// resolve.
Image: "fully-specified",
Expand Down Expand Up @@ -143,7 +143,7 @@ type data struct {
seen bool // Whether the image has been looked up before.
}

func (f fakeCache) get(ctx context.Context, ref name.Reference, _, _ string) (*imageData, error) {
func (f fakeCache) get(ctx context.Context, ref name.Reference, _, _ string, _ []corev1.LocalObjectReference) (*imageData, error) {
if d, ok := ref.(name.Digest); ok {
if data, found := f[d.String()]; found {
return data.id, nil
Expand Down
16 changes: 8 additions & 8 deletions pkg/pod/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,15 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
initContainers = append(initContainers, *workingDirInit)
}

// By default, use an empty pod template and take the one defined in the task run spec if any
podTemplate := pod.Template{}

if taskRun.Spec.PodTemplate != nil {
podTemplate = *taskRun.Spec.PodTemplate
}

// Resolve entrypoint for any steps that don't specify command.
stepContainers, err = resolveEntrypoints(ctx, b.EntrypointCache, taskRun.Namespace, taskRun.Spec.ServiceAccountName, stepContainers)
stepContainers, err = resolveEntrypoints(ctx, b.EntrypointCache, taskRun.Namespace, taskRun.Spec.ServiceAccountName, podTemplate.ImagePullSecrets, stepContainers)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -261,13 +268,6 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
stepContainers[i].Name = names.SimpleNameGenerator.RestrictLength(StepName(s.Name, i))
}

// By default, use an empty pod template and take the one defined in the task run spec if any
podTemplate := pod.Template{}

if taskRun.Spec.PodTemplate != nil {
podTemplate = *taskRun.Spec.PodTemplate
}

// Add podTemplate Volumes to the explicitly declared use volumes
volumes = append(volumes, taskSpec.Volumes...)
volumes = append(volumes, podTemplate.Volumes...)
Expand Down