Reduce cost of admin user check

This commits adds a caching mechanism to the Data object, such that
when large numbers of users exist in the system, the cost of determining
if there is at least one admin user will be low.

To ensure that previously marshalled Data objects contain the correct
cached admin user value, we exhaustively determine if there is an admin
user present whenever we unmarshal a Data object.

CHANGELOG.md

@@ -20,6 +20,7 @@
 - [#7995](https://github.com/influxdata/influxdb/issues/7995): Update liner dependency to handle docker exec.
 - [#7811](https://github.com/influxdata/influxdb/issues/7811): Kill query not killing query
 - [#7457](https://github.com/influxdata/influxdb/issues/7457): KILL QUERY should work during all phases of a query
+- [#8155](https://github.com/influxdata/influxdb/pull/8155): Simplify admin user check.
 
 ## v1.2.2 [2017-03-14]
 

services/httpd/handler.go

@@ -72,8 +72,8 @@ type Handler struct {
 	MetaClient interface {
 		Database(name string) *meta.DatabaseInfo
 		Authenticate(username, password string) (ui *meta.UserInfo, err error)
-		Users() []meta.UserInfo
 		User(username string) (*meta.UserInfo, error)
+		AdminUserExists() bool
 	}
 
 	QueryAuthorizer interface {
@@ -964,20 +964,8 @@ func authenticate(inner func(http.ResponseWriter, *http.Request, *meta.UserInfo)
 		}
 		var user *meta.UserInfo
 
-		// Retrieve user list.
-		uis := h.MetaClient.Users()
-
-		// See if admin user exists.
-		adminExists := false
-		for i := range uis {
-			if uis[i].Admin {
-				adminExists = true
-				break
-			}
-		}
-
 		// TODO corylanou: never allow this in the future without users
-		if requireAuthentication && adminExists {
+		if requireAuthentication && h.MetaClient.AdminUserExists() {
 			creds, err := parseCredentials(r)
 			if err != nil {
 				atomic.AddInt64(&h.stats.AuthenticationFailures, 1)

services/httpd/handler_test.go

@@ -88,16 +88,7 @@ func TestHandler_Query_Auth(t *testing.T) {
 	h := NewHandler(true)
 
 	// Set mock meta client functions for the handler to use.
-	h.MetaClient.UsersFn = func() []meta.UserInfo {
-		return []meta.UserInfo{
-			{
-				Name:       "user1",
-				Hash:       "abcd",
-				Admin:      true,
-				Privileges: make(map[string]influxql.Privilege),
-			},
-		}
-	}
+	h.MetaClient.AdminUserExistsFn = func() bool { return true }
 
 	h.MetaClient.UserFn = func(username string) (*meta.UserInfo, error) {
 		if username != "user1" {
@@ -355,8 +346,10 @@ func TestHandler_Query_ErrAuthorize(t *testing.T) {
 	h.QueryAuthorizer.AuthorizeQueryFn = func(u *meta.UserInfo, q *influxql.Query, db string) error {
 		return errors.New("marker")
 	}
-	h.MetaClient.UsersFn = func() []meta.UserInfo {
-		return []meta.UserInfo{
+	h.MetaClient.AdminUserExistsFn = func() bool { return true }
+	h.MetaClient.AuthenticateFn = func(u, p string) (*meta.UserInfo, error) {
+
+		users := []meta.UserInfo{
 			{
 				Name:  "admin",
 				Hash:  "admin",
@@ -370,9 +363,8 @@ func TestHandler_Query_ErrAuthorize(t *testing.T) {
 				},
 			},
 		}
-	}
-	h.MetaClient.AuthenticateFn = func(u, p string) (*meta.UserInfo, error) {
-		for _, user := range h.MetaClient.Users() {
+
+		for _, user := range users {
 			if u == user.Name {
 				if p == user.Hash {
 					return &user, nil
@@ -637,11 +629,11 @@ func NewHandler(requireAuthentication bool) *Handler {
 
 // HandlerMetaStore is a mock implementation of Handler.MetaClient.
 type HandlerMetaStore struct {
-	PingFn         func(d time.Duration) error
-	DatabaseFn     func(name string) *meta.DatabaseInfo
-	AuthenticateFn func(username, password string) (ui *meta.UserInfo, err error)
-	UsersFn        func() []meta.UserInfo
-	UserFn         func(username string) (*meta.UserInfo, error)
+	PingFn            func(d time.Duration) error
+	DatabaseFn        func(name string) *meta.DatabaseInfo
+	AuthenticateFn    func(username, password string) (ui *meta.UserInfo, err error)
+	UserFn            func(username string) (*meta.UserInfo, error)
+	AdminUserExistsFn func() bool
 }
 
 func (s *HandlerMetaStore) Ping(b bool) error {
@@ -660,8 +652,8 @@ func (s *HandlerMetaStore) Authenticate(username, password string) (ui *meta.Use
 	return s.AuthenticateFn(username, password)
 }
 
-func (s *HandlerMetaStore) Users() []meta.UserInfo {
-	return s.UsersFn()
+func (s *HandlerMetaStore) AdminUserExists() bool {
+	return s.AdminUserExistsFn()
 }
 
 func (s *HandlerMetaStore) User(username string) (*meta.UserInfo, error) {

services/meta/client.go

@@ -547,13 +547,7 @@ func (c *Client) UserPrivilege(username, database string) (*influxql.Privilege,
 func (c *Client) AdminUserExists() bool {
 	c.mu.RLock()
 	defer c.mu.RUnlock()
-
-	for _, u := range c.cacheData.Users {
-		if u.Admin {
-			return true
-		}
-	}
-	return false
+	return c.cacheData.AdminUserExists()
 }
 
 // Authenticate returns a UserInfo if the username and password match an existing entry.

services/meta/data.go

@@ -42,6 +42,10 @@ type Data struct {
 	Databases []DatabaseInfo
 	Users     []UserInfo
 
+	// adminUserExists provides a constant time mechanism for determining
+	// if there is at least one admin user.
+	adminUserExists bool
+
 	MaxShardGroupID uint64
 	MaxShardID      uint64
 }
@@ -568,6 +572,11 @@ func (data *Data) CreateUser(name, hash string, admin bool) error {
 		Admin: admin,
 	})
 
+	// We know there is now at least one admin user.
+	if admin {
+		data.adminUserExists = true
+	}
+
 	return nil
 }
 
@@ -575,10 +584,17 @@ func (data *Data) CreateUser(name, hash string, admin bool) error {
 func (data *Data) DropUser(name string) error {
 	for i := range data.Users {
 		if data.Users[i].Name == name {
+			wasAdmin := data.Users[i].Admin
 			data.Users = append(data.Users[:i], data.Users[i+1:]...)
+
+			// Maybe we dropped the only admin user?
+			if wasAdmin {
+				data.adminUserExists = data.hasAdminUser()
+			}
 			return nil
 		}
 	}
+
 	return ErrUserNotFound
 }
 
@@ -630,9 +646,17 @@ func (data *Data) SetAdminPrivilege(name string, admin bool) error {
 
 	ui.Admin = admin
 
+	// We could have promoted or revoked the only admin. Check if an admin
+	// user exists.
+	data.adminUserExists = data.hasAdminUser()
 	return nil
 }
 
+// AdminUserExists returns true if an admin user exists.
+func (data Data) AdminUserExists() bool {
+	return data.adminUserExists
+}
+
 // UserPrivileges gets the privileges for a user.
 func (data *Data) UserPrivileges(name string) (map[string]influxql.Privilege, error) {
 	ui := data.User(name)
@@ -714,6 +738,10 @@ func (data *Data) unmarshal(pb *internal.Data) {
 	for i, x := range pb.GetUsers() {
 		data.Users[i].unmarshal(x)
 	}
+
+	// Exhaustively determine if there is an admin user. The marshalled cache
+	// value may not be correct.
+	data.adminUserExists = data.hasAdminUser()
 }
 
 // MarshalBinary encodes the metadata to a binary format.
@@ -731,6 +759,17 @@ func (data *Data) UnmarshalBinary(buf []byte) error {
 	return nil
 }
 
+// hasAdminUser exhaustively checks for the presence of at least one admin
+// user.
+func (data *Data) hasAdminUser() bool {
+	for _, u := range data.Users {
+		if u.Admin {
+			return true
+		}
+	}
+	return false
+}
+
 // NodeInfo represents information about a single node in the cluster.
 type NodeInfo struct {
 	ID      uint64

services/meta/data_test.go

@@ -110,6 +110,84 @@ func Test_Data_CreateRetentionPolicy(t *testing.T) {
 	}
 }
 
+func TestData_AdminUserExists(t *testing.T) {
+	data := meta.Data{}
+
+	// No users means no admin.
+	if data.AdminUserExists() {
+		t.Fatal("no admin user should exist")
+	}
+
+	// Add a non-admin user.
+	if err := data.CreateUser("user1", "a", false); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), false; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Add an admin user.
+	if err := data.CreateUser("admin1", "a", true); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), true; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Remove the original user
+	if err := data.DropUser("user1"); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), true; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Add another admin
+	if err := data.CreateUser("admin2", "a", true); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), true; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Revoke privileges of the first admin
+	if err := data.SetAdminPrivilege("admin1", false); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), true; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Add user1 back.
+	if err := data.CreateUser("user1", "a", false); err != nil {
+		t.Fatal(err)
+	}
+	// Revoke remaining admin.
+	if err := data.SetAdminPrivilege("admin2", false); err != nil {
+		t.Fatal(err)
+	}
+	// No longer any admins
+	if got, exp := data.AdminUserExists(), false; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Make user1 an admin
+	if err := data.SetAdminPrivilege("user1", true); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), true; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+
+	// Drop user1...
+	if err := data.DropUser("user1"); err != nil {
+		t.Fatal(err)
+	}
+	if got, exp := data.AdminUserExists(), false; got != exp {
+		t.Fatalf("got %v, expected %v", got, exp)
+	}
+}
+
 func TestUserInfo_AuthorizeDatabase(t *testing.T) {
 	emptyUser := &meta.UserInfo{}
 	if !emptyUser.AuthorizeDatabase(influxql.NoPrivileges, "anydb") {