From 7e9281fc392947ced4365fe0067cc7dc8b3b5912 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 14 May 2015 13:07:31 -0400 Subject: [PATCH] Allow ServiceAccountsController to manage multiple named service accounts --- .../app/controllermanager.go | 2 +- .../serviceaccounts_controller.go | 69 ++++++--- .../serviceaccounts_controller_test.go | 140 ++++++++++++------ test/integration/service_account_test.go | 2 +- 4 files changed, 140 insertions(+), 73 deletions(-) diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index bfbb333b680..c9a7276f523 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -263,7 +263,7 @@ func (s *CMServer) Run(_ []string) error { serviceaccount.NewServiceAccountsController( kubeClient, - serviceaccount.DefaultServiceAccountControllerOptions(), + serviceaccount.DefaultServiceAccountsControllerOptions(), ).Run() select {} diff --git a/pkg/serviceaccount/serviceaccounts_controller.go b/pkg/serviceaccount/serviceaccounts_controller.go index 1f4e114b14d..8da08d65847 100644 --- a/pkg/serviceaccount/serviceaccounts_controller.go +++ b/pkg/serviceaccount/serviceaccounts_controller.go @@ -28,6 +28,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/fields" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" "github.com/golang/glog" ) @@ -41,24 +42,38 @@ func nameIndexFunc(obj interface{}) (string, error) { return meta.Name(), nil } -type ServiceAccountControllerOptions struct { - Name string +// ServiceAccountsControllerOptions contains options for running a ServiceAccountsController +type ServiceAccountsControllerOptions struct { + // Names is the set of service account names to ensure exist in every namespace + Names util.StringSet + + // ServiceAccountResync is the interval between full resyncs of ServiceAccounts. + // If non-zero, all service accounts will be re-listed this often. + // Otherwise, re-list will be delayed as long as possible (until the watch is closed or times out). ServiceAccountResync time.Duration - NamespaceResync time.Duration + + // NamespaceResync is the interval between full resyncs of Namespaces. + // If non-zero, all namespaces will be re-listed this often. + // Otherwise, re-list will be delayed as long as possible (until the watch is closed or times out). + NamespaceResync time.Duration } -func DefaultServiceAccountControllerOptions() ServiceAccountControllerOptions { - return ServiceAccountControllerOptions{Name: "default"} +func DefaultServiceAccountsControllerOptions() ServiceAccountsControllerOptions { + return ServiceAccountsControllerOptions{Names: util.NewStringSet("default")} } // NewServiceAccountsController returns a new *ServiceAccountsController. -func NewServiceAccountsController(cl *client.Client, options ServiceAccountControllerOptions) *ServiceAccountsController { +func NewServiceAccountsController(cl client.Interface, options ServiceAccountsControllerOptions) *ServiceAccountsController { e := &ServiceAccountsController{ client: cl, - name: options.Name, + names: options.Names, } - accountSelector := fields.SelectorFromSet(map[string]string{client.ObjectNameField: options.Name}) + accountSelector := fields.Everything() + if len(options.Names) == 1 { + // If we're maintaining a single account, we can scope the accounts we watch to just that name + accountSelector = fields.SelectorFromSet(map[string]string{client.ObjectNameField: options.Names.List()[0]}) + } e.serviceAccounts, e.serviceAccountController = framework.NewIndexerInformer( &cache.ListWatch{ ListFunc: func() (runtime.Object, error) { @@ -101,8 +116,8 @@ func NewServiceAccountsController(cl *client.Client, options ServiceAccountContr type ServiceAccountsController struct { stopChan chan struct{} - client *client.Client - name string + client client.Interface + names util.StringSet serviceAccounts cache.Indexer namespaces cache.Indexer @@ -137,27 +152,34 @@ func (e *ServiceAccountsController) serviceAccountDeleted(obj interface{}) { // corresponding secrets will be cleaned up during the Secret re-list return } - e.createDefaultServiceAccountIfNeeded(serviceAccount.Namespace) + // If the deleted service account is one we're maintaining, recreate it + if e.names.Has(serviceAccount.Name) { + e.createServiceAccountIfNeeded(serviceAccount.Name, serviceAccount.Namespace) + } } // namespaceAdded reacts to a Namespace creation by creating a default ServiceAccount object func (e *ServiceAccountsController) namespaceAdded(obj interface{}) { namespace := obj.(*api.Namespace) - e.createDefaultServiceAccountIfNeeded(namespace.Name) + for _, name := range e.names.List() { + e.createServiceAccountIfNeeded(name, namespace.Name) + } } // namespaceUpdated reacts to a Namespace update (or re-list) by creating a default ServiceAccount in the namespace if needed func (e *ServiceAccountsController) namespaceUpdated(oldObj interface{}, newObj interface{}) { newNamespace := newObj.(*api.Namespace) - e.createDefaultServiceAccountIfNeeded(newNamespace.Name) + for _, name := range e.names.List() { + e.createServiceAccountIfNeeded(name, newNamespace.Name) + } } -// createDefaultServiceAccountIfNeeded creates a default ServiceAccount in the given namespace if: -// * it default ServiceAccount does not already exist +// createServiceAccountIfNeeded creates a ServiceAccount with the given name in the given namespace if: +// * the named ServiceAccount does not already exist // * the specified namespace exists // * the specified namespace is in the ACTIVE phase -func (e *ServiceAccountsController) createDefaultServiceAccountIfNeeded(namespace string) { - serviceAccount, err := e.getDefaultServiceAccount(namespace) +func (e *ServiceAccountsController) createServiceAccountIfNeeded(name, namespace string) { + serviceAccount, err := e.getServiceAccount(name, namespace) if err != nil { glog.Error(err) return @@ -181,22 +203,21 @@ func (e *ServiceAccountsController) createDefaultServiceAccountIfNeeded(namespac return } - e.createDefaultServiceAccount(namespace) + e.createServiceAccount(name, namespace) } // createDefaultServiceAccount creates a default ServiceAccount in the specified namespace -func (e *ServiceAccountsController) createDefaultServiceAccount(namespace string) { +func (e *ServiceAccountsController) createServiceAccount(name, namespace string) { serviceAccount := &api.ServiceAccount{} - serviceAccount.Name = e.name + serviceAccount.Name = name serviceAccount.Namespace = namespace if _, err := e.client.ServiceAccounts(namespace).Create(serviceAccount); err != nil { glog.Error(err) } } -// getDefaultServiceAccount returns the default ServiceAccount for the given namespace -func (e *ServiceAccountsController) getDefaultServiceAccount(namespace string) (*api.ServiceAccount, error) { - +// getDefaultServiceAccount returns the ServiceAccount with the given name for the given namespace +func (e *ServiceAccountsController) getServiceAccount(name, namespace string) (*api.ServiceAccount, error) { key := &api.ServiceAccount{ObjectMeta: api.ObjectMeta{Namespace: namespace}} accounts, err := e.serviceAccounts.Index("namespace", key) if err != nil { @@ -205,7 +226,7 @@ func (e *ServiceAccountsController) getDefaultServiceAccount(namespace string) ( for _, obj := range accounts { serviceAccount := obj.(*api.ServiceAccount) - if e.name == serviceAccount.Name { + if name == serviceAccount.Name { return serviceAccount, nil } } diff --git a/pkg/serviceaccount/serviceaccounts_controller_test.go b/pkg/serviceaccount/serviceaccounts_controller_test.go index 4db08bb7e62..4d225eaabc4 100644 --- a/pkg/serviceaccount/serviceaccounts_controller_test.go +++ b/pkg/serviceaccount/serviceaccounts_controller_test.go @@ -23,7 +23,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi" - "github.com/GoogleCloudPlatform/kubernetes/pkg/client" + "github.com/GoogleCloudPlatform/kubernetes/pkg/client/testclient" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) @@ -51,6 +51,9 @@ func makeTestServer(t *testing.T, namespace string, serviceAccountResponse serve func TestServiceAccountCreation(t *testing.T) { ns := api.NamespaceDefault + defaultName := "default" + managedName := "managed" + activeNS := &api.Namespace{ ObjectMeta: api.ObjectMeta{Name: ns}, Status: api.NamespaceStatus{ @@ -63,79 +66,118 @@ func TestServiceAccountCreation(t *testing.T) { Phase: api.NamespaceTerminating, }, } - serviceAccount := &api.ServiceAccount{ + defaultServiceAccount := &api.ServiceAccount{ ObjectMeta: api.ObjectMeta{ - Name: "default", + Name: defaultName, + Namespace: ns, + ResourceVersion: "1", + }, + } + managedServiceAccount := &api.ServiceAccount{ + ObjectMeta: api.ObjectMeta{ + Name: managedName, + Namespace: ns, + ResourceVersion: "1", + }, + } + unmanagedServiceAccount := &api.ServiceAccount{ + ObjectMeta: api.ObjectMeta{ + Name: "other-unmanaged", Namespace: ns, ResourceVersion: "1", }, } testcases := map[string]struct { - ExistingNamespace *api.Namespace - ExistingServiceAccount *api.ServiceAccount + ExistingNamespace *api.Namespace + ExistingServiceAccounts []*api.ServiceAccount AddedNamespace *api.Namespace UpdatedNamespace *api.Namespace DeletedServiceAccount *api.ServiceAccount - ExpectCreatedServiceAccount bool + ExpectCreatedServiceAccounts []string }{ + "new active namespace missing serviceaccounts": { + ExistingServiceAccounts: []*api.ServiceAccount{}, + AddedNamespace: activeNS, + ExpectCreatedServiceAccounts: util.NewStringSet(defaultName, managedName).List(), + }, "new active namespace missing serviceaccount": { - AddedNamespace: activeNS, - ExpectCreatedServiceAccount: true, + ExistingServiceAccounts: []*api.ServiceAccount{managedServiceAccount}, + AddedNamespace: activeNS, + ExpectCreatedServiceAccounts: []string{defaultName}, }, - "new active namespace with serviceaccount": { - ExistingServiceAccount: serviceAccount, - AddedNamespace: activeNS, - ExpectCreatedServiceAccount: false, - }, - "new terminating namespace": { - AddedNamespace: terminatingNS, - ExpectCreatedServiceAccount: false, + "new active namespace with serviceaccounts": { + ExistingServiceAccounts: []*api.ServiceAccount{defaultServiceAccount, managedServiceAccount}, + AddedNamespace: activeNS, + ExpectCreatedServiceAccounts: []string{}, }, - "updated active namespace missing serviceaccount": { - UpdatedNamespace: activeNS, - ExpectCreatedServiceAccount: true, + "new terminating namespace": { + ExistingServiceAccounts: []*api.ServiceAccount{}, + AddedNamespace: terminatingNS, + ExpectCreatedServiceAccounts: []string{}, }, - "updated active namespace with serviceaccount": { - ExistingServiceAccount: serviceAccount, - UpdatedNamespace: activeNS, - ExpectCreatedServiceAccount: false, + + "updated active namespace missing serviceaccounts": { + ExistingServiceAccounts: []*api.ServiceAccount{}, + UpdatedNamespace: activeNS, + ExpectCreatedServiceAccounts: util.NewStringSet(defaultName, managedName).List(), + }, + "updated active namespace missing serviceaccount": { + ExistingServiceAccounts: []*api.ServiceAccount{defaultServiceAccount}, + UpdatedNamespace: activeNS, + ExpectCreatedServiceAccounts: []string{managedName}, + }, + "updated active namespace with serviceaccounts": { + ExistingServiceAccounts: []*api.ServiceAccount{defaultServiceAccount, managedServiceAccount}, + UpdatedNamespace: activeNS, + ExpectCreatedServiceAccounts: []string{}, }, "updated terminating namespace": { - UpdatedNamespace: terminatingNS, - ExpectCreatedServiceAccount: false, + ExistingServiceAccounts: []*api.ServiceAccount{}, + UpdatedNamespace: terminatingNS, + ExpectCreatedServiceAccounts: []string{}, }, "deleted serviceaccount without namespace": { - DeletedServiceAccount: serviceAccount, - ExpectCreatedServiceAccount: false, + DeletedServiceAccount: defaultServiceAccount, + ExpectCreatedServiceAccounts: []string{}, }, "deleted serviceaccount with active namespace": { - ExistingNamespace: activeNS, - DeletedServiceAccount: serviceAccount, - ExpectCreatedServiceAccount: true, + ExistingNamespace: activeNS, + DeletedServiceAccount: defaultServiceAccount, + ExpectCreatedServiceAccounts: []string{defaultName}, }, "deleted serviceaccount with terminating namespace": { - ExistingNamespace: terminatingNS, - DeletedServiceAccount: serviceAccount, - ExpectCreatedServiceAccount: false, + ExistingNamespace: terminatingNS, + DeletedServiceAccount: defaultServiceAccount, + ExpectCreatedServiceAccounts: []string{}, + }, + "deleted unmanaged serviceaccount with active namespace": { + ExistingNamespace: activeNS, + DeletedServiceAccount: unmanagedServiceAccount, + ExpectCreatedServiceAccounts: []string{}, + }, + "deleted unmanaged serviceaccount with terminating namespace": { + ExistingNamespace: terminatingNS, + DeletedServiceAccount: unmanagedServiceAccount, + ExpectCreatedServiceAccounts: []string{}, }, } for k, tc := range testcases { - - testServer, handler := makeTestServer(t, ns, serverResponse{http.StatusOK, serviceAccount}) - client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) - controller := NewServiceAccountsController(client, DefaultServiceAccountControllerOptions()) + client := testclient.NewSimpleFake(defaultServiceAccount, managedServiceAccount) + options := DefaultServiceAccountsControllerOptions() + options.Names = util.NewStringSet(defaultName, managedName) + controller := NewServiceAccountsController(client, options) if tc.ExistingNamespace != nil { controller.namespaces.Add(tc.ExistingNamespace) } - if tc.ExistingServiceAccount != nil { - controller.serviceAccounts.Add(tc.ExistingServiceAccount) + for _, s := range tc.ExistingServiceAccounts { + controller.serviceAccounts.Add(s) } if tc.AddedNamespace != nil { @@ -150,16 +192,20 @@ func TestServiceAccountCreation(t *testing.T) { controller.serviceAccountDeleted(tc.DeletedServiceAccount) } - if tc.ExpectCreatedServiceAccount { - if !handler.ValidateRequestCount(t, 1) { - t.Errorf("%s: Expected a single creation call", k) + if len(tc.ExpectCreatedServiceAccounts) != len(client.Actions) { + t.Errorf("%s: Expected to create accounts %#v. Actual actions were: %#v", k, tc.ExpectCreatedServiceAccounts, client.Actions) + continue + } + for i, expectedName := range tc.ExpectCreatedServiceAccounts { + action := client.Actions[i] + if action.Action != "create-serviceaccount" { + t.Errorf("%s: Unexpected action %s", k, action.Action) + break } - } else { - if !handler.ValidateRequestCount(t, 0) { - t.Errorf("%s: Expected no creation calls", k) + createdAccount := action.Value.(*api.ServiceAccount) + if createdAccount.Name != expectedName { + t.Errorf("%s: Expected %s to be created, got %s", k, expectedName, createdAccount.Name) } } - - testServer.Close() } } diff --git a/test/integration/service_account_test.go b/test/integration/service_account_test.go index 77b8a9d4518..6617ae52392 100644 --- a/test/integration/service_account_test.go +++ b/test/integration/service_account_test.go @@ -423,7 +423,7 @@ func startServiceAccountTestServer(t *testing.T) (*client.Client, client.Config, // Start the service account and service account token controllers tokenController := serviceaccount.NewTokensController(rootClient, serviceaccount.DefaultTokenControllerOptions(serviceaccount.JWTTokenGenerator(serviceAccountKey))) tokenController.Run() - serviceAccountController := serviceaccount.NewServiceAccountsController(rootClient, serviceaccount.DefaultServiceAccountControllerOptions()) + serviceAccountController := serviceaccount.NewServiceAccountsController(rootClient, serviceaccount.DefaultServiceAccountsControllerOptions()) serviceAccountController.Run() // Start the admission plugin reflectors serviceAccountAdmission.Run()