From 7b3eb9f7f8760b82ba903cfe8d8b2c5405a18c29 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Wed, 11 Dec 2024 16:23:28 +0530
Subject: [PATCH] fix: groups lookup performance issue with users with lots of
 groups (#20740)

fixes https://github.com/minio/minio/issues/20717
---
 cmd/iam-store.go | 100 +++++++++++++++++++++++++++++++++++++++++------
 cmd/iam.go       |   7 +++-
 2 files changed, 94 insertions(+), 13 deletions(-)

diff --git a/cmd/iam-store.go b/cmd/iam-store.go
index 943af171b..6d17a7fa7 100644
--- a/cmd/iam-store.go
+++ b/cmd/iam-store.go
@@ -26,6 +26,7 @@ import (
 	"path"
 	"sort"
 	"strings"
+	"sync"
 	"time"
 
 	jsoniter "github.com/json-iterator/go"
@@ -37,6 +38,7 @@ import (
 	"github.com/minio/minio/internal/jwt"
 	"github.com/minio/pkg/v3/env"
 	"github.com/minio/pkg/v3/policy"
+	"github.com/minio/pkg/v3/sync/errgroup"
 	"github.com/puzpuzpuz/xsync/v3"
 	"golang.org/x/sync/singleflight"
 )
@@ -357,6 +359,68 @@ func (c *iamCache) removeGroupFromMembershipsMap(group string) {
 	}
 }
 
+func (c *iamCache) policyDBGetGroups(store *IAMStoreSys, userPolicyPresent bool, groups ...string) ([]string, error) {
+	var policies []string
+	for _, group := range groups {
+		if store.getUsersSysType() == MinIOUsersSysType {
+			g, ok := c.iamGroupsMap[group]
+			if !ok {
+				continue
+			}
+
+			// Group is disabled, so we return no policy - this
+			// ensures the request is denied.
+			if g.Status == statusDisabled {
+				continue
+			}
+		}
+
+		policy, ok := c.iamGroupPolicyMap.Load(group)
+		if !ok {
+			continue
+		}
+
+		policies = append(policies, policy.toSlice()...)
+	}
+
+	found := len(policies) > 0
+	if found {
+		return policies, nil
+	}
+
+	if userPolicyPresent {
+		// if user mapping present and no group policies found
+		// rely on user policy for access, instead of fallback.
+		return nil, nil
+	}
+
+	var mu sync.Mutex
+
+	// no mappings found, fallback for all groups.
+	g := errgroup.WithNErrs(len(groups)).WithConcurrency(10) // load like 10 groups at a time.
+
+	for index := range groups {
+		index := index
+		g.Go(func() error {
+			err := store.loadMappedPolicy(context.TODO(), groups[index], regUser, true, c.iamGroupPolicyMap)
+			if err != nil && !errors.Is(err, errNoSuchPolicy) {
+				return err
+			}
+			if errors.Is(err, errNoSuchPolicy) {
+				return nil
+			}
+			policy, _ := c.iamGroupPolicyMap.Load(groups[index])
+			mu.Lock()
+			policies = append(policies, policy.toSlice()...)
+			mu.Unlock()
+			return nil
+		}, index)
+	}
+
+	err := errors.Join(g.Wait()...)
+	return policies, err
+}
+
 // policyDBGet - lower-level helper; does not take locks.
 //
 // If a group is passed, it returns policies associated with the group.
@@ -676,7 +740,8 @@ func (store *IAMStoreSys) LoadIAMCache(ctx context.Context, firstTime bool) erro
 type IAMStoreSys struct {
 	IAMStorageAPI
 
-	group *singleflight.Group
+	group  *singleflight.Group
+	policy *singleflight.Group
 }
 
 // HasWatcher - returns if the storage system has a watcher.
@@ -757,21 +822,32 @@ func (store *IAMStoreSys) PolicyDBGet(name string, groups ...string) ([]string,
 	cache := store.rlock()
 	defer store.runlock()
 
-	policies, _, err := cache.policyDBGet(store, name, false, false)
-	if err != nil {
-		return nil, err
-	}
-
-	userPolicyPresent := len(policies) > 0
-	for _, group := range groups {
-		ps, _, err := cache.policyDBGet(store, group, true, userPolicyPresent)
+	getPolicies := func() ([]string, error) {
+		policies, _, err := cache.policyDBGet(store, name, false, false)
 		if err != nil {
 			return nil, err
 		}
-		policies = append(policies, ps...)
-	}
 
-	return policies, nil
+		userPolicyPresent := len(policies) > 0
+
+		groupPolicies, err := cache.policyDBGetGroups(store, userPolicyPresent, groups...)
+		if err != nil {
+			return nil, err
+		}
+
+		policies = append(policies, groupPolicies...)
+		return policies, nil
+	}
+	if store.policy != nil {
+		val, err, _ := store.policy.Do(name, func() (interface{}, error) {
+			return getPolicies()
+		})
+		if err != nil {
+			return nil, err
+		}
+		return val.([]string), nil
+	}
+	return getPolicies()
 }
 
 // AddUsersToGroup - adds users to group, creating the group if needed.
diff --git a/cmd/iam.go b/cmd/iam.go
index c809eb929..896460599 100644
--- a/cmd/iam.go
+++ b/cmd/iam.go
@@ -178,13 +178,18 @@ func (sys *IAMSys) initStore(objAPI ObjectLayer, etcdClient *etcd.Client) {
 	}
 
 	if etcdClient == nil {
-		var group *singleflight.Group
+		var (
+			group  *singleflight.Group
+			policy *singleflight.Group
+		)
 		if env.Get("_MINIO_IAM_SINGLE_FLIGHT", config.EnableOn) == config.EnableOn {
 			group = &singleflight.Group{}
+			policy = &singleflight.Group{}
 		}
 		sys.store = &IAMStoreSys{
 			IAMStorageAPI: newIAMObjectStore(objAPI, sys.usersSysType),
 			group:         group,
+			policy:        policy,
 		}
 	} else {
 		sys.store = &IAMStoreSys{IAMStorageAPI: newIAMEtcdStore(etcdClient, sys.usersSysType)}