[#340] Fix encode object acl #356

r.loginov · 2024-04-10T07:20:44Z

r.loginov commented

2024-04-10 07:20:44 +00:00

close #340

In the process of encode the acl of an object,
we use a map. As a result, when traversing the
map, we can get a different sequence of permissions
each time. Therefore, a list is used instead of a map.

Signed-off-by: Roman Loginov r.loginov@yadro.com

close #340 In the process of encode the acl of an object, we use a map. As a result, when traversing the map, we can get a different sequence of permissions each time. Therefore, a list is used instead of a map. Signed-off-by: Roman Loginov <r.loginov@yadro.com>

r.loginov added the

bug

label 2024-04-10 07:20:44 +00:00

r.loginov self-assigned this 2024-04-10 07:20:44 +00:00

r.loginov requested review from storage-services-committers 2024-04-10 07:32:26 +00:00

r.loginov requested review from storage-services-developers 2024-04-10 07:32:26 +00:00

pogpp approved these changes 2024-04-10 07:56:55 +00:00

dkirillov reviewed 2024-04-10 08:29:47 +00:00

api/handler/acl.go Outdated

					
				@ -1665,3 +1695,3 @@

					}

					for key, val := range m {

					for _, val := range m.list {

dkirillov commented

2024-04-10 08:29:47 +00:00

Maybe we can just add the following

	sort.Slice(res.AccessControlList, func(i, j int) bool {
		return res.AccessControlList[i].Grantee.ID > res.AccessControlList[j].Grantee.ID
	})

after this loop and don't introduce accessList?

Maybe we can just add the following ```golang sort.Slice(res.AccessControlList, func(i, j int) bool { return res.AccessControlList[i].Grantee.ID > res.AccessControlList[j].Grantee.ID }) ``` after this loop and don't introduce `accessList`?

r.loginov commented

2024-04-10 09:04:14 +00:00

I've been thinking about it, and it will really solve the problem with the test. However, if we imagine a situation where we have several acl records for different users on the same object, then the order of records will still be non-deterministic. As far as I know, in aws s3, records are output (get-object-acl) in the order in which they were added, despite the fact that functionally it doesn't seem to affect anything. If it still doesn't matter to us, then of course it's better to use sorting.

dkirillov commented

2024-04-10 09:19:35 +00:00

Ok, let's keep current approach. But simplify a little:

diff --git a/api/handler/acl.go b/api/handler/acl.go
index a7cd017d..ea6a311c 100644
--- a/api/handler/acl.go
+++ b/api/handler/acl.go
@@ -1639,25 +1639,15 @@ type accessList struct {
        list []access
 }
 
-func (c *accessList) addAccess(recipient string, operations []eacl.Operation) {
+func (c *accessList) addAccess(recipient string, operation eacl.Operation) {
        for i, v := range c.list {
                if v.recipient == recipient {
-                       c.list[i].operations = operations
+                       c.list[i].operations = append(c.list[i].operations, operation)
                        return
                }
        }
 
-       c.list = append(c.list, access{recipient, operations})
-}
-
-func (c *accessList) getAccess(recipient string) []eacl.Operation {
-       for _, entry := range c.list {
-               if entry.recipient == recipient {
-                       return entry.operations
-               }
-       }
-
-       return []eacl.Operation{}
+       c.list = append(c.list, access{recipient, []eacl.Operation{operation}})
 }
 
 func (h *handler) encodeObjectACL(ctx context.Context, bucketACL *layer.BucketACL, bucketName, objectVersion string) *AccessControlPolicy {
@@ -1683,12 +1673,10 @@ func (h *handler) encodeObjectACL(ctx context.Context, bucketACL *layer.BucketAC
                        }
 
                        if len(op.Users) == 0 {
-                               list := append(m.getAccess(allUsersGroup), op.Op)
-                               m.addAccess(allUsersGroup, list)
+                               m.addAccess(allUsersGroup, op.Op)
                        } else {
                                for _, user := range op.Users {
-                                       list := append(m.getAccess(user), op.Op)
-                                       m.addAccess(user, list)
+                                       m.addAccess(user, op.Op)
                                }
                        }
                }

Ok, let's keep current approach. But simplify a little: ```diff diff --git a/api/handler/acl.go b/api/handler/acl.go index a7cd017d..ea6a311c 100644 --- a/api/handler/acl.go +++ b/api/handler/acl.go @@ -1639,25 +1639,15 @@ type accessList struct { list []access } -func (c *accessList) addAccess(recipient string, operations []eacl.Operation) { +func (c *accessList) addAccess(recipient string, operation eacl.Operation) { for i, v := range c.list { if v.recipient == recipient { - c.list[i].operations = operations + c.list[i].operations = append(c.list[i].operations, operation) return } } - c.list = append(c.list, access{recipient, operations}) -} - -func (c *accessList) getAccess(recipient string) []eacl.Operation { - for _, entry := range c.list { - if entry.recipient == recipient { - return entry.operations - } - } - - return []eacl.Operation{} + c.list = append(c.list, access{recipient, []eacl.Operation{operation}}) } func (h *handler) encodeObjectACL(ctx context.Context, bucketACL *layer.BucketACL, bucketName, objectVersion string) *AccessControlPolicy { @@ -1683,12 +1673,10 @@ func (h *handler) encodeObjectACL(ctx context.Context, bucketACL *layer.BucketAC } if len(op.Users) == 0 { - list := append(m.getAccess(allUsersGroup), op.Op) - m.addAccess(allUsersGroup, list) + m.addAccess(allUsersGroup, op.Op) } else { for _, user := range op.Users { - list := append(m.getAccess(user), op.Op) - m.addAccess(user, list) + m.addAccess(user, op.Op) } } } ```

👍 1

dkirillov marked this conversation as resolved

r.loginov force-pushed bugfix/340-flaky_test_put_object_acl from 0e2819b07f to d1f4941b63

2024-04-10 09:37:04 +00:00

Compare

dkirillov approved these changes 2024-04-10 09:40:26 +00:00

r.loginov force-pushed bugfix/340-flaky_test_put_object_acl from d1f4941b63 to 08dd6aff41

2024-04-10 13:11:44 +00:00

Compare

alexvanin approved these changes 2024-04-11 09:28:24 +00:00

alexvanin merged commit d8889fca56 into master

2024-04-11 09:28:31 +00:00