pbacsko commented on code in PR #1021:
URL: https://github.com/apache/yunikorn-core/pull/1021#discussion_r2140097673


##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}
+
+// LDAPResolverConfig holds the configuration for the LDAP resolver
+type LdapResolverConfig struct {
+       Host         string
+       Port         int
+       BaseDN       string
+       Filter       string
+       GroupAttr    string
+       ReturnAttr   []string
+       BindUser     string
+       BindPassword string
+       Insecure     bool
+       SSL          bool
+}
+
+// Default values for the LDAP resolver
+var ldapConf = LdapResolverConfig{
+       Host:         common.DefaultLdapHost,
+       Port:         common.DefaultLdapPort,
+       BaseDN:       common.DefaultLdapBaseDN,
+       Filter:       common.DefaultLdapFilter,
+       GroupAttr:    common.DefaultLdapGroupAttr,
+       ReturnAttr:   common.DefaultLdapReturnAttr,
+       BindUser:     common.DefaultLdapBindUser,
+       BindPassword: common.DefaultLdapBindPassword,
+       Insecure:     common.DefaultLdapInsecure,
+       SSL:          common.DefaultLdapSSL,
+}
+
+// read secrets from the secrets directory
+// returns true if at least one secret was loaded and the configuration is 
valid, false otherwise
+var readSecrets = func() bool {
+       secretsDir := common.LdapMountPath
+
+       // Read all files from secrets directory
+       files, err := os.ReadDir(secretsDir)
+       if err != nil {
+               log.Log(log.Security).Error("Unable to access LDAP secrets 
directory",
+                       zap.String("directory", secretsDir),
+                       zap.Error(err))
+               return false
+       }
+
+       secretCount := 0
+       validSecrets := make(map[string]interface{})
+
+       // Iterate over all secret files in the secrets directory
+       for _, file := range files {
+               fileName := file.Name()
+
+               // Skip non-secret entries such as Kubernetes internal metadata 
(e.g., symlinks like "..data" or directories like "..timestamp")
+               if strings.HasPrefix(fileName, "..") || file.IsDir() {
+                       log.Log(log.Security).Info("Ignoring non-secret entry 
(Kubernetes metadata entry or directory)",
+                               zap.String("name", fileName))
+                       continue
+               }
+
+               secretKey := fileName
+               secretValueBytes, err := os.ReadFile(filepath.Join(secretsDir, 
secretKey))
+               if err != nil {
+                       log.Log(log.Security).Warn("Could not read secret file",
+                               zap.String("file", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+               secretValue := strings.TrimSpace(string(secretValueBytes))
+
+               // Validate the secret value
+               validatedValue, err := ValidateSecretValue(secretKey, 
secretValue)
+               if err != nil {
+                       log.Log(log.Security).Warn("Invalid LDAP secret value",
+                               zap.String("key", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+
+               // Store the validated value
+               validSecrets[secretKey] = validatedValue
+               secretCount++
+
+               log.Log(log.Security).Debug("Loaded LDAP secret",
+                       zap.String("key", secretKey))
+       }
+
+       // Apply validated values to the configuration
+       if host, ok := validSecrets[common.LdapHost].(string); ok {
+               ldapConf.Host = host
+       }
+       if port, ok := validSecrets[common.LdapPort].(int); ok {
+               ldapConf.Port = port
+       }
+       if baseDN, ok := validSecrets[common.LdapBaseDN].(string); ok {
+               ldapConf.BaseDN = baseDN
+       }
+       if filter, ok := validSecrets[common.LdapFilter].(string); ok {
+               ldapConf.Filter = filter
+       }
+       if groupAttr, ok := validSecrets[common.LdapGroupAttr].(string); ok {
+               ldapConf.GroupAttr = groupAttr
+       }
+       if returnAttr, ok := validSecrets[common.LdapReturnAttr].([]string); ok 
{
+               ldapConf.ReturnAttr = returnAttr
+       }
+       if bindUser, ok := validSecrets[common.LdapBindUser].(string); ok {
+               ldapConf.BindUser = bindUser
+       }
+       if bindPassword, ok := validSecrets[common.LdapBindPassword].(string); 
ok {
+               ldapConf.BindPassword = bindPassword
+       }
+       if insecure, ok := validSecrets[common.LdapInsecure].(bool); ok {
+               ldapConf.Insecure = insecure
+       }
+       if ssl, ok := validSecrets[common.LdapSSL].(bool); ok {
+               ldapConf.SSL = ssl
+       }
+
+       // Validate the entire configuration
+       validator := NewLdapValidator()
+       isValid := validator.ValidateConfig(&ldapConf)
+
+       // Check if all required fields were provided in the secrets
+       requiredFields := []string{
+               common.LdapHost,
+               common.LdapPort,
+               common.LdapBaseDN,
+               common.LdapFilter,
+               common.LdapGroupAttr,
+               common.LdapReturnAttr,
+               common.LdapBindUser,
+               common.LdapBindPassword,
+       }
+
+       missingFields := []string{}
+       for _, field := range requiredFields {
+               if _, ok := validSecrets[field]; !ok {
+                       missingFields = append(missingFields, field)
+               }
+       }
+
+       if len(missingFields) > 0 {
+               log.Log(log.Security).Error("Missing required LDAP 
configuration fields",
+                       zap.Strings("missingFields", missingFields))
+               isValid = false
+       }
+
+       log.Log(log.Security).Info("Finished loading LDAP secrets",
+               zap.Int("numberOfSecretsLoaded", secretCount),
+               zap.Bool("configurationValid", isValid),
+               zap.Int("missingRequiredFields", len(missingFields)))
+
+       return secretCount > 0 && isValid && len(missingFields) == 0
+}
+
+func GetUserGroupCacheLdap() *UserGroupCache {

Review Comment:
   This function should take two arguments: one for `LdapAccess` and one for 
`SecretReader`. That way, we eliminate global state which can affect unit 
tests. See below for details.



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {

Review Comment:
   See comment below, store the `LdapAccess` as a variable in the struct, so 
such global variables are not needed.



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}
+
+// LDAPResolverConfig holds the configuration for the LDAP resolver
+type LdapResolverConfig struct {
+       Host         string
+       Port         int
+       BaseDN       string
+       Filter       string
+       GroupAttr    string
+       ReturnAttr   []string
+       BindUser     string
+       BindPassword string
+       Insecure     bool
+       SSL          bool
+}
+
+// Default values for the LDAP resolver
+var ldapConf = LdapResolverConfig{
+       Host:         common.DefaultLdapHost,
+       Port:         common.DefaultLdapPort,
+       BaseDN:       common.DefaultLdapBaseDN,
+       Filter:       common.DefaultLdapFilter,
+       GroupAttr:    common.DefaultLdapGroupAttr,
+       ReturnAttr:   common.DefaultLdapReturnAttr,
+       BindUser:     common.DefaultLdapBindUser,
+       BindPassword: common.DefaultLdapBindPassword,
+       Insecure:     common.DefaultLdapInsecure,
+       SSL:          common.DefaultLdapSSL,
+}
+
+// read secrets from the secrets directory
+// returns true if at least one secret was loaded and the configuration is 
valid, false otherwise
+var readSecrets = func() bool {
+       secretsDir := common.LdapMountPath
+
+       // Read all files from secrets directory
+       files, err := os.ReadDir(secretsDir)
+       if err != nil {
+               log.Log(log.Security).Error("Unable to access LDAP secrets 
directory",
+                       zap.String("directory", secretsDir),
+                       zap.Error(err))
+               return false
+       }
+
+       secretCount := 0
+       validSecrets := make(map[string]interface{})
+
+       // Iterate over all secret files in the secrets directory
+       for _, file := range files {
+               fileName := file.Name()
+
+               // Skip non-secret entries such as Kubernetes internal metadata 
(e.g., symlinks like "..data" or directories like "..timestamp")
+               if strings.HasPrefix(fileName, "..") || file.IsDir() {
+                       log.Log(log.Security).Info("Ignoring non-secret entry 
(Kubernetes metadata entry or directory)",
+                               zap.String("name", fileName))
+                       continue
+               }
+
+               secretKey := fileName
+               secretValueBytes, err := os.ReadFile(filepath.Join(secretsDir, 
secretKey))
+               if err != nil {
+                       log.Log(log.Security).Warn("Could not read secret file",
+                               zap.String("file", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+               secretValue := strings.TrimSpace(string(secretValueBytes))
+
+               // Validate the secret value
+               validatedValue, err := ValidateSecretValue(secretKey, 
secretValue)
+               if err != nil {
+                       log.Log(log.Security).Warn("Invalid LDAP secret value",
+                               zap.String("key", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+
+               // Store the validated value
+               validSecrets[secretKey] = validatedValue
+               secretCount++
+
+               log.Log(log.Security).Debug("Loaded LDAP secret",
+                       zap.String("key", secretKey))
+       }
+
+       // Apply validated values to the configuration
+       if host, ok := validSecrets[common.LdapHost].(string); ok {
+               ldapConf.Host = host
+       }
+       if port, ok := validSecrets[common.LdapPort].(int); ok {
+               ldapConf.Port = port
+       }
+       if baseDN, ok := validSecrets[common.LdapBaseDN].(string); ok {
+               ldapConf.BaseDN = baseDN
+       }
+       if filter, ok := validSecrets[common.LdapFilter].(string); ok {
+               ldapConf.Filter = filter
+       }
+       if groupAttr, ok := validSecrets[common.LdapGroupAttr].(string); ok {
+               ldapConf.GroupAttr = groupAttr
+       }
+       if returnAttr, ok := validSecrets[common.LdapReturnAttr].([]string); ok 
{
+               ldapConf.ReturnAttr = returnAttr
+       }
+       if bindUser, ok := validSecrets[common.LdapBindUser].(string); ok {
+               ldapConf.BindUser = bindUser
+       }
+       if bindPassword, ok := validSecrets[common.LdapBindPassword].(string); 
ok {
+               ldapConf.BindPassword = bindPassword
+       }
+       if insecure, ok := validSecrets[common.LdapInsecure].(bool); ok {
+               ldapConf.Insecure = insecure
+       }
+       if ssl, ok := validSecrets[common.LdapSSL].(bool); ok {
+               ldapConf.SSL = ssl
+       }
+
+       // Validate the entire configuration
+       validator := NewLdapValidator()
+       isValid := validator.ValidateConfig(&ldapConf)
+
+       // Check if all required fields were provided in the secrets
+       requiredFields := []string{
+               common.LdapHost,
+               common.LdapPort,
+               common.LdapBaseDN,
+               common.LdapFilter,
+               common.LdapGroupAttr,
+               common.LdapReturnAttr,
+               common.LdapBindUser,
+               common.LdapBindPassword,
+       }
+
+       missingFields := []string{}
+       for _, field := range requiredFields {
+               if _, ok := validSecrets[field]; !ok {
+                       missingFields = append(missingFields, field)
+               }
+       }
+
+       if len(missingFields) > 0 {
+               log.Log(log.Security).Error("Missing required LDAP 
configuration fields",
+                       zap.Strings("missingFields", missingFields))
+               isValid = false
+       }
+
+       log.Log(log.Security).Info("Finished loading LDAP secrets",
+               zap.Int("numberOfSecretsLoaded", secretCount),
+               zap.Bool("configurationValid", isValid),
+               zap.Int("missingRequiredFields", len(missingFields)))
+
+       return secretCount > 0 && isValid && len(missingFields) == 0
+}
+
+func GetUserGroupCacheLdap() *UserGroupCache {
+       secretsLoaded := readSecrets()
+
+       if !secretsLoaded {
+               // Log a FATAL level message - this is very prominent and will 
typically cause the application to exit
+               log.Log(log.Security).Fatal("LDAP configuration not found or 
invalid. No secrets were loaded from the secrets directory.",

Review Comment:
   Do we truly need this to be fatal? I'm wondering if we truly need to be that 
strict. 



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}
+
+// LDAPResolverConfig holds the configuration for the LDAP resolver
+type LdapResolverConfig struct {
+       Host         string
+       Port         int
+       BaseDN       string
+       Filter       string
+       GroupAttr    string
+       ReturnAttr   []string
+       BindUser     string
+       BindPassword string
+       Insecure     bool
+       SSL          bool
+}
+
+// Default values for the LDAP resolver
+var ldapConf = LdapResolverConfig{
+       Host:         common.DefaultLdapHost,
+       Port:         common.DefaultLdapPort,
+       BaseDN:       common.DefaultLdapBaseDN,
+       Filter:       common.DefaultLdapFilter,
+       GroupAttr:    common.DefaultLdapGroupAttr,
+       ReturnAttr:   common.DefaultLdapReturnAttr,
+       BindUser:     common.DefaultLdapBindUser,
+       BindPassword: common.DefaultLdapBindPassword,
+       Insecure:     common.DefaultLdapInsecure,
+       SSL:          common.DefaultLdapSSL,
+}
+
+// read secrets from the secrets directory
+// returns true if at least one secret was loaded and the configuration is 
valid, false otherwise
+var readSecrets = func() bool {
+       secretsDir := common.LdapMountPath
+
+       // Read all files from secrets directory
+       files, err := os.ReadDir(secretsDir)
+       if err != nil {
+               log.Log(log.Security).Error("Unable to access LDAP secrets 
directory",
+                       zap.String("directory", secretsDir),
+                       zap.Error(err))
+               return false
+       }
+
+       secretCount := 0
+       validSecrets := make(map[string]interface{})
+
+       // Iterate over all secret files in the secrets directory
+       for _, file := range files {
+               fileName := file.Name()
+
+               // Skip non-secret entries such as Kubernetes internal metadata 
(e.g., symlinks like "..data" or directories like "..timestamp")
+               if strings.HasPrefix(fileName, "..") || file.IsDir() {
+                       log.Log(log.Security).Info("Ignoring non-secret entry 
(Kubernetes metadata entry or directory)",
+                               zap.String("name", fileName))
+                       continue
+               }
+
+               secretKey := fileName
+               secretValueBytes, err := os.ReadFile(filepath.Join(secretsDir, 
secretKey))
+               if err != nil {
+                       log.Log(log.Security).Warn("Could not read secret file",
+                               zap.String("file", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+               secretValue := strings.TrimSpace(string(secretValueBytes))
+
+               // Validate the secret value
+               validatedValue, err := ValidateSecretValue(secretKey, 
secretValue)
+               if err != nil {
+                       log.Log(log.Security).Warn("Invalid LDAP secret value",
+                               zap.String("key", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+
+               // Store the validated value
+               validSecrets[secretKey] = validatedValue
+               secretCount++
+
+               log.Log(log.Security).Debug("Loaded LDAP secret",
+                       zap.String("key", secretKey))
+       }
+
+       // Apply validated values to the configuration
+       if host, ok := validSecrets[common.LdapHost].(string); ok {
+               ldapConf.Host = host
+       }
+       if port, ok := validSecrets[common.LdapPort].(int); ok {
+               ldapConf.Port = port
+       }
+       if baseDN, ok := validSecrets[common.LdapBaseDN].(string); ok {
+               ldapConf.BaseDN = baseDN
+       }
+       if filter, ok := validSecrets[common.LdapFilter].(string); ok {
+               ldapConf.Filter = filter
+       }
+       if groupAttr, ok := validSecrets[common.LdapGroupAttr].(string); ok {
+               ldapConf.GroupAttr = groupAttr
+       }
+       if returnAttr, ok := validSecrets[common.LdapReturnAttr].([]string); ok 
{
+               ldapConf.ReturnAttr = returnAttr
+       }
+       if bindUser, ok := validSecrets[common.LdapBindUser].(string); ok {
+               ldapConf.BindUser = bindUser
+       }
+       if bindPassword, ok := validSecrets[common.LdapBindPassword].(string); 
ok {
+               ldapConf.BindPassword = bindPassword
+       }
+       if insecure, ok := validSecrets[common.LdapInsecure].(bool); ok {
+               ldapConf.Insecure = insecure
+       }
+       if ssl, ok := validSecrets[common.LdapSSL].(bool); ok {
+               ldapConf.SSL = ssl
+       }
+
+       // Validate the entire configuration
+       validator := NewLdapValidator()
+       isValid := validator.ValidateConfig(&ldapConf)
+
+       // Check if all required fields were provided in the secrets
+       requiredFields := []string{
+               common.LdapHost,
+               common.LdapPort,
+               common.LdapBaseDN,
+               common.LdapFilter,
+               common.LdapGroupAttr,
+               common.LdapReturnAttr,
+               common.LdapBindUser,
+               common.LdapBindPassword,
+       }
+
+       missingFields := []string{}
+       for _, field := range requiredFields {
+               if _, ok := validSecrets[field]; !ok {
+                       missingFields = append(missingFields, field)
+               }
+       }
+
+       if len(missingFields) > 0 {
+               log.Log(log.Security).Error("Missing required LDAP 
configuration fields",
+                       zap.Strings("missingFields", missingFields))
+               isValid = false
+       }
+
+       log.Log(log.Security).Info("Finished loading LDAP secrets",
+               zap.Int("numberOfSecretsLoaded", secretCount),
+               zap.Bool("configurationValid", isValid),
+               zap.Int("missingRequiredFields", len(missingFields)))
+
+       return secretCount > 0 && isValid && len(missingFields) == 0
+}
+
+func GetUserGroupCacheLdap() *UserGroupCache {
+       secretsLoaded := readSecrets()
+
+       if !secretsLoaded {
+               // Log a FATAL level message - this is very prominent and will 
typically cause the application to exit
+               log.Log(log.Security).Fatal("LDAP configuration not found or 
invalid. No secrets were loaded from the secrets directory.",
+                       zap.String("secretsPath", common.LdapMountPath),
+                       zap.String("resolution", "Ensure LDAP secrets are 
properly mounted and accessible"))
+
+               // If the Fatal log doesn't cause an exit (depends on logger 
configuration),
+               // we could also panic here to ensure the application stops
+               panic("LDAP configuration not found or invalid")
+       }
+
+       return &UserGroupCache{
+               ugs:           map[string]*UserGroup{},
+               interval:      cleanerInterval * time.Second,
+               lookup:        LdapLookupUser,
+               lookupGroupID: LdapLookupGroupID,
+               groupIds:      LDAPLookupGroupIds,
+               stop:          make(chan struct{}),
+       }
+}
+
+// Default linux behaviour: a user is member of the primary group with the 
same name
+func LdapLookupUser(userName string) (*user.User, error) {
+       log.Log(log.Security).Debug("Performing LDAP user lookup",
+               zap.String("username", userName),
+               zap.String("defaultUID", common.DefaultLdapUserUID))
+       return &user.User{
+               Uid:      common.DefaultLdapUserUID,
+               Gid:      userName,
+               Username: userName,
+       }, nil
+}
+
+func LdapLookupGroupID(gid string) (*user.Group, error) {
+       log.Log(log.Security).Debug("Looking up LDAP group ID",
+               zap.String("groupID", gid))
+       group := user.Group{Gid: gid}
+       group.Name = gid
+       return &group, nil
+}
+
+func LDAPLookupGroupIds(osUser *user.User) ([]string, error) {

Review Comment:
   nit: LDAP vs Ldap, use consistent capitalization ("Ldap" is preferred)



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}
+
+// LDAPResolverConfig holds the configuration for the LDAP resolver
+type LdapResolverConfig struct {
+       Host         string
+       Port         int
+       BaseDN       string
+       Filter       string
+       GroupAttr    string
+       ReturnAttr   []string
+       BindUser     string
+       BindPassword string
+       Insecure     bool
+       SSL          bool
+}
+
+// Default values for the LDAP resolver
+var ldapConf = LdapResolverConfig{
+       Host:         common.DefaultLdapHost,
+       Port:         common.DefaultLdapPort,
+       BaseDN:       common.DefaultLdapBaseDN,
+       Filter:       common.DefaultLdapFilter,
+       GroupAttr:    common.DefaultLdapGroupAttr,
+       ReturnAttr:   common.DefaultLdapReturnAttr,
+       BindUser:     common.DefaultLdapBindUser,
+       BindPassword: common.DefaultLdapBindPassword,
+       Insecure:     common.DefaultLdapInsecure,
+       SSL:          common.DefaultLdapSSL,
+}
+
+// read secrets from the secrets directory
+// returns true if at least one secret was loaded and the configuration is 
valid, false otherwise
+var readSecrets = func() bool {
+       secretsDir := common.LdapMountPath
+
+       // Read all files from secrets directory
+       files, err := os.ReadDir(secretsDir)
+       if err != nil {
+               log.Log(log.Security).Error("Unable to access LDAP secrets 
directory",
+                       zap.String("directory", secretsDir),
+                       zap.Error(err))
+               return false
+       }
+
+       secretCount := 0
+       validSecrets := make(map[string]interface{})
+
+       // Iterate over all secret files in the secrets directory
+       for _, file := range files {
+               fileName := file.Name()
+
+               // Skip non-secret entries such as Kubernetes internal metadata 
(e.g., symlinks like "..data" or directories like "..timestamp")
+               if strings.HasPrefix(fileName, "..") || file.IsDir() {
+                       log.Log(log.Security).Info("Ignoring non-secret entry 
(Kubernetes metadata entry or directory)",
+                               zap.String("name", fileName))
+                       continue
+               }
+
+               secretKey := fileName
+               secretValueBytes, err := os.ReadFile(filepath.Join(secretsDir, 
secretKey))
+               if err != nil {
+                       log.Log(log.Security).Warn("Could not read secret file",
+                               zap.String("file", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+               secretValue := strings.TrimSpace(string(secretValueBytes))
+
+               // Validate the secret value
+               validatedValue, err := ValidateSecretValue(secretKey, 
secretValue)
+               if err != nil {
+                       log.Log(log.Security).Warn("Invalid LDAP secret value",
+                               zap.String("key", secretKey),
+                               zap.Error(err))
+                       continue
+               }
+
+               // Store the validated value
+               validSecrets[secretKey] = validatedValue
+               secretCount++
+
+               log.Log(log.Security).Debug("Loaded LDAP secret",
+                       zap.String("key", secretKey))
+       }
+
+       // Apply validated values to the configuration
+       if host, ok := validSecrets[common.LdapHost].(string); ok {
+               ldapConf.Host = host
+       }
+       if port, ok := validSecrets[common.LdapPort].(int); ok {
+               ldapConf.Port = port
+       }
+       if baseDN, ok := validSecrets[common.LdapBaseDN].(string); ok {
+               ldapConf.BaseDN = baseDN
+       }
+       if filter, ok := validSecrets[common.LdapFilter].(string); ok {
+               ldapConf.Filter = filter
+       }
+       if groupAttr, ok := validSecrets[common.LdapGroupAttr].(string); ok {
+               ldapConf.GroupAttr = groupAttr
+       }
+       if returnAttr, ok := validSecrets[common.LdapReturnAttr].([]string); ok 
{
+               ldapConf.ReturnAttr = returnAttr
+       }
+       if bindUser, ok := validSecrets[common.LdapBindUser].(string); ok {
+               ldapConf.BindUser = bindUser
+       }
+       if bindPassword, ok := validSecrets[common.LdapBindPassword].(string); 
ok {
+               ldapConf.BindPassword = bindPassword
+       }
+       if insecure, ok := validSecrets[common.LdapInsecure].(bool); ok {
+               ldapConf.Insecure = insecure
+       }
+       if ssl, ok := validSecrets[common.LdapSSL].(bool); ok {
+               ldapConf.SSL = ssl
+       }
+
+       // Validate the entire configuration
+       validator := NewLdapValidator()
+       isValid := validator.ValidateConfig(&ldapConf)
+
+       // Check if all required fields were provided in the secrets
+       requiredFields := []string{
+               common.LdapHost,
+               common.LdapPort,
+               common.LdapBaseDN,
+               common.LdapFilter,
+               common.LdapGroupAttr,
+               common.LdapReturnAttr,
+               common.LdapBindUser,
+               common.LdapBindPassword,
+       }
+
+       missingFields := []string{}
+       for _, field := range requiredFields {
+               if _, ok := validSecrets[field]; !ok {
+                       missingFields = append(missingFields, field)
+               }
+       }
+
+       if len(missingFields) > 0 {
+               log.Log(log.Security).Error("Missing required LDAP 
configuration fields",
+                       zap.Strings("missingFields", missingFields))
+               isValid = false
+       }
+
+       log.Log(log.Security).Info("Finished loading LDAP secrets",
+               zap.Int("numberOfSecretsLoaded", secretCount),
+               zap.Bool("configurationValid", isValid),
+               zap.Int("missingRequiredFields", len(missingFields)))
+
+       return secretCount > 0 && isValid && len(missingFields) == 0
+}
+
+func GetUserGroupCacheLdap() *UserGroupCache {
+       secretsLoaded := readSecrets()
+
+       if !secretsLoaded {
+               // Log a FATAL level message - this is very prominent and will 
typically cause the application to exit
+               log.Log(log.Security).Fatal("LDAP configuration not found or 
invalid. No secrets were loaded from the secrets directory.",
+                       zap.String("secretsPath", common.LdapMountPath),
+                       zap.String("resolution", "Ensure LDAP secrets are 
properly mounted and accessible"))
+
+               // If the Fatal log doesn't cause an exit (depends on logger 
configuration),
+               // we could also panic here to ensure the application stops
+               panic("LDAP configuration not found or invalid")
+       }
+
+       return &UserGroupCache{
+               ugs:           map[string]*UserGroup{},
+               interval:      cleanerInterval * time.Second,
+               lookup:        LdapLookupUser,
+               lookupGroupID: LdapLookupGroupID,
+               groupIds:      LDAPLookupGroupIds,

Review Comment:
   These functions should be part of a type called `LdapLookup`. So this
   ```
   return &UserGroupCache{
                ugs:           map[string]*UserGroup{},
                interval:      cleanerInterval * time.Second,
                lookup:        LdapLookupUser,
                lookupGroupID: LdapLookupGroupID,
                groupIds:      LDAPLookupGroupIds,
                   stop:          make(chan struct{}),
        }
   ```
   becomes
   ```
   ldapLookup: &LdapLookup{secretReader: secretReader, ldapAccess: ldapAccess}
   
   return &UserGroupCache{
                ugs:           map[string]*UserGroup{},
                interval:      cleanerInterval * time.Second,
                lookup:        ldapLookup.LdapLookupUser,
                lookupGroupID: ldapLookup.LdapLookupGroupID,
                groupIds:      ldapLookup.LDAPLookupGroupIds,
                   stop:          make(chan struct{}),
        }
   ```
   
   Which is better because we can store both `secretReader` and `ldapAccess` 
inside `ldapLookup`. That way, there's no need to always create an 
`LdapAccessImpl` every time we call `LDAPLookupGroupIds()`. Also makes unit 
testing completely stateless - there will be no need to setup/teardown stuff, 
defer calls, etc.



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}
+
+// LDAPResolverConfig holds the configuration for the LDAP resolver
+type LdapResolverConfig struct {
+       Host         string
+       Port         int
+       BaseDN       string
+       Filter       string
+       GroupAttr    string
+       ReturnAttr   []string
+       BindUser     string
+       BindPassword string
+       Insecure     bool
+       SSL          bool
+}
+
+// Default values for the LDAP resolver
+var ldapConf = LdapResolverConfig{
+       Host:         common.DefaultLdapHost,
+       Port:         common.DefaultLdapPort,
+       BaseDN:       common.DefaultLdapBaseDN,
+       Filter:       common.DefaultLdapFilter,
+       GroupAttr:    common.DefaultLdapGroupAttr,
+       ReturnAttr:   common.DefaultLdapReturnAttr,
+       BindUser:     common.DefaultLdapBindUser,
+       BindPassword: common.DefaultLdapBindPassword,
+       Insecure:     common.DefaultLdapInsecure,
+       SSL:          common.DefaultLdapSSL,
+}
+
+// read secrets from the secrets directory
+// returns true if at least one secret was loaded and the configuration is 
valid, false otherwise
+var readSecrets = func() bool {

Review Comment:
   Just like in case of `LdapAccess`, I propose an interface for this, eg. 
`SecretReader` with two impementations: `SecretReaderImpl` and 
`SecretReaderMock`.



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}

Review Comment:
   Can be removed if we store `LdapAccess` as struct member.



##########
pkg/common/security/usergroup_test.go:
##########
@@ -27,10 +27,67 @@ import (
 
        "gotest.tools/v3/assert"
 
+       "github.com/go-ldap/ldap/v3"
+
        "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/common/configs"
        "github.com/apache/yunikorn-scheduler-interface/lib/go/si"
 )
 
+// Helper function to set up the mock LDAP implementation for testing
+func setupMockLdap() {
+       // Save the original newLdapAccessImpl function
+       originalLdapAccessImpl := newLdapAccessImpl
+
+       // Replace with mock implementation
+       newLdapAccessImpl = func(config *LdapResolverConfig) LdapAccess {
+               // Use the mockLdapSearchResult function from 
usergroup_ldap_resolver_mock.go
+               return &LdapAccessMock{
+                       SearchFunc: func(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+                               // Extract username from the search filter
+                               username := ""
+                               if searchRequest != nil && searchRequest.Filter 
!= "" {
+                                       // Simple extraction - this assumes the 
filter format is consistent
+                                       parts := 
strings.Split(searchRequest.Filter, "=")
+                                       if len(parts) > 1 {
+                                               username = 
strings.TrimRight(parts[len(parts)-1], ")")
+                                       }
+                               }
+                               return mockLdapSearchResult(username)
+                       },
+               }
+       }
+
+       // Mock readSecrets to return true (successful configuration)
+       originalReadSecrets := readSecrets
+       readSecrets = func() bool {
+               return true
+       }
+
+       // Store the original functions to be restored in teardown
+       originalFunctions["newLdapAccessImpl"] = originalLdapAccessImpl
+       originalFunctions["readSecrets"] = originalReadSecrets
+}
+
+// Helper function to tear down the mock LDAP implementation after testing
+func teardownMockLdap() {
+       // Restore the original functions
+       if originalImpl, ok := originalFunctions["newLdapAccessImpl"]; ok {
+               if factory, ok := originalImpl.(ldapAccessFactory); ok {
+                       newLdapAccessImpl = factory
+               }
+       }
+
+       if originalRead, ok := originalFunctions["readSecrets"]; ok {
+               if readFunc, ok := originalRead.(func() bool); ok {
+                       readSecrets = readFunc
+               }
+       }
+}

Review Comment:
   With the proposed changes I outlined in `usergroup_ldap_resolver.go`, this 
can be all eliminated.



##########
pkg/common/security/usergroup_ldap_resolver.go:
##########
@@ -0,0 +1,375 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package security
+
+import (
+       "crypto/tls"
+       "fmt"
+       "os"
+       "os/user"
+       "path/filepath"
+       "strings"
+       "time"
+
+       "go.uber.org/zap"
+
+       "github.com/go-ldap/ldap/v3"
+
+       "github.com/apache/yunikorn-core/pkg/common"
+       "github.com/apache/yunikorn-core/pkg/log"
+)
+
+// This file contains the implementation of the LDAP resolver for user groups
+
+// LdapAccess defines the interface for LDAP operations
+type LdapAccess interface {
+       // DialURL establishes a connection to the LDAP server
+       DialURL(url string, options ...ldap.DialOpt) (*ldap.Conn, error)
+
+       // Bind authenticates with the LDAP server
+       Bind(conn *ldap.Conn, username, password string) error
+
+       // Search performs an LDAP search operation
+       Search(conn *ldap.Conn, searchRequest *ldap.SearchRequest) 
(*ldap.SearchResult, error)
+
+       // Close closes the LDAP connection
+       Close(conn *ldap.Conn)
+}
+
+// LdapAccessImpl implements the LdapAccess interface with real LDAP operations
+type LdapAccessImpl struct{}
+
+func (l *LdapAccessImpl) DialURL(url string, options ...ldap.DialOpt) 
(*ldap.Conn, error) {
+       return ldap.DialURL(url, options...)
+}
+
+func (l *LdapAccessImpl) Bind(conn *ldap.Conn, username, password string) 
error {
+       return conn.Bind(username, password)
+}
+
+func (l *LdapAccessImpl) Search(conn *ldap.Conn, searchRequest 
*ldap.SearchRequest) (*ldap.SearchResult, error) {
+       return conn.Search(searchRequest)
+}
+
+func (l *LdapAccessImpl) Close(conn *ldap.Conn) {
+       conn.Close()
+}
+
+// ldapAccessFactory is a function type that creates LdapAccess instances
+type ldapAccessFactory func(config *LdapResolverConfig) LdapAccess
+
+// defaultLdapAccessFactory is the default factory function that creates real 
LdapAccessImpl instances
+var defaultLdapAccessFactory ldapAccessFactory = func(config 
*LdapResolverConfig) LdapAccess {
+       return &LdapAccessImpl{}
+}
+
+// newLdapAccessImpl creates a new LdapAccess instance using the current 
factory
+// This can be replaced in tests to return mock implementations
+var newLdapAccessImpl = defaultLdapAccessFactory
+
+// resetLdapAccessFactory resets the factory to the default implementation
+// This is used in tests to ensure the global state is restored
+func resetLdapAccessFactory() {
+       newLdapAccessImpl = defaultLdapAccessFactory
+}
+
+// LDAPResolverConfig holds the configuration for the LDAP resolver
+type LdapResolverConfig struct {
+       Host         string
+       Port         int
+       BaseDN       string
+       Filter       string
+       GroupAttr    string
+       ReturnAttr   []string
+       BindUser     string
+       BindPassword string
+       Insecure     bool
+       SSL          bool

Review Comment:
   nit: "UseSSL" instead of "SSL" to make it less ambiguous



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to