From 9f95cf7b4ffb0c8884e3295398d17aafa8fc756d Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Wed, 5 Apr 2017 12:48:32 -0400 Subject: [PATCH] serviceaccount admission: return correct tokens Fix a bug in serviceaccount admission introduced when we switched everything to use shared informers. That change accidentally reused the list of secrets instead of creating a new one, resulting in all secrets in the namespace being returned as possible service account tokens, instead of limiting it only to the actual service account tokens, as it did before the shared informer conversion. This also adds a unit test to ensure there is no future regression here. --- plugin/pkg/admission/serviceaccount/BUILD | 2 + .../pkg/admission/serviceaccount/admission.go | 12 ++-- .../serviceaccount/admission_test.go | 58 +++++++++++++++++++ 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/plugin/pkg/admission/serviceaccount/BUILD b/plugin/pkg/admission/serviceaccount/BUILD index 704725c4d59..2877fd2d625 100644 --- a/plugin/pkg/admission/serviceaccount/BUILD +++ b/plugin/pkg/admission/serviceaccount/BUILD @@ -43,12 +43,14 @@ go_test( "//pkg/api:go_default_library", "//pkg/client/clientset_generated/internalclientset/fake:go_default_library", "//pkg/client/informers/informers_generated/internalversion:go_default_library", + "//pkg/client/listers/core/internalversion:go_default_library", "//pkg/controller:go_default_library", "//pkg/kubelet/types:go_default_library", "//vendor:k8s.io/apimachinery/pkg/api/errors", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apiserver/pkg/admission", + "//vendor:k8s.io/client-go/tools/cache", ], ) diff --git a/plugin/pkg/admission/serviceaccount/admission.go b/plugin/pkg/admission/serviceaccount/admission.go index 0be8318814f..f45586c7831 100644 --- a/plugin/pkg/admission/serviceaccount/admission.go +++ b/plugin/pkg/admission/serviceaccount/admission.go @@ -287,18 +287,20 @@ func (s *serviceAccount) getReferencedServiceAccountToken(serviceAccount *api.Se // getServiceAccountTokens returns all ServiceAccountToken secrets for the given ServiceAccount func (s *serviceAccount) getServiceAccountTokens(serviceAccount *api.ServiceAccount) ([]*api.Secret, error) { - tokens, err := s.secretLister.Secrets(serviceAccount.Namespace).List(labels.Everything()) + secrets, err := s.secretLister.Secrets(serviceAccount.Namespace).List(labels.Everything()) if err != nil { return nil, err } - for _, token := range tokens { - if token.Type != api.SecretTypeServiceAccountToken { + tokens := []*api.Secret{} + + for _, secret := range secrets { + if secret.Type != api.SecretTypeServiceAccountToken { continue } - if serviceaccount.InternalIsServiceAccountToken(token, serviceAccount) { - tokens = append(tokens, token) + if serviceaccount.InternalIsServiceAccountToken(secret, serviceAccount) { + tokens = append(tokens, secret) } } return tokens, nil diff --git a/plugin/pkg/admission/serviceaccount/admission_test.go b/plugin/pkg/admission/serviceaccount/admission_test.go index be2ff21c12d..f5b86315566 100644 --- a/plugin/pkg/admission/serviceaccount/admission_test.go +++ b/plugin/pkg/admission/serviceaccount/admission_test.go @@ -25,9 +25,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission" + "k8s.io/client-go/tools/cache" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake" informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion" + corelisters "k8s.io/kubernetes/pkg/client/listers/core/internalversion" "k8s.io/kubernetes/pkg/controller" kubelet "k8s.io/kubernetes/pkg/kubelet/types" ) @@ -849,3 +851,59 @@ func TestMultipleReferencedSecrets(t *testing.T) { t.Errorf("expected first referenced secret to be mounted, got %q", name) } } + +func newSecret(secretType api.SecretType, namespace, name, serviceAccountName, serviceAccountUID string) *api.Secret { + return &api.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + Annotations: map[string]string{ + api.ServiceAccountNameKey: serviceAccountName, + api.ServiceAccountUIDKey: serviceAccountUID, + }, + }, + Type: secretType, + } +} + +func TestGetServiceAccountTokens(t *testing.T) { + admit := NewServiceAccount() + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + admit.secretLister = corelisters.NewSecretLister(indexer) + + ns := "namespace" + serviceAccountUID := "12345" + + sa := &api.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultServiceAccountName, + Namespace: ns, + UID: types.UID(serviceAccountUID), + }, + } + + nonSATokenSecret := newSecret(api.SecretTypeDockercfg, ns, "nonSATokenSecret", DefaultServiceAccountName, serviceAccountUID) + indexer.Add(nonSATokenSecret) + + differentSAToken := newSecret(api.SecretTypeServiceAccountToken, ns, "differentSAToken", "someOtherSA", "someOtherUID") + indexer.Add(differentSAToken) + + matchingSAToken := newSecret(api.SecretTypeServiceAccountToken, ns, "matchingSAToken", DefaultServiceAccountName, serviceAccountUID) + indexer.Add(matchingSAToken) + + tokens, err := admit.getServiceAccountTokens(sa) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(tokens) != 1 { + names := make([]string, 0, len(tokens)) + for _, token := range tokens { + names = append(names, token.Name) + } + t.Fatalf("expected only 1 token, got %v", names) + } + if e, a := matchingSAToken.Name, tokens[0].Name; e != a { + t.Errorf("expected token %s, got %s", e, a) + } +}