pbacsko commented on code in PR #901:
URL: https://github.com/apache/yunikorn-core/pull/901#discussion_r1665472404
##########
pkg/common/security/usergroup_test.go:
##########
@@ -19,34 +19,79 @@
package security
import (
+ "fmt"
+ "reflect"
"strings"
"testing"
+ "time"
"gotest.tools/v3/assert"
+ "github.com/apache/yunikorn-core/pkg/common"
"github.com/apache/yunikorn-scheduler-interface/lib/go/si"
)
+func (c *UserGroupCache) getUGsize() int {
+ c.lock.RLock()
+ defer c.lock.RUnlock()
+ return len(c.ugs)
+}
+
+func (c *UserGroupCache) getUGGroupSize(user string) int {
+ c.lock.RLock()
+ defer c.lock.RUnlock()
+ ug := c.ugs[user]
+ return len(ug.Groups)
+}
+
+func (c *UserGroupCache) getUGmap() map[string]*UserGroup {
+ c.lock.RLock()
+ defer c.lock.RUnlock()
+ return c.ugs
+}
+
func TestGetUserGroupCache(t *testing.T) {
// get the cache with the test resolver set
testCache := GetUserGroupCache("test")
- if testCache == nil {
- t.Fatal("Cache create failed")
- }
- if len(testCache.ugs) != 0 {
- t.Errorf("Cache not empty: %v", testCache.ugs)
- }
+ assert.Assert(t, testCache != nil, "Cache create failed")
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ testCache.Stop()
+ assert.Assert(t, instance == nil, "instance should be nil")
+ assert.Assert(t, stopped.Load())
+
+ // get the cache with the os resolver set
+ testCache = GetUserGroupCache("os")
+ assert.Assert(t, testCache != nil, "Cache create failed")
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ testCache.Stop()
+ assert.Assert(t, instance == nil, "instance should be nil")
+ assert.Assert(t, stopped.Load())
+
+ // get the cache with the default resolver set
+ testCache = GetUserGroupCache("unknown")
+ assert.Assert(t, testCache != nil, "Cache create failed")
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ testCache.Stop()
+ assert.Assert(t, instance == nil, "instance should be nil")
+ assert.Assert(t, stopped.Load())
+
+ // test for re stop again
+ testCache.Stop()
+ assert.Assert(t, instance == nil, "instance should be nil")
+ assert.Assert(t, stopped.Load())
}
func TestGetUserGroup(t *testing.T) {
testCache := GetUserGroupCache("test")
testCache.resetCache()
// test cache should be empty now
- if len(testCache.ugs) != 0 {
- t.Fatalf("Cache not empty: %v", testCache.ugs)
- }
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
ug, err := testCache.GetUserGroup("testuser1")
+ testCache.lock.Lock()
Review Comment:
This lock is not needed here. I believe it should be moved right before
L#106 when you directly access `testCache.ugs`. `GetUserGroup()` obtains a lock
to read the map and returns a **copy** of the `UserGroup` type, not a pointer.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -64,27 +109,29 @@ func TestGetUserGroup(t *testing.T) {
}
// click over the clock: if we do not get the cached version the new
time will differ from the cache update
cachedUG.resolved -= 5
+ testCache.lock.Unlock()
+
ug, err = testCache.GetUserGroup("testuser1")
+ testCache.lock.Lock()
if err != nil || ug.resolved != cachedUG.resolved {
t.Errorf("User 'testuser1' not returned from Cache, resolution
time differs: %d got %d (err = %v)", ug.resolved, cachedUG.resolved, err)
}
+ testCache.lock.Unlock()
Review Comment:
This lock can be removed. `GetUserGroup()` returns a copy and internally
uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -208,9 +311,11 @@ func TestConvertUGI(t *testing.T) {
if err != nil {
t.Errorf("known user, no groups, convert should not have
failed: %v", err)
}
+ testCache.lock.Lock()
if ug.User != "testuser1" || len(ug.Groups) != 2 || ug.resolved == 0 ||
ug.failed {
t.Errorf("User 'testuser1' not resolved correctly: %v", ug)
}
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `ConvertUGI()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -162,38 +225,78 @@ func TestCacheCleanUp(t *testing.T) {
t.Error("Lookup should not have failed: testuser2 user")
}
+ testCache.lock.Lock()
ug := testCache.ugs["testuser1"]
if ug.failed {
t.Error("User 'testuser1' not resolved as a success")
}
// expire the successful lookup
ug.resolved -= 2 * poscache
+ testCache.lock.Unlock()
// resolve a non existing user
_, err = testCache.GetUserGroup("unknown")
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
+ testCache.lock.Lock()
ug = testCache.ugs["unknown"]
if !ug.failed {
t.Error("User 'unknown' not resolved as a failure")
}
// expire the failed lookup
ug.resolved -= 2 * negcache
+ testCache.lock.Unlock()
testCache.cleanUpCache()
- if len(testCache.ugs) != 1 {
- t.Errorf("Cache not cleaned up : %v", testCache.ugs)
- }
+ assert.Equal(t, 1, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+}
+
+func TestIntervalCacheCleanUp(t *testing.T) {
+ testCache := GetUserGroupCache("test")
+ testCache.resetCache()
+ // test cache should be empty now
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ // resolve an existing user
+ _, err := testCache.GetUserGroup("testuser1")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
+
+ _, err = testCache.GetUserGroup("testuser2")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
+
+ testCache.lock.Lock()
+ ug := testCache.ugs["testuser1"]
+ assert.Assert(t, !ug.failed, "User 'testuser1' not resolved as a
success")
+ testCache.lock.Unlock()
+
+ // expire the successful lookup
+ testCache.lock.Lock()
+ ug.resolved -= 2 * poscache
+
+ testCache.lock.Unlock()
Review Comment:
Resolve the `UserGroup` object for "testuser1" here:
```
// expire the successful lookup
testCache.lock.Lock()
ug := testCache.ugs["testuser1"]
ug.resolved -= 2 * poscache
testCache.lock.Unlock()
```
This way it's cleaner and more obvious what you're modifying.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -93,64 +140,80 @@ func TestBrokenUserGroup(t *testing.T) {
if ug.Groups[0] != "100" {
t.Errorf("User 'testuser2' primary group resolved while it
should not: %v", ug)
}
+ testCache.lock.Unlock()
ug, err = testCache.GetUserGroup("testuser3")
if err != nil {
t.Error("Lookup should not have failed: testuser3")
}
- if len(testCache.ugs) != 2 {
- t.Errorf("Cache not updated should have 2 entries %d",
len(testCache.ugs))
- }
- // check returned info: 4 groups, primary group is duplicate
- if len(ug.Groups) != 4 {
- t.Errorf("User 'testuser3' not resolved correctly: duplicate
primary group not filtered %v", ug)
- }
+
+ assert.Equal(t, 2, testCache.getUGsize(), "Cache not updated should
have 2 entries %d", len(testCache.ugs))
+ assert.Equal(t, 4, testCache.getUGGroupSize("testuser3"), "User
'testuser3' not resolved correctly: duplicate primary group not filtered %v",
ug)
+
+ ug, err = testCache.GetUserGroup("unknown")
+ assert.ErrorContains(t, err, "lookup failed for user: unknown")
+
+ ug, err = testCache.GetUserGroup("testuser4")
+ assert.NilError(t, err)
+
+ ug, err = testCache.GetUserGroup("testuser5")
+ assert.ErrorContains(t, err, "lookup failed for user: testuser5")
+
+ ug, err = testCache.GetUserGroup("invalid-gid-user")
+ testCache.lock.Lock()
+ assert.ErrorContains(t, err, "lookup failed for user: invalid-gid-user")
+ exceptedGroup := []string{"1_001"}
+ assert.Assert(t, reflect.DeepEqual(ug.Groups, exceptedGroup),
fmt.Errorf("group should be: %v, but got: %v", exceptedGroup, ug.Groups))
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `GetUserGroup()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -93,64 +140,80 @@ func TestBrokenUserGroup(t *testing.T) {
if ug.Groups[0] != "100" {
t.Errorf("User 'testuser2' primary group resolved while it
should not: %v", ug)
}
+ testCache.lock.Unlock()
ug, err = testCache.GetUserGroup("testuser3")
if err != nil {
t.Error("Lookup should not have failed: testuser3")
}
- if len(testCache.ugs) != 2 {
- t.Errorf("Cache not updated should have 2 entries %d",
len(testCache.ugs))
- }
- // check returned info: 4 groups, primary group is duplicate
- if len(ug.Groups) != 4 {
- t.Errorf("User 'testuser3' not resolved correctly: duplicate
primary group not filtered %v", ug)
- }
+
+ assert.Equal(t, 2, testCache.getUGsize(), "Cache not updated should
have 2 entries %d", len(testCache.ugs))
+ assert.Equal(t, 4, testCache.getUGGroupSize("testuser3"), "User
'testuser3' not resolved correctly: duplicate primary group not filtered %v",
ug)
+
+ ug, err = testCache.GetUserGroup("unknown")
+ assert.ErrorContains(t, err, "lookup failed for user: unknown")
+
+ ug, err = testCache.GetUserGroup("testuser4")
+ assert.NilError(t, err)
+
+ ug, err = testCache.GetUserGroup("testuser5")
+ assert.ErrorContains(t, err, "lookup failed for user: testuser5")
+
+ ug, err = testCache.GetUserGroup("invalid-gid-user")
+ testCache.lock.Lock()
+ assert.ErrorContains(t, err, "lookup failed for user: invalid-gid-user")
+ exceptedGroup := []string{"1_001"}
+ assert.Assert(t, reflect.DeepEqual(ug.Groups, exceptedGroup),
fmt.Errorf("group should be: %v, but got: %v", exceptedGroup, ug.Groups))
+ testCache.lock.Unlock()
}
func TestGetUserGroupFail(t *testing.T) {
testCache := GetUserGroupCache("test")
testCache.resetCache()
// test cache should be empty now
- if len(testCache.ugs) != 0 {
- t.Fatalf("Cache not empty: %v", testCache.ugs)
- }
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
// resolve an empty user
ug, err := testCache.GetUserGroup("")
+ testCache.lock.Lock()
if err == nil {
t.Error("Lookup should have failed: empty user")
}
// ug is empty everything should be nil..
if ug.User != "" || len(ug.Groups) != 0 || ug.resolved != 0 ||
ug.failed {
t.Errorf("UserGroup is not empty: %v", ug)
}
+ testCache.lock.Unlock()
// resolve a non existing user
ug, err = testCache.GetUserGroup("unknown")
+ testCache.lock.Lock()
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
// ug is partially filled and failed flag is set
if ug.User != "unknown" || len(ug.Groups) != 0 || !ug.failed {
t.Errorf("UserGroup is not empty: %v", ug)
}
+ testCache.lock.Unlock()
+
ug, err = testCache.GetUserGroup("unknown")
+ testCache.lock.Lock()
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
// ug is partially filled and failed flag is set: error message should
show that the cache was returned
if err != nil && !strings.Contains(err.Error(), "cached data returned")
{
t.Errorf("UserGroup not returned from Cache: %v, error: %v",
ug, err)
}
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `GetUserGroup()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -93,64 +140,80 @@ func TestBrokenUserGroup(t *testing.T) {
if ug.Groups[0] != "100" {
t.Errorf("User 'testuser2' primary group resolved while it
should not: %v", ug)
}
+ testCache.lock.Unlock()
ug, err = testCache.GetUserGroup("testuser3")
if err != nil {
t.Error("Lookup should not have failed: testuser3")
}
- if len(testCache.ugs) != 2 {
- t.Errorf("Cache not updated should have 2 entries %d",
len(testCache.ugs))
- }
- // check returned info: 4 groups, primary group is duplicate
- if len(ug.Groups) != 4 {
- t.Errorf("User 'testuser3' not resolved correctly: duplicate
primary group not filtered %v", ug)
- }
+
+ assert.Equal(t, 2, testCache.getUGsize(), "Cache not updated should
have 2 entries %d", len(testCache.ugs))
+ assert.Equal(t, 4, testCache.getUGGroupSize("testuser3"), "User
'testuser3' not resolved correctly: duplicate primary group not filtered %v",
ug)
+
+ ug, err = testCache.GetUserGroup("unknown")
+ assert.ErrorContains(t, err, "lookup failed for user: unknown")
+
+ ug, err = testCache.GetUserGroup("testuser4")
+ assert.NilError(t, err)
+
+ ug, err = testCache.GetUserGroup("testuser5")
+ assert.ErrorContains(t, err, "lookup failed for user: testuser5")
+
+ ug, err = testCache.GetUserGroup("invalid-gid-user")
+ testCache.lock.Lock()
+ assert.ErrorContains(t, err, "lookup failed for user: invalid-gid-user")
+ exceptedGroup := []string{"1_001"}
+ assert.Assert(t, reflect.DeepEqual(ug.Groups, exceptedGroup),
fmt.Errorf("group should be: %v, but got: %v", exceptedGroup, ug.Groups))
+ testCache.lock.Unlock()
}
func TestGetUserGroupFail(t *testing.T) {
testCache := GetUserGroupCache("test")
testCache.resetCache()
// test cache should be empty now
- if len(testCache.ugs) != 0 {
- t.Fatalf("Cache not empty: %v", testCache.ugs)
- }
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
// resolve an empty user
ug, err := testCache.GetUserGroup("")
+ testCache.lock.Lock()
if err == nil {
t.Error("Lookup should have failed: empty user")
}
// ug is empty everything should be nil..
if ug.User != "" || len(ug.Groups) != 0 || ug.resolved != 0 ||
ug.failed {
t.Errorf("UserGroup is not empty: %v", ug)
}
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `GetUserGroup()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -162,38 +225,78 @@ func TestCacheCleanUp(t *testing.T) {
t.Error("Lookup should not have failed: testuser2 user")
}
+ testCache.lock.Lock()
ug := testCache.ugs["testuser1"]
if ug.failed {
t.Error("User 'testuser1' not resolved as a success")
}
// expire the successful lookup
ug.resolved -= 2 * poscache
+ testCache.lock.Unlock()
// resolve a non existing user
_, err = testCache.GetUserGroup("unknown")
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
+ testCache.lock.Lock()
ug = testCache.ugs["unknown"]
if !ug.failed {
t.Error("User 'unknown' not resolved as a failure")
}
// expire the failed lookup
ug.resolved -= 2 * negcache
+ testCache.lock.Unlock()
testCache.cleanUpCache()
- if len(testCache.ugs) != 1 {
- t.Errorf("Cache not cleaned up : %v", testCache.ugs)
- }
+ assert.Equal(t, 1, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+}
+
+func TestIntervalCacheCleanUp(t *testing.T) {
+ testCache := GetUserGroupCache("test")
+ testCache.resetCache()
+ // test cache should be empty now
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ // resolve an existing user
+ _, err := testCache.GetUserGroup("testuser1")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
+
+ _, err = testCache.GetUserGroup("testuser2")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
+
+ testCache.lock.Lock()
+ ug := testCache.ugs["testuser1"]
+ assert.Assert(t, !ug.failed, "User 'testuser1' not resolved as a
success")
+ testCache.lock.Unlock()
+
+ // expire the successful lookup
+ testCache.lock.Lock()
+ ug.resolved -= 2 * poscache
+
+ testCache.lock.Unlock()
+ // resolve a non existing user
+ _, err = testCache.GetUserGroup("unknown")
+ assert.Assert(t, err != nil, "Lookup should have failed: unknown user")
+ testCache.lock.Lock()
+ ug = testCache.ugs["unknown"]
+ assert.Assert(t, ug.failed, "User 'unknown' not resolved as a failure")
+
+ // expire the failed lookup
+ ug.resolved -= 2 * negcache
+ testCache.lock.Unlock()
+
+ // sleep to wait for interval, it will trigger cleanUpCache
+ time.Sleep(testCache.interval + 1*time.Second)
Review Comment:
Nit: just `time.Second` instead of `1*time.Second`.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -64,27 +109,29 @@ func TestGetUserGroup(t *testing.T) {
}
// click over the clock: if we do not get the cached version the new
time will differ from the cache update
cachedUG.resolved -= 5
+ testCache.lock.Unlock()
+
ug, err = testCache.GetUserGroup("testuser1")
+ testCache.lock.Lock()
if err != nil || ug.resolved != cachedUG.resolved {
t.Errorf("User 'testuser1' not returned from Cache, resolution
time differs: %d got %d (err = %v)", ug.resolved, cachedUG.resolved, err)
}
+ testCache.lock.Unlock()
}
func TestBrokenUserGroup(t *testing.T) {
testCache := GetUserGroupCache("test")
testCache.resetCache()
// test cache should be empty now
- if len(testCache.ugs) != 0 {
- t.Fatalf("Cache not empty: %v", testCache.ugs)
- }
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
ug, err := testCache.GetUserGroup("testuser2")
if err != nil {
t.Error("Lookup should not have failed: testuser2")
}
- if len(testCache.ugs) != 1 {
- t.Errorf("Cache not updated should have 1 entry %d",
len(testCache.ugs))
- }
+
+ assert.Equal(t, 1, testCache.getUGsize(), "Cache not updated should
have 1 entry %d", testCache.getUGmap())
+ testCache.lock.Lock()
Review Comment:
This lock/unlock pair can be removed. `GetUserGroup()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -93,64 +140,80 @@ func TestBrokenUserGroup(t *testing.T) {
if ug.Groups[0] != "100" {
t.Errorf("User 'testuser2' primary group resolved while it
should not: %v", ug)
}
+ testCache.lock.Unlock()
ug, err = testCache.GetUserGroup("testuser3")
if err != nil {
t.Error("Lookup should not have failed: testuser3")
}
- if len(testCache.ugs) != 2 {
- t.Errorf("Cache not updated should have 2 entries %d",
len(testCache.ugs))
- }
- // check returned info: 4 groups, primary group is duplicate
- if len(ug.Groups) != 4 {
- t.Errorf("User 'testuser3' not resolved correctly: duplicate
primary group not filtered %v", ug)
- }
+
+ assert.Equal(t, 2, testCache.getUGsize(), "Cache not updated should
have 2 entries %d", len(testCache.ugs))
+ assert.Equal(t, 4, testCache.getUGGroupSize("testuser3"), "User
'testuser3' not resolved correctly: duplicate primary group not filtered %v",
ug)
+
+ ug, err = testCache.GetUserGroup("unknown")
+ assert.ErrorContains(t, err, "lookup failed for user: unknown")
+
+ ug, err = testCache.GetUserGroup("testuser4")
+ assert.NilError(t, err)
+
+ ug, err = testCache.GetUserGroup("testuser5")
+ assert.ErrorContains(t, err, "lookup failed for user: testuser5")
+
+ ug, err = testCache.GetUserGroup("invalid-gid-user")
+ testCache.lock.Lock()
+ assert.ErrorContains(t, err, "lookup failed for user: invalid-gid-user")
+ exceptedGroup := []string{"1_001"}
+ assert.Assert(t, reflect.DeepEqual(ug.Groups, exceptedGroup),
fmt.Errorf("group should be: %v, but got: %v", exceptedGroup, ug.Groups))
+ testCache.lock.Unlock()
}
func TestGetUserGroupFail(t *testing.T) {
testCache := GetUserGroupCache("test")
testCache.resetCache()
// test cache should be empty now
- if len(testCache.ugs) != 0 {
- t.Fatalf("Cache not empty: %v", testCache.ugs)
- }
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
// resolve an empty user
ug, err := testCache.GetUserGroup("")
+ testCache.lock.Lock()
if err == nil {
t.Error("Lookup should have failed: empty user")
}
// ug is empty everything should be nil..
if ug.User != "" || len(ug.Groups) != 0 || ug.resolved != 0 ||
ug.failed {
t.Errorf("UserGroup is not empty: %v", ug)
}
+ testCache.lock.Unlock()
// resolve a non existing user
ug, err = testCache.GetUserGroup("unknown")
+ testCache.lock.Lock()
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
// ug is partially filled and failed flag is set
if ug.User != "unknown" || len(ug.Groups) != 0 || !ug.failed {
t.Errorf("UserGroup is not empty: %v", ug)
}
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `GetUserGroup()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -162,38 +225,78 @@ func TestCacheCleanUp(t *testing.T) {
t.Error("Lookup should not have failed: testuser2 user")
}
+ testCache.lock.Lock()
ug := testCache.ugs["testuser1"]
if ug.failed {
t.Error("User 'testuser1' not resolved as a success")
}
// expire the successful lookup
ug.resolved -= 2 * poscache
+ testCache.lock.Unlock()
// resolve a non existing user
_, err = testCache.GetUserGroup("unknown")
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
+ testCache.lock.Lock()
ug = testCache.ugs["unknown"]
if !ug.failed {
t.Error("User 'unknown' not resolved as a failure")
}
// expire the failed lookup
ug.resolved -= 2 * negcache
+ testCache.lock.Unlock()
testCache.cleanUpCache()
- if len(testCache.ugs) != 1 {
- t.Errorf("Cache not cleaned up : %v", testCache.ugs)
- }
+ assert.Equal(t, 1, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+}
+
+func TestIntervalCacheCleanUp(t *testing.T) {
+ testCache := GetUserGroupCache("test")
+ testCache.resetCache()
+ // test cache should be empty now
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ // resolve an existing user
+ _, err := testCache.GetUserGroup("testuser1")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
+
+ _, err = testCache.GetUserGroup("testuser2")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
+
+ testCache.lock.Lock()
+ ug := testCache.ugs["testuser1"]
+ assert.Assert(t, !ug.failed, "User 'testuser1' not resolved as a
success")
+ testCache.lock.Unlock()
Review Comment:
This block can be removed, see above.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -231,12 +336,14 @@ func TestConvertUGI(t *testing.T) {
if err != nil {
t.Errorf("unknown user with groups, convert should not have
failed: %v", err)
}
+ testCache.lock.Lock()
if ug.User != "unknown2" || len(ug.Groups) != 1 || ug.resolved == 0 ||
ug.failed {
t.Fatalf("User 'unknown2' not resolved correctly: %v", ug)
}
if ug.Groups[0] != group {
t.Errorf("groups not initialised correctly on convert: expected
'%s' got '%s'", group, ug.Groups[0])
}
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `ConvertUGI()` returns a copy and
internally uses a mutex to access the map.
##########
pkg/common/security/usergroup_test.go:
##########
@@ -162,38 +225,78 @@ func TestCacheCleanUp(t *testing.T) {
t.Error("Lookup should not have failed: testuser2 user")
}
+ testCache.lock.Lock()
ug := testCache.ugs["testuser1"]
if ug.failed {
t.Error("User 'testuser1' not resolved as a success")
}
// expire the successful lookup
ug.resolved -= 2 * poscache
+ testCache.lock.Unlock()
// resolve a non existing user
_, err = testCache.GetUserGroup("unknown")
if err == nil {
t.Error("Lookup should have failed: unknown user")
}
+ testCache.lock.Lock()
ug = testCache.ugs["unknown"]
if !ug.failed {
t.Error("User 'unknown' not resolved as a failure")
}
// expire the failed lookup
ug.resolved -= 2 * negcache
+ testCache.lock.Unlock()
testCache.cleanUpCache()
- if len(testCache.ugs) != 1 {
- t.Errorf("Cache not cleaned up : %v", testCache.ugs)
- }
+ assert.Equal(t, 1, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+}
+
+func TestIntervalCacheCleanUp(t *testing.T) {
+ testCache := GetUserGroupCache("test")
+ testCache.resetCache()
+ // test cache should be empty now
+ assert.Equal(t, 0, testCache.getUGsize(), "Cache is not empty: %v",
testCache.getUGmap())
+
+ // resolve an existing user
+ _, err := testCache.GetUserGroup("testuser1")
+ assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
Review Comment:
Small improvement, you can check the `failed` flag here:
```
user1Ug, err := testCache.GetUserGroup("testuser1")
assert.NilError(t, err, "Lookup should not have failed: testuser1 user")
assert.Assert(t, !user1Ug.failed, "User 'testuser1' not resolved as a
success")
```
##########
pkg/common/security/usergroup_test.go:
##########
@@ -251,4 +358,14 @@ func TestConvertUGI(t *testing.T) {
if err == nil {
t.Errorf("invalid username, convert should have failed: %v",
err)
}
+
+ // try unknown user with empty group when forced
+ ugi.User = "unknown"
+ ugi.Groups = []string{}
+ ug, err = testCache.ConvertUGI(ugi, true)
+ exceptedGroup := []string{common.AnonymousGroup}
+ testCache.lock.Lock()
+ assert.Assert(t, reflect.DeepEqual(ug.Groups, exceptedGroup), "group
should be: %v, but got: %v", exceptedGroup, ug.Groups)
+ testCache.lock.Unlock()
Review Comment:
This lock/unlock pair can be removed. `ConvertUGI()` returns a copy and
internally uses a mutex to access the map.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
