You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2023/01/02 07:16:45 UTC
[GitHub] [yunikorn-core] 0yukali0 opened a new pull request, #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
0yukali0 opened a new pull request, #492:
URL: https://github.com/apache/yunikorn-core/pull/492
### What is this PR for?
Current ACL unit tests do not adopt table-driven style, so I rewrite the ACL unit test file.
### What type of PR is it?
* [ ] - Bug Fix
* [x] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1446
### How should this be tested?
make test in the core rep
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] 0yukali0 commented on a diff in pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #492:
URL: https://github.com/apache/yunikorn-core/pull/492#discussion_r1060276119
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
}
+
+ if expected.groups != nil && got.groups != nil {
+ for groupname, expectedAllow := range expected.groups {
+ if gotAllow, ok := got.groups[groupname]; !ok {
+ return fmt.Errorf("groupname %s does not exist", groupname)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("groupname %s is not same", groupname)
+ }
+ }
+ } else if (expected.groups != nil && got.groups == nil) || (expected.groups == nil && got.groups != nil) {
+ return errors.New("groups of an acl is not nil, expect one and got one should be same")
+ }
+ return nil
}
-func TestACLSpecialCase(t *testing.T) {
- acl, err := NewACL(" ")
- if err == nil {
- t.Errorf("parsing passed for string: ' '")
- }
+func TestACLCreate(t *testing.T) {
+ tests := []struct {
+ input string
+ expected ACL
+ }{
+ {
+ "",
+ ACL{allAllowed: false}},
+ {
+ " ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1",
+ ACL{users: map[string]bool{"user1": true}, allAllowed: false}},
+ {
+ "user1,user2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, allAllowed: false}},
+ {
+ "user1,user2 ",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1,user2 group1",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true}, allAllowed: false},
+ },
+ {
+ "user1,user2 group1,group2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "user2 group1,group2",
+ ACL{users: map[string]bool{"user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ " group1,group2",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "* group1,group2",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "user1,user2 *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "*",
+ ACL{users: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "* ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ " *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "dotted.user",
+ ACL{users: map[string]bool{"dotted.user": true}, allAllowed: false},
+ },
+ {
+ "user,user",
+ ACL{users: map[string]bool{"user": true}, allAllowed: false},
+ },
+ {
+ " dotted.group",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false},
+ },
+ {
+ " group,group",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group": true}, allAllowed: false},
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.input, func(t *testing.T) {
+ got, err := NewACL(tt.input)
+ if err != nil {
+ t.Errorf("parsing failed for string: %s", tt.input)
+ }
- acl, err = NewACL("dotted.user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'dotted.user' acl has incorrect user list: %v", acl)
- }
- acl, err = NewACL("user,user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'user,user' acl has incorrect user list: %v", acl)
+ if err = IsSameACL(got, tt.expected); err != nil {
+ t.Error(err.Error())
+ }
+ })
}
- acl, err = NewACL(" dotted.group")
- if err != nil || len(acl.groups) > 0 {
- t.Errorf("parsing failed for string: ' dotted.group' acl has incorrect group list: %v", acl)
- }
- acl, err = NewACL(" group,group")
- if err != nil || len(acl.groups) != 1 {
- t.Errorf("parsing failed for string: 'group,group' acl has incorrect group list: %v", acl)
+}
+
+func TestNewACLErrorCase(t *testing.T) {
+ tests := []struct {
+ caseName string
+ acl string
+ }{
+ {"spaces", " "},
+ {"number of spaces is higher than 1 is not allowed", " a b "},
+ }
+ for _, tt := range tests {
+ t.Run(tt.caseName, func(t *testing.T) {
+ if _, err := NewACL(tt.acl); err == nil {
+ t.Errorf("parsing %s string should be failed", tt.acl)
+ }
+ })
}
}
func TestACLAccess(t *testing.T) {
- acl, err := NewACL("user1,user2 group1,group2")
- assert.NilError(t, err, "parsing failed for string: 'user1,user2 group1,group2'")
- user := UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, should have been denied")
- user = UserGroup{User: "user1", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "user1 (no groups) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3", "group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 (2nd group) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3"}}
- assert.Assert(t, !acl.CheckAccess(user), "user3/group3 should have been denied")
-
- acl, err = NewACL("*")
- assert.NilError(t, err, "parsing failed for string: '*'")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "no user, wildcard should have been allowed")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "user1/group1, wildcard should have been allowed")
-
- acl, err = NewACL("")
- assert.NilError(t, err, "parsing failed for string: ''")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, empty ACL always deny")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, !acl.CheckAccess(user), "user1/group1, empty ACL always deny")
+ tests := []struct {
+ acl string
+ visitor UserGroup
+ expected bool
+ }{
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "", Groups: nil},
+ false,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user1", Groups: nil},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3", "group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3"}},
+ false,
+ },
+ {
+ "*",
+ UserGroup{User: "", Groups: nil},
+ true,
+ },
+ {
+ "*",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ acl: "",
+ visitor: UserGroup{User: "", Groups: nil},
+ expected: false,
+ },
+ {
+ "",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ false,
+ },
+ }
+ for _, tt := range tests {
+ t.Run(fmt.Sprintf("vistor %v, acl %s", tt.visitor, tt.acl), func(t *testing.T) {
+ acl, err := NewACL(tt.acl)
+ if err != nil {
+ t.Error(err.Error())
Review Comment:
Oops, I forgot that lint would request developer to handle the err in the UT.
So I updated the message of newACL error in the latest commit.
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #492:
URL: https://github.com/apache/yunikorn-core/pull/492#discussion_r1060226609
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
}
+
+ if expected.groups != nil && got.groups != nil {
Review Comment:
Same as for users
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
Review Comment:
You are not catching the case that `got` has more users than `expected`
Simplify and change check ordering:
1. check if length of both maps are equal: if not return
2. check equality of the members: let the range handle nil maps.
3. Combine the user not found and the allow flag difference in one check:
```
gotAllow, ok := got.users[username]; !ok || gotAllow != expectedAllow
```
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
}
+
+ if expected.groups != nil && got.groups != nil {
+ for groupname, expectedAllow := range expected.groups {
+ if gotAllow, ok := got.groups[groupname]; !ok {
+ return fmt.Errorf("groupname %s does not exist", groupname)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("groupname %s is not same", groupname)
+ }
+ }
+ } else if (expected.groups != nil && got.groups == nil) || (expected.groups == nil && got.groups != nil) {
+ return errors.New("groups of an acl is not nil, expect one and got one should be same")
+ }
+ return nil
}
-func TestACLSpecialCase(t *testing.T) {
- acl, err := NewACL(" ")
- if err == nil {
- t.Errorf("parsing passed for string: ' '")
- }
+func TestACLCreate(t *testing.T) {
+ tests := []struct {
+ input string
+ expected ACL
+ }{
+ {
+ "",
+ ACL{allAllowed: false}},
+ {
+ " ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1",
+ ACL{users: map[string]bool{"user1": true}, allAllowed: false}},
+ {
+ "user1,user2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, allAllowed: false}},
+ {
+ "user1,user2 ",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1,user2 group1",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true}, allAllowed: false},
+ },
+ {
+ "user1,user2 group1,group2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "user2 group1,group2",
+ ACL{users: map[string]bool{"user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ " group1,group2",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "* group1,group2",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "user1,user2 *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "*",
+ ACL{users: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "* ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ " *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "dotted.user",
+ ACL{users: map[string]bool{"dotted.user": true}, allAllowed: false},
+ },
+ {
+ "user,user",
+ ACL{users: map[string]bool{"user": true}, allAllowed: false},
+ },
+ {
+ " dotted.group",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false},
+ },
+ {
+ " group,group",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group": true}, allAllowed: false},
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.input, func(t *testing.T) {
+ got, err := NewACL(tt.input)
+ if err != nil {
+ t.Errorf("parsing failed for string: %s", tt.input)
+ }
- acl, err = NewACL("dotted.user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'dotted.user' acl has incorrect user list: %v", acl)
- }
- acl, err = NewACL("user,user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'user,user' acl has incorrect user list: %v", acl)
+ if err = IsSameACL(got, tt.expected); err != nil {
+ t.Error(err.Error())
+ }
+ })
}
- acl, err = NewACL(" dotted.group")
- if err != nil || len(acl.groups) > 0 {
- t.Errorf("parsing failed for string: ' dotted.group' acl has incorrect group list: %v", acl)
- }
- acl, err = NewACL(" group,group")
- if err != nil || len(acl.groups) != 1 {
- t.Errorf("parsing failed for string: 'group,group' acl has incorrect group list: %v", acl)
+}
+
+func TestNewACLErrorCase(t *testing.T) {
+ tests := []struct {
+ caseName string
+ acl string
+ }{
+ {"spaces", " "},
+ {"number of spaces is higher than 1 is not allowed", " a b "},
+ }
+ for _, tt := range tests {
+ t.Run(tt.caseName, func(t *testing.T) {
+ if _, err := NewACL(tt.acl); err == nil {
+ t.Errorf("parsing %s string should be failed", tt.acl)
+ }
+ })
}
}
func TestACLAccess(t *testing.T) {
- acl, err := NewACL("user1,user2 group1,group2")
- assert.NilError(t, err, "parsing failed for string: 'user1,user2 group1,group2'")
- user := UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, should have been denied")
- user = UserGroup{User: "user1", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "user1 (no groups) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3", "group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 (2nd group) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3"}}
- assert.Assert(t, !acl.CheckAccess(user), "user3/group3 should have been denied")
-
- acl, err = NewACL("*")
- assert.NilError(t, err, "parsing failed for string: '*'")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "no user, wildcard should have been allowed")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "user1/group1, wildcard should have been allowed")
-
- acl, err = NewACL("")
- assert.NilError(t, err, "parsing failed for string: ''")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, empty ACL always deny")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, !acl.CheckAccess(user), "user1/group1, empty ACL always deny")
+ tests := []struct {
+ acl string
+ visitor UserGroup
+ expected bool
+ }{
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "", Groups: nil},
+ false,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user1", Groups: nil},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3", "group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3"}},
+ false,
+ },
+ {
+ "*",
+ UserGroup{User: "", Groups: nil},
+ true,
+ },
+ {
+ "*",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ acl: "",
+ visitor: UserGroup{User: "", Groups: nil},
+ expected: false,
Review Comment:
nit: remove struct field names
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
}
+
+ if expected.groups != nil && got.groups != nil {
+ for groupname, expectedAllow := range expected.groups {
+ if gotAllow, ok := got.groups[groupname]; !ok {
+ return fmt.Errorf("groupname %s does not exist", groupname)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("groupname %s is not same", groupname)
+ }
+ }
+ } else if (expected.groups != nil && got.groups == nil) || (expected.groups == nil && got.groups != nil) {
+ return errors.New("groups of an acl is not nil, expect one and got one should be same")
+ }
+ return nil
}
-func TestACLSpecialCase(t *testing.T) {
- acl, err := NewACL(" ")
- if err == nil {
- t.Errorf("parsing passed for string: ' '")
- }
+func TestACLCreate(t *testing.T) {
+ tests := []struct {
+ input string
+ expected ACL
+ }{
+ {
+ "",
+ ACL{allAllowed: false}},
+ {
+ " ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1",
+ ACL{users: map[string]bool{"user1": true}, allAllowed: false}},
+ {
+ "user1,user2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, allAllowed: false}},
+ {
+ "user1,user2 ",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1,user2 group1",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true}, allAllowed: false},
+ },
+ {
+ "user1,user2 group1,group2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "user2 group1,group2",
+ ACL{users: map[string]bool{"user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ " group1,group2",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "* group1,group2",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "user1,user2 *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "*",
+ ACL{users: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "* ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ " *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "dotted.user",
+ ACL{users: map[string]bool{"dotted.user": true}, allAllowed: false},
+ },
+ {
+ "user,user",
+ ACL{users: map[string]bool{"user": true}, allAllowed: false},
+ },
+ {
+ " dotted.group",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false},
+ },
+ {
+ " group,group",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group": true}, allAllowed: false},
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.input, func(t *testing.T) {
+ got, err := NewACL(tt.input)
+ if err != nil {
+ t.Errorf("parsing failed for string: %s", tt.input)
+ }
- acl, err = NewACL("dotted.user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'dotted.user' acl has incorrect user list: %v", acl)
- }
- acl, err = NewACL("user,user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'user,user' acl has incorrect user list: %v", acl)
+ if err = IsSameACL(got, tt.expected); err != nil {
+ t.Error(err.Error())
+ }
+ })
}
- acl, err = NewACL(" dotted.group")
- if err != nil || len(acl.groups) > 0 {
- t.Errorf("parsing failed for string: ' dotted.group' acl has incorrect group list: %v", acl)
- }
- acl, err = NewACL(" group,group")
- if err != nil || len(acl.groups) != 1 {
- t.Errorf("parsing failed for string: 'group,group' acl has incorrect group list: %v", acl)
+}
+
+func TestNewACLErrorCase(t *testing.T) {
+ tests := []struct {
+ caseName string
+ acl string
+ }{
+ {"spaces", " "},
+ {"number of spaces is higher than 1 is not allowed", " a b "},
+ }
+ for _, tt := range tests {
+ t.Run(tt.caseName, func(t *testing.T) {
+ if _, err := NewACL(tt.acl); err == nil {
+ t.Errorf("parsing %s string should be failed", tt.acl)
+ }
+ })
}
}
func TestACLAccess(t *testing.T) {
- acl, err := NewACL("user1,user2 group1,group2")
- assert.NilError(t, err, "parsing failed for string: 'user1,user2 group1,group2'")
- user := UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, should have been denied")
- user = UserGroup{User: "user1", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "user1 (no groups) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3", "group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 (2nd group) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3"}}
- assert.Assert(t, !acl.CheckAccess(user), "user3/group3 should have been denied")
-
- acl, err = NewACL("*")
- assert.NilError(t, err, "parsing failed for string: '*'")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "no user, wildcard should have been allowed")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "user1/group1, wildcard should have been allowed")
-
- acl, err = NewACL("")
- assert.NilError(t, err, "parsing failed for string: ''")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, empty ACL always deny")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, !acl.CheckAccess(user), "user1/group1, empty ACL always deny")
+ tests := []struct {
+ acl string
+ visitor UserGroup
+ expected bool
+ }{
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "", Groups: nil},
+ false,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user1", Groups: nil},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3", "group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3"}},
+ false,
+ },
+ {
+ "*",
+ UserGroup{User: "", Groups: nil},
+ true,
+ },
+ {
+ "*",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ acl: "",
+ visitor: UserGroup{User: "", Groups: nil},
+ expected: false,
+ },
+ {
+ "",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ false,
+ },
+ }
+ for _, tt := range tests {
+ t.Run(fmt.Sprintf("vistor %v, acl %s", tt.visitor, tt.acl), func(t *testing.T) {
+ acl, err := NewACL(tt.acl)
+ if err != nil {
+ t.Error(err.Error())
Review Comment:
nit: explain that parse error is unexpected as you did before
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s closed pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s closed pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
URL: https://github.com/apache/yunikorn-core/pull/492
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] 0yukali0 commented on a diff in pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #492:
URL: https://github.com/apache/yunikorn-core/pull/492#discussion_r1060261909
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
Review Comment:
Hi @wilfred-s
Understood, thank you for comments.
I am going to update the commit later.
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] 0yukali0 commented on a diff in pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #492:
URL: https://github.com/apache/yunikorn-core/pull/492#discussion_r1060262256
##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
package security
import (
+ "errors"
+ "fmt"
"testing"
-
- "gotest.tools/assert"
)
-func TestACLCreate(t *testing.T) {
- _, err := NewACL("")
- if err != nil {
- t.Errorf("parsing failed for string: ''")
- }
- _, err = NewACL(" ")
- if err != nil {
- t.Errorf("parsing failed for string: ' '")
- }
- _, err = NewACL("user1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1'")
- }
- _, err = NewACL("user1,user2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2'")
- }
- _, err = NewACL("user1,user2 ")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 '")
- }
- _, err = NewACL("user1,user2 group1")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1'")
- }
- _, err = NewACL("user1,user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
- }
- _, err = NewACL("user2 group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: 'user2 group1,group2'")
- }
- _, err = NewACL(" group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: ' group1,group2'")
- }
- _, err = NewACL("* group1,group2")
- if err != nil {
- t.Errorf("parsing failed for string: '* group1,group2'")
- }
- _, err = NewACL("user1,user2 *")
- if err != nil {
- t.Errorf("parsing failed for string: 'user1,user2 *'")
- }
- _, err = NewACL("*")
- if err != nil {
- t.Errorf("parsing failed for string: '*'")
- }
- _, err = NewACL("* ")
- if err != nil {
- t.Errorf("parsing failed for string: '* '")
- }
- _, err = NewACL(" *")
- if err != nil {
- t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+ if got.allAllowed != expected.allAllowed {
+ return errors.New("allAllowed is not same")
+ }
+ if expected.users != nil && got.users != nil {
+ for username, expectedAllow := range expected.users {
+ if gotAllow, ok := got.users[username]; !ok {
+ return fmt.Errorf("username %s does not exist", username)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("username %s is not same", username)
+ }
+ }
+ } else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+ return errors.New("users of an acl is not nil, expect one and got one should be same")
}
+
+ if expected.groups != nil && got.groups != nil {
+ for groupname, expectedAllow := range expected.groups {
+ if gotAllow, ok := got.groups[groupname]; !ok {
+ return fmt.Errorf("groupname %s does not exist", groupname)
+ } else if expectedAllow != gotAllow {
+ return fmt.Errorf("groupname %s is not same", groupname)
+ }
+ }
+ } else if (expected.groups != nil && got.groups == nil) || (expected.groups == nil && got.groups != nil) {
+ return errors.New("groups of an acl is not nil, expect one and got one should be same")
+ }
+ return nil
}
-func TestACLSpecialCase(t *testing.T) {
- acl, err := NewACL(" ")
- if err == nil {
- t.Errorf("parsing passed for string: ' '")
- }
+func TestACLCreate(t *testing.T) {
+ tests := []struct {
+ input string
+ expected ACL
+ }{
+ {
+ "",
+ ACL{allAllowed: false}},
+ {
+ " ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1",
+ ACL{users: map[string]bool{"user1": true}, allAllowed: false}},
+ {
+ "user1,user2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, allAllowed: false}},
+ {
+ "user1,user2 ",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: make(map[string]bool), allAllowed: false}},
+ {
+ "user1,user2 group1",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true}, allAllowed: false},
+ },
+ {
+ "user1,user2 group1,group2",
+ ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "user2 group1,group2",
+ ACL{users: map[string]bool{"user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ " group1,group2",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+ },
+ {
+ "* group1,group2",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "user1,user2 *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "*",
+ ACL{users: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "* ",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ " *",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+ },
+ {
+ "dotted.user",
+ ACL{users: map[string]bool{"dotted.user": true}, allAllowed: false},
+ },
+ {
+ "user,user",
+ ACL{users: map[string]bool{"user": true}, allAllowed: false},
+ },
+ {
+ " dotted.group",
+ ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false},
+ },
+ {
+ " group,group",
+ ACL{users: make(map[string]bool), groups: map[string]bool{"group": true}, allAllowed: false},
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.input, func(t *testing.T) {
+ got, err := NewACL(tt.input)
+ if err != nil {
+ t.Errorf("parsing failed for string: %s", tt.input)
+ }
- acl, err = NewACL("dotted.user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'dotted.user' acl has incorrect user list: %v", acl)
- }
- acl, err = NewACL("user,user")
- if err != nil || len(acl.users) != 1 {
- t.Errorf("parsing failed for string: 'user,user' acl has incorrect user list: %v", acl)
+ if err = IsSameACL(got, tt.expected); err != nil {
+ t.Error(err.Error())
+ }
+ })
}
- acl, err = NewACL(" dotted.group")
- if err != nil || len(acl.groups) > 0 {
- t.Errorf("parsing failed for string: ' dotted.group' acl has incorrect group list: %v", acl)
- }
- acl, err = NewACL(" group,group")
- if err != nil || len(acl.groups) != 1 {
- t.Errorf("parsing failed for string: 'group,group' acl has incorrect group list: %v", acl)
+}
+
+func TestNewACLErrorCase(t *testing.T) {
+ tests := []struct {
+ caseName string
+ acl string
+ }{
+ {"spaces", " "},
+ {"number of spaces is higher than 1 is not allowed", " a b "},
+ }
+ for _, tt := range tests {
+ t.Run(tt.caseName, func(t *testing.T) {
+ if _, err := NewACL(tt.acl); err == nil {
+ t.Errorf("parsing %s string should be failed", tt.acl)
+ }
+ })
}
}
func TestACLAccess(t *testing.T) {
- acl, err := NewACL("user1,user2 group1,group2")
- assert.NilError(t, err, "parsing failed for string: 'user1,user2 group1,group2'")
- user := UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, should have been denied")
- user = UserGroup{User: "user1", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "user1 (no groups) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3", "group1"}}
- assert.Assert(t, acl.CheckAccess(user), "group1 (2nd group) should have been allowed")
- user = UserGroup{User: "user3", Groups: []string{"group3"}}
- assert.Assert(t, !acl.CheckAccess(user), "user3/group3 should have been denied")
-
- acl, err = NewACL("*")
- assert.NilError(t, err, "parsing failed for string: '*'")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, acl.CheckAccess(user), "no user, wildcard should have been allowed")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, acl.CheckAccess(user), "user1/group1, wildcard should have been allowed")
-
- acl, err = NewACL("")
- assert.NilError(t, err, "parsing failed for string: ''")
- user = UserGroup{User: "", Groups: nil}
- assert.Assert(t, !acl.CheckAccess(user), "no user, empty ACL always deny")
- user = UserGroup{User: "user1", Groups: []string{"group1"}}
- assert.Assert(t, !acl.CheckAccess(user), "user1/group1, empty ACL always deny")
+ tests := []struct {
+ acl string
+ visitor UserGroup
+ expected bool
+ }{
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "", Groups: nil},
+ false,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user1", Groups: nil},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3", "group1"}},
+ true,
+ },
+ {
+ "user1,user2 group1,group2",
+ UserGroup{User: "user3", Groups: []string{"group3"}},
+ false,
+ },
+ {
+ "*",
+ UserGroup{User: "", Groups: nil},
+ true,
+ },
+ {
+ "*",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ true,
+ },
+ {
+ acl: "",
+ visitor: UserGroup{User: "", Groups: nil},
+ expected: false,
+ },
+ {
+ "",
+ UserGroup{User: "user1", Groups: []string{"group1"}},
+ false,
+ },
+ }
+ for _, tt := range tests {
+ t.Run(fmt.Sprintf("vistor %v, acl %s", tt.visitor, tt.acl), func(t *testing.T) {
+ acl, err := NewACL(tt.acl)
+ if err != nil {
+ t.Error(err.Error())
Review Comment:
I would remove the err later because the acl string is in the create unit tests.
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #492:
URL: https://github.com/apache/yunikorn-core/pull/492#issuecomment-1368712359
# [Codecov](https://codecov.io/gh/apache/yunikorn-core/pull/492?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#492](https://codecov.io/gh/apache/yunikorn-core/pull/492?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2c391e) into [master](https://codecov.io/gh/apache/yunikorn-core/commit/ef3f0c7a10b5c5362f327ba9a84b463910a2b138?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ef3f0c7) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #492 +/- ##
=======================================
Coverage 73.07% 73.07%
=======================================
Files 69 69
Lines 10371 10371
=======================================
Hits 7579 7579
Misses 2544 2544
Partials 248 248
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org