New model for buckets #172

lx · 2021-12-14T12:57:32Z

lx commented

2021-12-14 12:57:32 +00:00

This should allow to implement CreateBucket and DeleteBucket, as well as garage-specific features such as aliasing and renaming bucket, as well as local aliases where keys have their own namespace of buckets they see.

new model that supports aliases
adapt all existing code
add bucket alias and bucket unalias
include in model possibility to store key's permission to create and delete buckets
include in model possibility to store bucket website config
write migration code

For next PRs:

command line to manage allow_create_bucket flag
implement CreateBucket and DeleteBucket #97
implement GetWebsiteConfiguration
fix permissions for Put,GetWebsiteConfiguration (make use of allow_owner)
actually make use of website configuration document in web endpoint
cli options to more directly manipulate the alias database (e.g. directly call purge_global_bucket_alias) and to repair bucket alias links if inconsistent

This should allow to implement CreateBucket and DeleteBucket, as well as garage-specific features such as aliasing and renaming bucket, as well as local aliases where keys have their own namespace of buckets they see. - [x] new model that supports aliases - [x] adapt all existing code - [x] add `bucket alias` and `bucket unalias` - [x] include in model possibility to store key's permission to create and delete buckets - [x] include in model possibility to store bucket website config - [x] write migration code For next PRs: - [x] command line to manage `allow_create_bucket` flag - [x] implement CreateBucket and DeleteBucket #97 - [x] implement GetWebsiteConfiguration - [x] fix permissions for Put,GetWebsiteConfiguration (make use of `allow_owner`) - [x] actually make use of website configuration document in web endpoint - [ ] cli options to more directly manipulate the alias database (e.g. directly call purge_global_bucket_alias) and to repair bucket alias links if inconsistent

lx force-pushed new-buckets from f2edb31ee1 to c3e8309ee5

2021-12-14 13:56:23 +00:00

Compare

lx force-pushed new-buckets from c3e8309ee5 to 66e1e78d19

2021-12-14 14:07:58 +00:00

Compare

lx force-pushed new-buckets from 66e1e78d19 to 63a515a9f9

2021-12-14 14:53:57 +00:00

Compare

lx force-pushed new-buckets from 69886ef1a7 to ca54ff40c6

2021-12-15 14:39:18 +00:00

Compare

lx force-pushed new-buckets from 3e75fc9b6b to ea5cddd30b

2021-12-16 12:34:33 +00:00

Compare

lx changed title from ~~WIP: New model for buckets~~ to New model for buckets

2021-12-16 15:18:45 +00:00

trinity-1686a reviewed 2021-12-16 23:32:26 +00:00

trinity-1686a left a comment

first round of review (up to model sub-crate, I did not get the time to go further).

I think try_migrate on tables should take a hint about the version. At the moment we only have a single one, in the future we might have multiple, and could easily parse the wrong one.

@quentin could you run your benchmark scripts on this vs main. I wonder how/if the extra indection impact first byte latency

first round of review (up to model sub-crate, I did not get the time to go further). I think `try_migrate` on tables should take a hint about the version. At the moment we only have a single one, in the future we might have multiple, and could easily parse the wrong one. @quentin could you run your benchmark scripts on this vs main. I wonder how/if the extra indection impact first byte latency

src/api/api_server.rs Outdated

					
				@ -106,2 +109,4 @@

					let endpoint = Endpoint::from_request(&req, bucket.map(ToOwned::to_owned))?;

					let bucket_name = match endpoint.authorization_type() {

trinity-1686a commented

2021-12-16 19:34:55 +00:00

there is a .get_bucket() on Endpoint which can be used to do the same thing more concisely

there is a `.get_bucket()` on `Endpoint` which can be used to do the same thing more concisely

lx marked this conversation as resolved

src/model/bucket_helper.rs Outdated

					
				@ -0,0 +8,4 @@

				pub struct BucketHelper<'a>(pub(crate) &'a Garage);

				#[allow(clippy::ptr_arg)]

trinity-1686a commented

2021-12-16 23:25:40 +00:00

I think this link could be one line lower to only cover the "offending" function

lx marked this conversation as resolved

src/model/bucket_helper.rs Outdated

					
				@ -0,0 +18,4 @@

							.ok()

							.map(|by| Uuid::try_from(&by))

							.flatten();

						if let Some(bucket_id) = hexbucket {

trinity-1686a commented

2021-12-16 23:24:25 +00:00

can't this pose problem if someone create a bucket which name is hex of the right size?

lx commented

2021-12-17 11:33:48 +00:00

I added a check of the validity of bucket names. AWS bucket names are max 63 characters long, so no risk of it being a hex uuid.

👍 1

trinity-1686a commented

2021-12-17 13:17:31 +00:00

could you add a comment saying that?

lx marked this conversation as resolved

src/model/bucket_helper.rs Outdated

					
				@ -0,0 +37,4 @@

						}

					}

					#[allow(clippy::ptr_arg)]

trinity-1686a commented

2021-12-16 23:25:12 +00:00

I don't think this lint is required

lx marked this conversation as resolved

src/model/bucket_table.rs Outdated

					
				@ -50,0 +32,4 @@

					/// (under all of its global alias names)

					pub website_access: crdt::Lww<bool>,

					/// The website configuration XML document

					pub website_config: crdt::Lww<Option<ByteBuf>>,

trinity-1686a commented

2021-12-16 21:06:41 +00:00

I'd argue only website_config should exist, with a Some(_) generated automatically when migrating from 0.5 with website enabled.
I also think this should probably contain a WebsiteConfiguration (or some flattened form of it), to not require parsing XML on each web request, however doing so does have downsides if we add things to this struct in the future.

I'd argue only `website_config` should exist, with a `Some(_)` generated automatically when migrating from 0.5 with website enabled. I also think this should probably contain a `WebsiteConfiguration` (or some flattened form of it), to not require parsing XML on each web request, however doing so does have downsides if we add things to this struct in the future.

lx commented

2021-12-17 11:13:40 +00:00

Concerning the first point (removing website_access and storing only the config as an option), I think yes that's probably better because it's closer to how Garage works currently. I was hesitant because in AWS the permissions and the website config seem to be handled separately, but Garage has its own logic and doesn't implement AWS's ACLs, so yes we can simplify here for now and come back to it later when/if we work on ACLs.

Concerning storing a WebsiteConfiguration instead of a ByteBuf, there are upsides and downsides.

Upsides:

don't parse xml at every web request
the messagepack representation is probably smaller

Downsides:

we have to move the definition of WebsiteConfiguration in the model crate (for now we have kept very separate the structs that manage Garage's internal state and structs that represent official items of the S3 spec) ; if we don't want to do this, the alternative is to have our own struct that keeps only the relevant aspects of a WebsiteConfiguration, but it adds more complexity
we are now parsing messagepack for the website configuration for all accesses to this and not just when doing a website call

Alternatives:

if we store XML, we can cache the parsed XML in the web/ module for a nominal duratio of a few seconds to avoid parsing at every request
if handling the website config ends up taking too much cpu resource, we can move it to a separate table
we could also implement optimisations in the table/ module to keep a cache of deserialized versions of stuff stored in the table

In other words, storing a struct here 1/ has disadvantages in terms of keeping a clean architectures, and 2/ looks to me a bit like a case of premature optimization (we can have a separate reflexion later concerning how we approach performance on Garage tables, which is a whole topic on its own with multiple aspects such as caching, minimizing representation sizes, splitting stuff in separate tables, etc).

Concerning the first point (removing `website_access` and storing only the config as an option), I think yes that's probably better because it's closer to how Garage works currently. I was hesitant because in AWS the permissions and the website config seem to be handled separately, but Garage has its own logic and doesn't implement AWS's ACLs, so yes we can simplify here for now and come back to it later when/if we work on ACLs. Concerning storing a WebsiteConfiguration instead of a ByteBuf, there are upsides and downsides. Upsides: - don't parse xml at every web request - the messagepack representation is probably smaller Downsides: - we have to move the definition of WebsiteConfiguration in the model crate (for now we have kept very separate the structs that manage Garage's internal state and structs that represent official items of the S3 spec) ; if we don't want to do this, the alternative is to have our own struct that keeps only the relevant aspects of a WebsiteConfiguration, but it adds more complexity - we are now parsing messagepack for the website configuration for all accesses to this and not just when doing a website call Alternatives: - if we store XML, we can cache the parsed XML in the `web/` module for a nominal duratio of a few seconds to avoid parsing at every request - if handling the website config ends up taking too much cpu resource, we can move it to a separate table - we could also implement optimisations in the `table/` module to keep a cache of deserialized versions of stuff stored in the table In other words, storing a struct here 1/ has disadvantages in terms of keeping a clean architectures, and 2/ looks to me a bit like a case of premature optimization (we can have a separate reflexion later concerning how we approach performance on Garage tables, which is a whole topic on its own with multiple aspects such as caching, minimizing representation sizes, splitting stuff in separate tables, etc).

lx marked this conversation as resolved

src/model/permission.rs Outdated

					
				@ -0,0 +33,4 @@

								}

								if !other.allow_write {

									self.allow_write = false;

								}

trinity-1686a commented

2021-12-16 20:47:39 +00:00

allow_owner is not merged. Is it sementically read-only hence don't needs to (if so, please document it), or just forgotten?

lx commented

2021-12-17 11:14:59 +00:00

Just forgotten, thx

lx marked this conversation as resolved

src/util/crdt/crdt.rs Outdated

					
				@ -31,0 +33,4 @@

					T: Eq,

				{

					fn merge(&mut self, other: &Self) {

						if self != other {

trinity-1686a commented

2021-12-16 20:28:04 +00:00

I find this implementation surprising. I'd expect Some(true) and Some(false) to be merged to Some(true), not None.

I find this implementation surprising. I'd expect `Some(true)` and `Some(false)` to be merged to `Some(true)`, not `None`.

lx commented

2021-12-17 11:20:45 +00:00

So I haven't written a doc comment for this yet, but the idea of this impl is that we can make a CRDT of any type T that doesn't implement CRDT, by declaring that different values merge to None. We probably should use this more often in fact, instead of trying to merge things when we know we don't want to merge them (which is what the AutoCrdt trait is used for most of the time). This cases arises very often, for example with a Lww or a LwwMap: the value type has to be a CRDT so that we have a rule for what to do when timestamps aren't enough to disambiguate (in a distributed system, anything can happen!), and with AutoCrdt the rule is to make an arbitrary (but determinstic) choice between the two. When using an option instead with this impl, ambiguity cases are explicitely stored as None, which allows us to detect the ambiguity and handle it in the way we want. This truly depends on the semantics of the application: the crdt module is just a toolbox of things that can be taken when needed when building models. In the precise case where we are using it here, i.e. for storing the website configuration, the logic is that if we have two website configurations and we don't know which one is the correct one, we can just disable website access until an admin gives a new configuration (we try to have a safe behavior instead of an unpredictable one -- but anyways this is an extreme edge case as it mostly should never happen that the timestamps are the same).

So I haven't written a doc comment for this yet, but the idea of this impl is that we can make a CRDT of any type T that doesn't implement CRDT, by declaring that different values merge to None. We probably should use this more often in fact, instead of trying to merge things when we know we don't want to merge them (which is what the AutoCrdt trait is used for most of the time). This cases arises very often, for example with a `Lww` or a `LwwMap`: the value type has to be a CRDT so that we have a rule for what to do when timestamps aren't enough to disambiguate (in a distributed system, anything can happen!), and with AutoCrdt the rule is to make an arbitrary (but determinstic) choice between the two. When using an option instead with this impl, ambiguity cases are explicitely stored as None, which allows us to detect the ambiguity and handle it in the way we want. This truly depends on the semantics of the application: the `crdt` module is just a toolbox of things that can be taken when needed when building models. In the precise case where we are using it here, i.e. for storing the website configuration, the logic is that if we have two website configurations and we don't know which one is the correct one, we can just disable website access until an admin gives a new configuration (we try to have a safe behavior instead of an unpredictable one -- but anyways this is an extreme edge case as it mostly should never happen that the timestamps are the same).

trinity-1686a commented

2021-12-18 16:55:16 +00:00

I understand now, but I'm not sure it's well fit to some place it's used.
For website configuration, if two updates happen at the same time, I think it makes more sense to consider an arbitrary one happened an instant before the other, and got overwritten, than to merge both into the state "website disabled"

I understand now, but I'm not sure it's well fit to some place it's used. For website configuration, if two updates happen at the same time, I think it makes more sense to consider an arbitrary one happened an instant before the other, and got overwritten, than to merge both into the state "website disabled"

lx marked this conversation as resolved

trinity-1686a reviewed 2021-12-18 15:48:44 +00:00

src/model/bucket_table.rs Outdated

					
				@ -6,1 +9,3 @@

				use crate::key_table::PermissionSet;

				use crate::permission::BucketKeyPerm;

				pub const DEFAULT_WEBSITE_CONFIGURATION: &[u8] = b""; // TODO (an XML WebsiteConfiguration document per the AWS spec)

trinity-1686a commented

2021-12-18 15:48:44 +00:00

this is the equivalent to garage default config in 0.5.0

<?xml version="1.0" encoding="UTF-8"?>
<WebsiteConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
  <IndexDocument>
    <Suffix>index.html</Suffix>
  </IndexDocument>
</WebsiteConfiguration>

I'm still not convinced this should be XML, not because of performances reasons, but because I don't think we should store raw S3 payloads in general (and I generaly don't like XML)

this is the equivalent to garage default config in 0.5.0 ```xml <?xml version="1.0" encoding="UTF-8"?> <WebsiteConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> <IndexDocument> <Suffix>index.html</Suffix> </IndexDocument> </WebsiteConfiguration> ``` I'm still not convinced this should be XML, not because of performances reasons, but because I don't think we should store raw S3 payloads in general (and I generaly don't like XML)

lx marked this conversation as resolved

trinity-1686a reviewed 2021-12-18 17:14:10 +00:00

src/api/s3_bucket.rs Outdated

					
				@ -38,0 +50,4 @@

						.authorized_buckets

						.items()

						.iter()

						.filter(|(_, perms)| perms.allow_read || perms.allow_write)

trinity-1686a commented

2021-12-18 16:02:12 +00:00

would it make sense to be bucket owner without read nor write permission?

lx marked this conversation as resolved

src/api/s3_bucket.rs Outdated

					
				@ -38,0 +67,4 @@

										if let Some(alias_ent) = alias_ent {

											if let Some(alias_p) = alias_ent.state.get().as_option() {

												if alias_p.bucket_id == *bucket_id {

													aliases.insert(alias_ent.name().to_string(), *bucket_id);

trinity-1686a commented

2021-12-18 16:42:19 +00:00

this is a lot of indentation. It may be possible to remove a few layers by using Option::and_then, and doing param.aliases.items().filter(|(_,_,active)| active)

this is a lot of indentation. It may be possible to remove a few layers by using Option::and_then, and doing `param.aliases.items().filter(|(_,_,active)| active)`

lx marked this conversation as resolved

lx force-pushed new-buckets from cf5fe9080f to dc1971e427

2022-01-03 10:33:47 +00:00

Compare

lx force-pushed new-buckets from f74ce3f0b6 to fdbf5bc235

2022-01-03 11:17:52 +00:00

Compare

lx force-pushed new-buckets from 301b7b928a to 70c84d8d44

2022-01-03 17:58:30 +00:00

Compare

lx force-pushed new-buckets from a431e18736 to b5b2e47754

2022-01-03 18:18:57 +00:00