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

fix(taskrun): emit warning for missing secret in ServiceAccount instead of failing #7761

Merged
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
24 changes: 23 additions & 1 deletion pkg/pod/creds_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"regexp"
"strings"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -29,8 +30,12 @@ import (
"github.com/tektoncd/pipeline/pkg/credentials/gitcreds"
"github.com/tektoncd/pipeline/pkg/names"
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes"
"knative.dev/pkg/controller"
"knative.dev/pkg/logging"
)

const (
Expand All @@ -51,7 +56,8 @@ var dnsLabel1123Forbidden = regexp.MustCompile("[^a-zA-Z0-9-]+")
// Any errors encountered during this process are returned to the
// caller. If no matching annotated secrets are found, nil lists with a
// nil error are returned.
func credsInit(ctx context.Context, serviceAccountName, namespace string, kubeclient kubernetes.Interface) ([]string, []corev1.Volume, []corev1.VolumeMount, error) {
func credsInit(ctx context.Context, obj runtime.Object, serviceAccountName, namespace string, kubeclient kubernetes.Interface) ([]string, []corev1.Volume, []corev1.VolumeMount, error) {
Copy link
Member

Choose a reason for hiding this comment

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

The serviceAccountName could not be obtained from the runtime.Object by casting it to a task, but this approach keeps the code simpler the che change smaller, so I'm fine with it.

logger := logging.FromContext(ctx)
cfg := config.FromContextOrDefaults(ctx)
if cfg != nil && cfg.FeatureFlags != nil && cfg.FeatureFlags.DisableCredsInit {
return nil, nil, nil, nil
Expand All @@ -73,6 +79,17 @@ func credsInit(ctx context.Context, serviceAccountName, namespace string, kubecl
var volumeMounts []corev1.VolumeMount
var volumes []corev1.Volume
var args []string
var missingSecrets []string

defer func() {
recorder := controller.GetEventRecorder(ctx)
if len(missingSecrets) > 0 && recorder != nil && obj != nil {
recorder.Eventf(obj, corev1.EventTypeWarning, "FailedToRetrieveSecret",
"Unable to retrieve some secrets (%s); attempting to use them may not succeed.",
strings.Join(missingSecrets, ", "))
}
}()

// Track duplicated secrets, prevent errors like this:
// Pod "xxx" is invalid: spec.containers[0].volumeMounts[12].mountPath: Invalid value:
// "/tekton/creds-secrets/demo-docker-credentials": must be unique
Expand All @@ -87,6 +104,11 @@ func credsInit(ctx context.Context, serviceAccountName, namespace string, kubecl
visitedSecrets[secretEntry.Name] = struct{}{}

secret, err := kubeclient.CoreV1().Secrets(namespace).Get(ctx, secretEntry.Name, metav1.GetOptions{})
if k8serrors.IsNotFound(err) {
missingSecrets = append(missingSecrets, secretEntry.Name)
logger.Warnf("Secret %q in ServiceAccount %s/%s not found, skipping", secretEntry.Name, namespace, serviceAccountName)
continue
}
if err != nil {
return nil, nil, nil, err
}
Expand Down
55 changes: 54 additions & 1 deletion pkg/pod/creds_init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
fakek8s "k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/tools/record"
"knative.dev/pkg/controller"
logtesting "knative.dev/pkg/logging/testing"
"knative.dev/pkg/system"
)
Expand All @@ -49,6 +51,7 @@ func TestCredsInit(t *testing.T) {
wantVolumeMounts []corev1.VolumeMount
objs []runtime.Object
envVars []corev1.EnvVar
events []string
ctx context.Context
}{{
desc: "service account exists with no secrets; nothing to initialize",
Expand Down Expand Up @@ -288,11 +291,52 @@ func TestCredsInit(t *testing.T) {
MountPath: "/tekton/creds-secrets/my-creds",
}},
ctx: context.Background(),
}, {
desc: "service account has not found secrets",
objs: []runtime.Object{
&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{Name: serviceAccountName, Namespace: namespace},
Secrets: []corev1.ObjectReference{
{Name: "my-creds"},
{Name: "not-exist-1"},
{Name: "not-exist-2"},
},
},
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "my-creds",
Namespace: namespace,
Annotations: map[string]string{
"tekton.dev/git-0": "github.com",
},
},
Type: "kubernetes.io/basic-auth",
Data: map[string][]byte{
"username": []byte("foo"),
"password": []byte("BestEver"),
},
},
},
envVars: []corev1.EnvVar{},
wantArgs: []string{
"-basic-git=my-creds=github.com",
},
wantVolumeMounts: []corev1.VolumeMount{{
Name: "tekton-internal-secret-volume-my-creds-9l9zj",
MountPath: "/tekton/creds-secrets/my-creds",
}},
events: []string{
`Warning FailedToRetrieveSecret Unable to retrieve some secrets (not-exist-1, not-exist-2); attempting to use them may not succeed.`,
},
ctx: context.Background(),
}} {
t.Run(c.desc, func(t *testing.T) {
names.TestingSeed()
eventObj := &corev1.Event{}
kubeclient := fakek8s.NewSimpleClientset(c.objs...)
args, volumes, volumeMounts, err := credsInit(c.ctx, serviceAccountName, namespace, kubeclient)
recorder := record.NewFakeRecorder(1000)
c.ctx = controller.WithEventRecorder(c.ctx, recorder)
args, volumes, volumeMounts, err := credsInit(c.ctx, eventObj, serviceAccountName, namespace, kubeclient)
if err != nil {
t.Fatalf("credsInit: %v", err)
}
Expand All @@ -305,6 +349,15 @@ func TestCredsInit(t *testing.T) {
if d := cmp.Diff(c.wantVolumeMounts, volumeMounts); d != "" {
t.Fatalf("Diff %s", diff.PrintWantGot(d))
}
if len(recorder.Events) != len(c.events) {
t.Fatalf("Expected %d events, got %d", len(c.events), len(recorder.Events))
}
for i, e := range c.events {
message := <-recorder.Events
if message != e {
t.Fatalf("Expected event %d to be %q, got %q", i, e, message)
}
}
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/pod/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1.TaskRun, taskSpec v1.Ta
if config.IsSpireEnabled(ctx) {
commonExtraEntrypointArgs = append(commonExtraEntrypointArgs, "-enable_spire")
}
credEntrypointArgs, credVolumes, credVolumeMounts, err := credsInit(ctx, taskRun.Spec.ServiceAccountName, taskRun.Namespace, b.KubeClient)
credEntrypointArgs, credVolumes, credVolumeMounts, err := credsInit(ctx, taskRun, taskRun.Spec.ServiceAccountName, taskRun.Namespace, b.KubeClient)
if err != nil {
return nil, err
}
Expand Down