Code review comment for lp:~gaurav-gangalwar/swift/object_acl

On Fri, Aug 12, 2011 at 7:40 PM, gholt <email address hidden> wrote:

> The object acl seems to override the container acl; so I could grant Susan
> write access to my container and then she could create objects that I could
> not delete or overwrite, even though I have write access to the container.
> Also, I could seemingly take away Susan's access to a container, but she
> might be able to access objects in the container still.
>

Write object-acl will only control POST and GETorHEAD on the object, You can
DELETE or PUT objects inside the container if you have write access or owner
of the account.

>
> This feature would need to be off by default. If a large cluster operator
> were to upgrade to this code and not realize they had to add "object_acl =
> false" to their configuration, they'd have some difficulties. Not only can
> this have a dramatic impact on memcache usage, but it also will up the
> number of backend requests.
>

Ok, in large clusters if its enabled then it might lead to excess memcache
usage, but it will not affect any functionality if object-acl is not set.

>
> Why is the "object_size" memcached?
>

object_size is not used for now, will remove it.

>
> In the object GETorHEAD the acls are retrieved even if 'swift.authorize'
> not in req.environ. This isn't often used, but was added to allow
> short-circuiting such extra queries to lower memcache and backend requests
> when they aren't needed.
>

In which case swift.authorize will not be in the req.environ? I thought auth
subsystem will set it for each req.

>
> The logic for setting req.acl is different than before. It used to be that
> write access could be granted to a container without granting read access
> (blind drop box type-thing). In the object GETorHEAD, now write access also
> grants read access.
>

Yes, allowing write and not allowing read is not supposed to be the expected
behaviour, so write acl will have read/write access to the object.

>
> With segmented objects, why the change to giving the end user all headers
> from the backend server?
>

This filtering is done by the backend servers, so it was redundant here,
thats why.

>
> In the container GETorHEAD, the change to calling container_info before
> performing the backend container request causes extra requests that aren't
> needed. The way it was coded before, it just always did the container
> backend request and returned the response depending on the acl values in the
> backend response.
>

In case of unauthorized it will be only one request. I thought of keeping a
single interface for memcache, could we remove memcache updation code from
Container ops?

>
> The code now requires write access to a container to delete or post to that
> container, whereas before it just required the swift.authorize method to
> allow it. This change means a user that has full access to an account but
> isn't explicitly in the write list for a container can't do anything about
> it.
>

No, if you are the owner of account, authorize will not check for any access
list, it will give you all access to the containers and objects.

>
> Whereas there are some new functional tests, there aren't any new unit
> tests and many existing unit tests are failing, but I think you were already
> aware of that part and working on it.
>

Yes will update this soon.

>
> --
> https://code.launchpad.net/~gaurav-gangalwar/swift/object_acl/+merge/70003<https://code.launchpad.net/%7Egaurav-gangalwar/swift/object_acl/+merge/70003>
> You are the owner of lp:~gaurav-gangalwar/swift/object_acl.
>