Bucket namespaces pull req. 5872

Bucket namespaces pull req. 5872

[Pete Zaitcev]

I took a decent look at the pull request 5872
  https://github.com/ceph/ceph/pull/5872
It implements something called "bucket namespaces": a way to make
buckets qualified with a prefix that permits different users use
buckets with the same name.

I think I like the idea overall, but the implementation raises
some questions. The most important in my mind is: why use rgw_user?

In the wip-5073, rgw_user is needed because tenant there adds
a namespace both to users and buckets. But here, users are not
in a namespace, only buckets are. Or at least that's what I see
in the code, please set me straight if I'm wrong.

Conceptually, the user name is just a label, and this patch keeps
those labels compatible. I think, the information about a user
should contain the user's bucket namespace, but the user's label
does not need to have it. So, RGWUserInfo should have the bucket
namespace name (and possibly has_own_bns), and rgw_user is superfluous.

If we could get rid of rgw_user, I would be onboard with this.

Less importantly, I do not like the generosity with knobs.
The rgw_swift_create_account_with_bns shold go away with rgw_user.
The rgw_swift_account_in_url should be possible to incorporate
in a compatible fashion (it does not add an extra next_tok()).
The rgw_keystone_accepted_admin_roles... okay, that one might
be needed. Swift has an equivalent of it.

Finally, there are some miniscule technical issues.
 - Is it just me, or do encoding and decoding of RGWUserInfo do
   not match?  Decoding appears to make provision for wip-5073,
   which we may not even need.
 - The --own-bucket-namespace should not be a boolean, but the
   namespace's name.
 - There's some junk imported from wip-5073; I'll work on cleaning
   that up.

-- Pete

Re: Bucket namespaces pull req. 5872

[Radoslaw Zarzynski]

Hello Pete,

Thank you for your review and sorry for very late response.

> The most important in my mind is: why use rgw_user?

You are completely right, rgw_user isn’t necessary for BNS. I preserved
it because of type-correctness. Having separate language type for basic
entities makes code easier to understand and extend in future. This trait
could be particularly important if Yehuda decide to implement multi-
tenancy. Those are the reasons I’m behind keeping rgw_user even if the
entire information it carries would be solely an ID string.

> I think, the information about a user should contain the user's bucket namespace,
> but the user's label does not need to have it. So, RGWUserInfo should have the
> bucket namespace name (and possibly has_own_bns)

Yeah, bucket namespace is a part of account/RGWUserInfo*. Property
“has_own_bns” even now is serialized together with RGWUserInfo.
It’s passed as a part of rgw_user in order to skip reworking signatures
of many functions and methods. However, I would say that in the end
it’s worth to perform such refactoring and thus have rgw_user cleaned.
I will work on that.

> Less importantly, I do not like the generosity with knobs.

Me too, but sometimes introduction of configuration parameter is
necessary to not enforce new behavior that may break backward
compatibility.

> The rgw_swift_create_account_with_bns shold go away with rgw_user.

Option "rgw_swift_create_account_with_bns" is needed mostly due to
integration with OpenStack (Keystone) when accounts* are automatically
created at first access. Without the parameter you would lose ability to
tell radosgw what is more important for you: compliance with Swift API
or previous behavior that still may be useful in some cases. Creating
massive amount of accounts by hand might not be an option here.

> The rgw_swift_account_in_url should be possible to incorporate
> in a compatible fashion (it does not add an extra next_tok()).

According to "rgw_swift_account_in_url": I don’t see viable method for
deducing whether two tokens in URL refer to 1) account and bucket or
2) bucket and object. Of course, we may apply some kind of heuristic
like scanning the first token for auth prefix (eg. “AUTH_”, “KEY_”) but
this would introduce limitations on bucket naming.

> - Is it just me, or do encoding and decoding of RGWUserInfo do
>   not match?  Decoding appears to make provision for wip-5073,
>   which we may not even need.

I'm not sure whether I understood you correctly. Serialization of bufferlist
is versioned and we provide code to support previous versions. However,
in the case of removing "tenant" (if we decide it's desired at the moment),
touching original commits of wip-5073 in order to simply avoid adding it
might be better.

> - The --own-bucket-namespace should not be a boolean, but the
>   namespace's name.

We may switch to this behavior if we think there are some use cases
which could be covered with such transition. From technical point of
view this is not a problem.

Best regards,
Radoslaw Zarzynski

* in the context of radosgw under term "account" I understand some parts
of RGWUserInfo and bucket index.
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bucket namespaces pull req. 5872

[Radoslaw Zarzynski]

Another thing worth discussion is the relation between bucket
namespaces and multi-tenancy. I understand MT as a mean to
provide grouping and separation between user IDs while BNS
does the same but in the domain of bucket names. In that sense
both things are perfectly orthogonal.

PR #5872 contains more than plain BNS. It also splits account
owner from identity used to authorize a given operation. Since
now an authentication subsystem (eg. Keystone or the internal,
RADOS-based one) need to set three basic things in req_state:
account owner (req_state::user), authentication identify used by
the verify_permission() methods (req_state::auth_user) and level
of permission (req_state::perm_mask). It also may create new
account (RGWUserInfo) if necessary.

Choice of identifiers in req_state::user and rgw_user::auth_user
is delegated to specific authentication subsystem. One could set
both to the same value while other might decide to differentiate
them and reflect state of some external source-of-truth (Keystone).
However, core of rgw doesn't need to care nor even be aware about
multi-tenancy.

From my understanding wip-5073 implements the tenant concept
at the core layer.

Best regards,
Radoslaw Zarzynski


On Thu, Oct 15, 2015 at 7:25 AM, Pete Zaitcev <zaitcev@redhat.com> wrote:
> I took a decent look at the pull request 5872
>   https://github.com/ceph/ceph/pull/5872
> It implements something called "bucket namespaces": a way to make
> buckets qualified with a prefix that permits different users use
> buckets with the same name.
>
> I think I like the idea overall, but the implementation raises
> some questions. The most important in my mind is: why use rgw_user?
>
> In the wip-5073, rgw_user is needed because tenant there adds
> a namespace both to users and buckets. But here, users are not
> in a namespace, only buckets are. Or at least that's what I see
> in the code, please set me straight if I'm wrong.
>
> Conceptually, the user name is just a label, and this patch keeps
> those labels compatible. I think, the information about a user
> should contain the user's bucket namespace, but the user's label
> does not need to have it. So, RGWUserInfo should have the bucket
> namespace name (and possibly has_own_bns), and rgw_user is superfluous.
>
> If we could get rid of rgw_user, I would be onboard with this.
>
> Less importantly, I do not like the generosity with knobs.
> The rgw_swift_create_account_with_bns shold go away with rgw_user.
> The rgw_swift_account_in_url should be possible to incorporate
> in a compatible fashion (it does not add an extra next_tok()).
> The rgw_keystone_accepted_admin_roles... okay, that one might
> be needed. Swift has an equivalent of it.
>
> Finally, there are some miniscule technical issues.
>  - Is it just me, or do encoding and decoding of RGWUserInfo do
>    not match?  Decoding appears to make provision for wip-5073,
>    which we may not even need.
>  - The --own-bucket-namespace should not be a boolean, but the
>    namespace's name.
>  - There's some junk imported from wip-5073; I'll work on cleaning
>    that up.
>
> -- Pete

Re: Bucket namespaces pull req. 5872

[Pete Zaitcev]

On Mon, 26 Oct 2015 16:09:41 +0100
Radoslaw Zarzynski <rzarzynski@mirantis.com> wrote:

> Those are the reasons I’m behind keeping rgw_user even if the
> entire information it carries would be solely an ID string.

Okay. It felt a little obfuscatory but perhaps it's my kernel background talking.

> Yeah, bucket namespace is a part of account/RGWUserInfo*. Property
> “has_own_bns” even now is serialized together with RGWUserInfo.

Very well, we're good.

> > The rgw_swift_create_account_with_bns shold go away with rgw_user.
> 
> Option "rgw_swift_create_account_with_bns" is needed mostly due to
> integration with OpenStack (Keystone) when accounts* are automatically
> created at first access. Without the parameter you would lose ability to
> tell radosgw what is more important for you: compliance with Swift API
> or previous behavior that still may be useful in some cases. Creating
> massive amount of accounts by hand might not be an option here.

I'm buying the logic here: at the time of auto-creation, we do not
possess the information about the account being auto-created wanting BNS
or not. Still, it feels unsatisfactory. I'd rather look into some sort
of user attributes in Keystone or whatnot. I'll investigate and report.

> > The rgw_swift_account_in_url should be possible to incorporate
> > in a compatible fashion (it does not add an extra next_tok()).
> 
> According to "rgw_swift_account_in_url": I don’t see viable method for
> deducing whether two tokens in URL refer to 1) account and bucket or
> 2) bucket and object. Of course, we may apply some kind of heuristic
> like scanning the first token for auth prefix (eg. “AUTH_”, “KEY_”) but
> this would introduce limitations on bucket naming.

That makes sense, but it's not how I read the actual code. I'll look again.

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bucket namespaces pull req. 5872

[Pete Zaitcev]

On Mon, 26 Oct 2015 16:09:41 +0100
Radoslaw Zarzynski <rzarzynski@mirantis.com> wrote:

> > The rgw_swift_account_in_url should be possible to incorporate
> > in a compatible fashion (it does not add an extra next_tok()).
> 
> According to "rgw_swift_account_in_url": I don’t see viable method for
> deducing whether two tokens in URL refer to 1) account and bucket or
> 2) bucket and object. Of course, we may apply some kind of heuristic
> like scanning the first token for auth prefix (eg. “AUTH_”, “KEY_”) but
> this would introduce limitations on bucket naming.

I thought a bit more about this and I want to backtrack on my agreement.
Your reasoning would be sound if we did not know the format of the
incoming URL ahead of time. Indeed there's no telling if it's
/account/bucket or /bucket/object. But actually we do have that knowledge,
because it's the URL that we gave to the client when it authenticated!

There may be an exception, namely if a client gets a token across a cluster
upgrade. For a while we should recognize old tokens and thus old StorageURL
formats. This makes me think that the kind of path parsing could be a
parameter hidden in the token somewhere.

Anyhow, rather than speculate about it, I'm going to put together a patch
to go on top of current WIP 5073 and then you'll see what I mean.

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html