Deuxfleurs/garage
48
55
Fork 73
Code Issues 100 Pull requests 15 Releases 30 Wiki Activity

New model for buckets #172

Merged
lx merged 19 commits from new-buckets into main 2022-01-10 11:32:42 +00:00
Conversation 0 Commits 19 Files changed 58 +243 -107
18 changed files with 243 additions and 107 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit b1cfd16913 - Show all commits

1
Cargo.lock generated
View file

@ -403,6 +403,7 @@ dependencies = [
"rand", "rand",
"rmp-serde 0.15.5", "rmp-serde 0.15.5",
"serde", "serde",
"serde_bytes",
"sled", "sled",
"structopt", "structopt",
"tokio", "tokio",

8
src/api/api_server.rs
View file

@ -109,11 +109,9 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
let endpoint = Endpoint::from_request(&req, bucket.map(ToOwned::to_owned))?; let endpoint = Endpoint::from_request(&req, bucket.map(ToOwned::to_owned))?;
let bucket_name = match endpoint.authorization_type() { let bucket_name = match endpoint.get_bucket() {
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-16 19:34:55 +00:00
Outdated
Review
Copy link

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
Authorization::None => { None => return handle_request_without_bucket(garage, req, api_key, endpoint).await,
return handle_request_without_bucket(garage, req, api_key, endpoint).await Some(bucket) => bucket.to_string(),
}
Authorization::Read(bucket) | Authorization::Write(bucket) => bucket.to_string(),
}; };
let bucket_id = resolve_bucket(&garage, &bucket_name, &api_key).await?; let bucket_id = resolve_bucket(&garage, &bucket_name, &api_key).await?;

2
src/api/s3_bucket.rs
View file

@ -67,7 +67,7 @@ pub async fn handle_list_buckets(garage: &Garage, api_key: &Key) -> Result<Respo
if let Some(alias_ent) = alias_ent { if let Some(alias_ent) = alias_ent {
if let Some(alias_p) = alias_ent.state.get().as_option() { if let Some(alias_p) = alias_ent.state.get().as_option() {
if alias_p.bucket_id == *bucket_id { if alias_p.bucket_id == *bucket_id {
aliases.insert(alias_ent.name.clone(), *bucket_id); aliases.insert(alias_ent.name().to_string(), *bucket_id);
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-18 16:42:19 +00:00
Outdated
Review
Copy link

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)`
} }
} }
} }

2
src/api/s3_website.rs
View file

@ -25,7 +25,6 @@ pub async fn handle_delete_website(
.ok_or(Error::NotFound)?; .ok_or(Error::NotFound)?;
if let crdt::Deletable::Present(param) = &mut bucket.state { if let crdt::Deletable::Present(param) = &mut bucket.state {
param.website_access.update(false);
param.website_config.update(None); param.website_config.update(None);
garage.bucket_table.insert(&bucket).await?; garage.bucket_table.insert(&bucket).await?;
} else { } else {
@ -57,7 +56,6 @@ pub async fn handle_put_website(
conf.validate()?; conf.validate()?;
if let crdt::Deletable::Present(param) = &mut bucket.state { if let crdt::Deletable::Present(param) = &mut bucket.state {
param.website_access.update(true);
param param
.website_config .website_config
.update(Some(ByteBuf::from(body.to_vec()))); .update(Some(ByteBuf::from(body.to_vec())));

1
src/garage/Cargo.toml
View file

@ -35,6 +35,7 @@ sled = "0.34"
rmp-serde = "0.15" rmp-serde = "0.15"
serde = { version = "1.0", default-features = false, features = ["derive", "rc"] } serde = { version = "1.0", default-features = false, features = ["derive", "rc"] }
serde_bytes = "0.11"
structopt = { version = "0.3", default-features = false } structopt = { version = "0.3", default-features = false }
toml = "0.5" toml = "0.5"

155
src/garage/admin.rs
View file

@ -4,6 +4,7 @@ use std::sync::Arc;
use async_trait::async_trait; use async_trait::async_trait;
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use serde_bytes::ByteBuf;
use garage_util::crdt::*; use garage_util::crdt::*;
use garage_util::data::*; use garage_util::data::*;
@ -27,6 +28,8 @@ use crate::repair::Repair;
pub const ADMIN_RPC_PATH: &str = "garage/admin_rpc.rs/Rpc"; pub const ADMIN_RPC_PATH: &str = "garage/admin_rpc.rs/Rpc";
macro_rules! INVALID_BUCKET_NAME_MESSAGE { () => { "Invalid bucket name: {}. See AWS documentation for constraints on S3 bucket names:\nhttps://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html" }; }
#[derive(Debug, Serialize, Deserialize)] #[derive(Debug, Serialize, Deserialize)]
pub enum AdminRpc { pub enum AdminRpc {
BucketOperation(BucketOperation), BucketOperation(BucketOperation),
@ -142,14 +145,14 @@ impl AdminRpcHandler {
})); }));
alias alias
} }
None => BucketAlias::new(name.clone(), bucket.id), None => BucketAlias::new(name.clone(), bucket.id)
.ok_or_message(format!(INVALID_BUCKET_NAME_MESSAGE!(), name))?,
}; };
bucket bucket.state.as_option_mut().unwrap().aliases.merge_raw(
.state name,
.as_option_mut() alias.state.timestamp(),
.unwrap() &true,
.aliases );
.update_in_place(name.clone(), true);
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
self.garage.bucket_alias_table.insert(&alias).await?; self.garage.bucket_alias_table.insert(&alias).await?;
Ok(AdminRpc::Ok(format!("Bucket {} was created.", name))) Ok(AdminRpc::Ok(format!("Bucket {} was created.", name)))
@ -222,7 +225,7 @@ impl AdminRpcHandler {
// 2. delete bucket alias // 2. delete bucket alias
bucket_alias.state.update(Deletable::Deleted); bucket_alias.state.update(Deletable::Deleted);
self.garage.bucket_alias_table.insert(&bucket_alias).await?; self.garage.bucket_alias_table.insert(&bucket_alias).await?;
// 3. delete bucket alias // 3. delete bucket
bucket.state = Deletable::delete(); bucket.state = Deletable::delete();
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
@ -259,15 +262,36 @@ impl AdminRpcHandler {
} }
} }
key_param.local_aliases = key_param if !is_valid_bucket_name(&query.new_name) {
.local_aliases return Err(Error::Message(format!(
.update_mutator(query.new_name.clone(), Deletable::present(bucket_id)); INVALID_BUCKET_NAME_MESSAGE!(),
query.new_name
)));
}
// Checks ok, add alias
let mut bucket_p = bucket.state.as_option_mut().unwrap();
let bucket_p_local_alias_key = (key.key_id.clone(), query.new_name.clone());
// Calculate the timestamp to assign to this aliasing in the two local_aliases maps
// (the one from key to bucket, and the reverse one stored in the bucket iself)
// so that merges on both maps in case of a concurrent operation resolve
// to the same alias being set
let alias_ts = increment_logical_clock_2(
key_param.local_aliases.get_timestamp(&query.new_name),
bucket_p
.local_aliases
.get_timestamp(&bucket_p_local_alias_key),
);
key_param.local_aliases = LwwMap::raw_item(
query.new_name.clone(),
alias_ts,
Deletable::present(bucket_id),
);
self.garage.key_table.insert(&key).await?; self.garage.key_table.insert(&key).await?;
let mut bucket_p = bucket.state.as_option_mut().unwrap(); bucket_p.local_aliases = LwwMap::raw_item(bucket_p_local_alias_key, alias_ts, true);
bucket_p.local_aliases = bucket_p
.local_aliases
.update_mutator((key.key_id.clone(), query.new_name.clone()), true);
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
Ok(AdminRpc::Ok(format!( Ok(AdminRpc::Ok(format!(
@ -275,40 +299,47 @@ impl AdminRpcHandler {
query.new_name, bucket_id, key.key_id query.new_name, bucket_id, key.key_id
))) )))
} else { } else {
let mut alias = self let alias = self
.garage .garage
.bucket_alias_table .bucket_alias_table
.get(&EmptyKey, &query.new_name) .get(&EmptyKey, &query.new_name)
.await? .await?;
.unwrap_or(BucketAlias {
name: query.new_name.clone(),
state: Lww::new(Deletable::delete()),
});
if let Some(existing_alias) = alias.state.get().as_option() { if let Some(existing_alias) = alias.as_ref() {
if existing_alias.bucket_id == bucket_id { if let Some(p) = existing_alias.state.get().as_option() {
return Ok(AdminRpc::Ok(format!( if p.bucket_id == bucket_id {
"Alias {} already points to bucket {:?}", return Ok(AdminRpc::Ok(format!(
query.new_name, bucket_id "Alias {} already points to bucket {:?}",
))); query.new_name, bucket_id
} else { )));
return Err(Error::Message(format!( } else {
"Alias {} already exists and points to different bucket: {:?}", return Err(Error::Message(format!(
query.new_name, existing_alias.bucket_id "Alias {} already exists and points to different bucket: {:?}",
))); query.new_name, p.bucket_id
)));
}
} }
} }
// Checks ok, add alias // Checks ok, add alias
alias let mut bucket_p = bucket.state.as_option_mut().unwrap();
.state
.update(Deletable::present(AliasParams { bucket_id })); let alias_ts = increment_logical_clock_2(
bucket_p.aliases.get_timestamp(&query.new_name),
alias.as_ref().map(|a| a.state.timestamp()).unwrap_or(0),
);
let alias = match alias {
None => BucketAlias::new(query.new_name.clone(), bucket_id)
.ok_or_message(format!(INVALID_BUCKET_NAME_MESSAGE!(), query.new_name))?,
Some(mut a) => {
a.state = Lww::raw(alias_ts, Deletable::present(AliasParams { bucket_id }));
a
}
};
self.garage.bucket_alias_table.insert(&alias).await?; self.garage.bucket_alias_table.insert(&alias).await?;
let mut bucket_p = bucket.state.as_option_mut().unwrap(); bucket_p.aliases = LwwMap::raw_item(query.new_name.clone(), alias_ts, true);
bucket_p.aliases = bucket_p
.aliases
.update_mutator(query.new_name.clone(), true);
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
Ok(AdminRpc::Ok(format!( Ok(AdminRpc::Ok(format!(
@ -336,14 +367,14 @@ impl AdminRpcHandler {
.bucket_helper() .bucket_helper()
.get_existing_bucket(bucket_id) .get_existing_bucket(bucket_id)
.await?; .await?;
let mut bucket_state = bucket.state.as_option_mut().unwrap(); let mut bucket_p = bucket.state.as_option_mut().unwrap();
let has_other_aliases = bucket_state let has_other_aliases = bucket_p
.aliases .aliases
.items() .items()
.iter() .iter()
.any(|(_, _, active)| *active) .any(|(_, _, active)| *active)
|| bucket_state || bucket_p
.local_aliases .local_aliases
.items() .items()
.iter() .iter()
@ -352,15 +383,22 @@ impl AdminRpcHandler {
return Err(Error::Message(format!("Bucket {} doesn't have other aliases, please delete it instead of just unaliasing.", query.name))); return Err(Error::Message(format!("Bucket {} doesn't have other aliases, please delete it instead of just unaliasing.", query.name)));
} }
// Checks ok, remove alias
let mut key_param = key.state.as_option_mut().unwrap(); let mut key_param = key.state.as_option_mut().unwrap();
key_param.local_aliases = key_param let bucket_p_local_alias_key = (key.key_id.clone(), query.name.clone());
.local_aliases
.update_mutator(query.name.clone(), Deletable::delete()); let alias_ts = increment_logical_clock_2(
key_param.local_aliases.get_timestamp(&query.name),
bucket_p
.local_aliases
.get_timestamp(&bucket_p_local_alias_key),
);
key_param.local_aliases =
LwwMap::raw_item(query.name.clone(), alias_ts, Deletable::delete());
self.garage.key_table.insert(&key).await?; self.garage.key_table.insert(&key).await?;
bucket_state.local_aliases = bucket_state bucket_p.local_aliases = LwwMap::raw_item(bucket_p_local_alias_key, alias_ts, false);
.local_aliases
.update_mutator((key.key_id.clone(), query.name.clone()), false);
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
Ok(AdminRpc::Ok(format!( Ok(AdminRpc::Ok(format!(
@ -401,12 +439,17 @@ impl AdminRpcHandler {
.get(&EmptyKey, &query.name) .get(&EmptyKey, &query.name)
.await? .await?
.ok_or_message("Internal error: alias not found")?; .ok_or_message("Internal error: alias not found")?;
alias.state.update(Deletable::delete());
// Checks ok, remove alias
let alias_ts = increment_logical_clock_2(
alias.state.timestamp(),
bucket_state.aliases.get_timestamp(&query.name),
);
alias.state = Lww::raw(alias_ts, Deletable::delete());
self.garage.bucket_alias_table.insert(&alias).await?; self.garage.bucket_alias_table.insert(&alias).await?;
bucket_state.aliases = bucket_state bucket_state.aliases = LwwMap::raw_item(query.name.clone(), alias_ts, false);
.aliases
.update_mutator(query.name.clone(), false);
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
Ok(AdminRpc::Ok(format!("Bucket alias {} deleted", query.name))) Ok(AdminRpc::Ok(format!("Bucket alias {} deleted", query.name)))
@ -494,7 +537,13 @@ impl AdminRpcHandler {
)); ));
} }
bucket_state.website_access.update(query.allow); let website = if query.allow {
Some(ByteBuf::from(DEFAULT_WEBSITE_CONFIGURATION.to_vec()))
} else {
None
};
bucket_state.website_config.update(website);
self.garage.bucket_table.insert(&bucket).await?; self.garage.bucket_table.insert(&bucket).await?;
let msg = if query.allow { let msg = if query.allow {

2
src/garage/cli/cmd.rs
View file

@ -167,7 +167,7 @@ pub async fn cmd_admin(
let mut table = vec![]; let mut table = vec![];
for alias in bl { for alias in bl {
if let Some(p) = alias.state.get().as_option() { if let Some(p) = alias.state.get().as_option() {
table.push(format!("\t{}\t{:?}", alias.name, p.bucket_id)); table.push(format!("\t{}\t{:?}", alias.name(), p.bucket_id));
} }
} }
format_table(table); format_table(table);

2
src/garage/cli/util.rs
View file

@ -82,7 +82,7 @@ pub fn print_bucket_info(bucket: &Bucket, relevant_keys: &HashMap<String, Key>)
match &bucket.state { match &bucket.state {
Deletable::Deleted => println!("Bucket is deleted."), Deletable::Deleted => println!("Bucket is deleted."),
Deletable::Present(p) => { Deletable::Present(p) => {
println!("Website access: {}", p.website_access.get()); println!("Website access: {}", p.website_config.get().is_some());
println!("\nGlobal aliases:"); println!("\nGlobal aliases:");
for (alias, _, active) in p.aliases.items().iter() { for (alias, _, active) in p.aliases.items().iter() {

43
src/model/bucket_alias_table.rs
View file

@ -8,7 +8,7 @@ use garage_util::data::*;
/// in the global namespace. /// in the global namespace.
#[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
pub struct BucketAlias { pub struct BucketAlias {
pub name: String, name: String,
pub state: crdt::Lww<crdt::Deletable<AliasParams>>, pub state: crdt::Lww<crdt::Deletable<AliasParams>>,
} }
@ -22,15 +22,22 @@ impl AutoCrdt for AliasParams {
} }
impl BucketAlias { impl BucketAlias {
pub fn new(name: String, bucket_id: Uuid) -> Self { pub fn new(name: String, bucket_id: Uuid) -> Option<Self> {
BucketAlias { if !is_valid_bucket_name(&name) {
name, None
state: crdt::Lww::new(crdt::Deletable::present(AliasParams { bucket_id })), } else {
Some(BucketAlias {
name,
state: crdt::Lww::new(crdt::Deletable::present(AliasParams { bucket_id })),
})
} }
} }
pub fn is_deleted(&self) -> bool { pub fn is_deleted(&self) -> bool {
self.state.get().is_deleted() self.state.get().is_deleted()
} }
pub fn name(&self) -> &str {
&self.name
}
} }
impl Crdt for BucketAlias { impl Crdt for BucketAlias {
@ -62,3 +69,29 @@ impl TableSchema for BucketAliasTable {
filter.apply(entry.is_deleted()) filter.apply(entry.is_deleted())
} }
} }
/// Check if a bucket name is valid.
///
/// The requirements are listed here:
///
/// <https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html>
///
/// In the case of Garage, bucket names must not be hex-encoded
/// 32 byte string, which is excluded thanks to the
/// maximum length of 63 bytes given in the spec.
pub fn is_valid_bucket_name(n: &str) -> bool {
// Bucket names must be between 3 and 63 characters
n.len() >= 3 && n.len() <= 63
// Bucket names must be composed of lowercase letters, numbers,
// dashes and dots
&& n.chars().all(|c| matches!(c, '.' | '-' | 'a'..='z' | '0'..='9'))
// Bucket names must start and end with a letter or a number
&& !n.starts_with(&['-', '.'][..])
&& !n.ends_with(&['-', '.'][..])
// Bucket names must not be formated as an IP address
&& n.parse::<std::net::IpAddr>().is_err()
// Bucket names must not start wih "xn--"
&& !n.starts_with("xn--")
// Bucket names must not end with "-s3alias"
&& !n.ends_with("-s3alias")
}

12
src/model/bucket_helper.rs
View file

@ -8,12 +8,21 @@ use crate::garage::Garage;
pub struct BucketHelper<'a>(pub(crate) &'a Garage); pub struct BucketHelper<'a>(pub(crate) &'a Garage);
#[allow(clippy::ptr_arg)]
impl<'a> BucketHelper<'a> { impl<'a> BucketHelper<'a> {
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-16 23:25:40 +00:00
Outdated
Review
Copy link

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

I think this link could be one line lower to only cover the "offending" function
#[allow(clippy::ptr_arg)]
pub async fn resolve_global_bucket_name( pub async fn resolve_global_bucket_name(
&self, &self,
bucket_name: &String, bucket_name: &String,
) -> Result<Option<Uuid>, Error> { ) -> Result<Option<Uuid>, Error> {
// Bucket names in Garage are aliases, true bucket identifiers
// are 32-byte UUIDs. This function resolves bucket names into
// their full identifier by looking up in the bucket_alias_table.
// This function also allows buckets to be identified by their
// full UUID (hex-encoded). Here, if the name to be resolved is a
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-16 23:24:25 +00:00
Outdated
Review
Copy link

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

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
Outdated
Review
Copy link

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.

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
Outdated
Review
Copy link

could you add a comment saying that?

could you add a comment saying that?
// hex string of the correct length, it is directly parsed as a bucket
// identifier which is returned. There is no risk of this conflicting
// with an actual bucket name: bucket names are max 63 chars long by
// the AWS spec, and hex-encoded UUIDs are 64 chars long.
let hexbucket = hex::decode(bucket_name.as_str()) let hexbucket = hex::decode(bucket_name.as_str())
.ok() .ok()
.map(|by| Uuid::try_from(&by)) .map(|by| Uuid::try_from(&by))
@ -37,7 +46,6 @@ impl<'a> BucketHelper<'a> {
} }
} }
#[allow(clippy::ptr_arg)]
pub async fn get_existing_bucket(&self, bucket_id: Uuid) -> Result<Bucket, Error> { pub async fn get_existing_bucket(&self, bucket_id: Uuid) -> Result<Bucket, Error> {
self.0 self.0
.bucket_table .bucket_table

9
src/model/bucket_table.rs
View file

@ -8,6 +8,8 @@ use garage_util::time::*;
use crate::permission::BucketKeyPerm; use crate::permission::BucketKeyPerm;
pub const DEFAULT_WEBSITE_CONFIGURATION: &[u8] = b""; // TODO (an XML WebsiteConfiguration document per the AWS spec)
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-18 15:48:44 +00:00
Outdated
Review
Copy link

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)
/// A bucket is a collection of objects /// A bucket is a collection of objects
/// ///
/// Its parameters are not directly accessible as: /// Its parameters are not directly accessible as:
@ -29,9 +31,8 @@ pub struct BucketParams {
/// Map of key with access to the bucket, and what kind of access they give /// Map of key with access to the bucket, and what kind of access they give
pub authorized_keys: crdt::Map<String, BucketKeyPerm>, pub authorized_keys: crdt::Map<String, BucketKeyPerm>,
/// Whether this bucket is allowed for website access /// Whether this bucket is allowed for website access
/// (under all of its global alias names) /// (under all of its global alias names),
pub website_access: crdt::Lww<bool>, /// and if so, the website configuration XML document
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-16 21:06:41 +00:00
Outdated
Review
Copy link

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
Outdated
Review
Copy link

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).
/// The website configuration XML document
pub website_config: crdt::Lww<Option<ByteBuf>>, pub website_config: crdt::Lww<Option<ByteBuf>>,
/// Map of aliases that are or have been given to this bucket /// Map of aliases that are or have been given to this bucket
/// in the global namespace /// in the global namespace
@ -50,7 +51,6 @@ impl BucketParams {
BucketParams { BucketParams {
creation_date: now_msec(), creation_date: now_msec(),
authorized_keys: crdt::Map::new(), authorized_keys: crdt::Map::new(),
website_access: crdt::Lww::new(false),
website_config: crdt::Lww::new(None), website_config: crdt::Lww::new(None),
aliases: crdt::LwwMap::new(), aliases: crdt::LwwMap::new(),
local_aliases: crdt::LwwMap::new(), local_aliases: crdt::LwwMap::new(),
@ -61,7 +61,6 @@ impl BucketParams {
impl Crdt for BucketParams { impl Crdt for BucketParams {
fn merge(&mut self, o: &Self) { fn merge(&mut self, o: &Self) {
self.authorized_keys.merge(&o.authorized_keys); self.authorized_keys.merge(&o.authorized_keys);
self.website_access.merge(&o.website_access);
self.website_config.merge(&o.website_config); self.website_config.merge(&o.website_config);
self.aliases.merge(&o.aliases); self.aliases.merge(&o.aliases);
self.local_aliases.merge(&o.local_aliases); self.local_aliases.merge(&o.local_aliases);

2
src/model/key_table.rs
View file

@ -190,7 +190,7 @@ impl TableSchema for KeyTable {
local_aliases: crdt::LwwMap::new(), local_aliases: crdt::LwwMap::new(),
}) })
}; };
let name = crdt::Lww::migrate_from_raw(old_k.name.timestamp(), old_k.name.get().clone()); let name = crdt::Lww::raw(old_k.name.timestamp(), old_k.name.get().clone());
Some(Key { Some(Key {
key_id: old_k.key_id, key_id: old_k.key_id,
secret_key: old_k.secret_key, secret_key: old_k.secret_key,

32
src/model/migrate.rs
View file

@ -1,5 +1,7 @@
use std::sync::Arc; use std::sync::Arc;
use serde_bytes::ByteBuf;
use garage_table::util::EmptyKey; use garage_table::util::EmptyKey;
use garage_util::crdt::*; use garage_util::crdt::*;
use garage_util::data::*; use garage_util::data::*;
@ -38,6 +40,16 @@ impl Migrate {
old_bucket: &old_bucket::Bucket, old_bucket: &old_bucket::Bucket,
old_bucket_p: &old_bucket::BucketParams, old_bucket_p: &old_bucket::BucketParams,
) -> Result<(), Error> { ) -> Result<(), Error> {
let bucket_id = blake2sum(old_bucket.name.as_bytes());
let new_name = if is_valid_bucket_name(&old_bucket.name) {
old_bucket.name.clone()
} else {
// if old bucket name was not valid, replace it by
// a hex-encoded name derived from its identifier
hex::encode(&bucket_id.as_slice()[..16])
};
let mut new_ak = Map::new(); let mut new_ak = Map::new();
for (k, ts, perm) in old_bucket_p.authorized_keys.items().iter() { for (k, ts, perm) in old_bucket_p.authorized_keys.items().iter() {
new_ak.put( new_ak.put(
@ -52,27 +64,27 @@ impl Migrate {
} }
let mut aliases = LwwMap::new(); let mut aliases = LwwMap::new();
aliases.update_in_place(old_bucket.name.clone(), true); aliases.update_in_place(new_name.clone(), true);
let website = if *old_bucket_p.website.get() {
Some(ByteBuf::from(DEFAULT_WEBSITE_CONFIGURATION.to_vec()))
} else {
None
};
let new_bucket = Bucket { let new_bucket = Bucket {
id: blake2sum(old_bucket.name.as_bytes()), id: bucket_id,
state: Deletable::Present(BucketParams { state: Deletable::Present(BucketParams {
creation_date: now_msec(), creation_date: now_msec(),
authorized_keys: new_ak.clone(), authorized_keys: new_ak.clone(),
website_access: Lww::new(*old_bucket_p.website.get()), website_config: Lww::new(website),
website_config: Lww::new(None),
aliases, aliases,
local_aliases: LwwMap::new(), local_aliases: LwwMap::new(),
}), }),
}; };
self.garage.bucket_table.insert(&new_bucket).await?; self.garage.bucket_table.insert(&new_bucket).await?;
let new_alias = BucketAlias { let new_alias = BucketAlias::new(new_name.clone(), new_bucket.id).unwrap();
name: old_bucket.name.clone(),
state: Lww::new(Deletable::Present(AliasParams {
bucket_id: new_bucket.id,
})),
};
self.garage.bucket_alias_table.insert(&new_alias).await?; self.garage.bucket_alias_table.insert(&new_alias).await?;
for (k, perm) in new_ak.items().iter() { for (k, perm) in new_ak.items().iter() {

3
src/model/permission.rs
View file

@ -34,6 +34,9 @@ impl Crdt for BucketKeyPerm {
if !other.allow_write { if !other.allow_write {
self.allow_write = false; self.allow_write = false;
} }
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-16 20:47:39 +00:00
Outdated
Review
Copy link

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

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
Outdated
Review
Copy link

Just forgotten, thx

Just forgotten, thx
if !other.allow_owner {
self.allow_owner = false;
}
} }
_ => (), _ => (),
} }

7
src/util/crdt/lww.rs
View file

@ -61,7 +61,7 @@ where
/// ///
/// Compared to new, the CRDT's timestamp is not set to now /// Compared to new, the CRDT's timestamp is not set to now
/// but must be set to the previous, non-compatible, CRDT's timestamp. /// but must be set to the previous, non-compatible, CRDT's timestamp.
pub fn migrate_from_raw(ts: u64, value: T) -> Self { pub fn raw(ts: u64, value: T) -> Self {
Self { ts, v: value } Self { ts, v: value }
} }
@ -77,6 +77,11 @@ where
self.v = new_value; self.v = new_value;
} }
/// Get the timestamp currently associated with the value
pub fn timestamp(&self) -> u64 {
self.ts
}
/// Get the CRDT value /// Get the CRDT value
pub fn get(&self) -> &T { pub fn get(&self) -> &T {
&self.v &self.v

62
src/util/crdt/lww_map.rs
View file

@ -37,11 +37,12 @@ where
pub fn new() -> Self { pub fn new() -> Self {
Self { vals: vec![] } Self { vals: vec![] }
} }
/// Used to migrate from a map defined in an incompatible format. This produces /// Used to migrate from a map defined in an incompatible format. This produces
/// a map that contains a single item with the specified timestamp (copied from /// a map that contains a single item with the specified timestamp (copied from
/// the incompatible format). Do this as many times as you have items to migrate, /// the incompatible format). Do this as many times as you have items to migrate,
/// and put them all together using the CRDT merge operator. /// and put them all together using the CRDT merge operator.
pub fn migrate_from_raw_item(k: K, ts: u64, v: V) -> Self { pub fn raw_item(k: K, ts: u64, v: V) -> Self {
Self { Self {
vals: vec![(k, ts, v)], vals: vec![(k, ts, v)],
} }
@ -74,9 +75,37 @@ where
Self { vals: new_vals } Self { vals: new_vals }
} }
/// Updates a value in place in the map (this generates
/// a new timestamp)
pub fn update_in_place(&mut self, k: K, new_v: V) { pub fn update_in_place(&mut self, k: K, new_v: V) {
self.merge(&self.update_mutator(k, new_v)); self.merge(&self.update_mutator(k, new_v));
} }
/// Updates a value in place in the map, from a
/// (key, timestamp, value) triple, only if the given
/// timestamp is larger than the timestamp currently
/// in the map
pub fn merge_raw(&mut self, k: &K, ts2: u64, v2: &V) {
match self.vals.binary_search_by(|(k2, _, _)| k2.cmp(k)) {
Ok(i) => {
let (_, ts1, _v1) = &self.vals[i];
match ts2.cmp(ts1) {
Ordering::Greater => {
self.vals[i].1 = ts2;
self.vals[i].2 = v2.clone();
}
Ordering::Equal => {
self.vals[i].2.merge(v2);
}
Ordering::Less => (),
}
}
Err(i) => {
self.vals.insert(i, (k.clone(), ts2, v2.clone()));
}
}
}
/// Takes all of the values of the map and returns them. The current map is reset to the /// Takes all of the values of the map and returns them. The current map is reset to the
/// empty map. This is very usefull to produce in-place a new map that contains only a delta /// empty map. This is very usefull to produce in-place a new map that contains only a delta
/// that modifies a certain value: /// that modifies a certain value:
@ -103,10 +132,12 @@ where
let vals = std::mem::take(&mut self.vals); let vals = std::mem::take(&mut self.vals);
Self { vals } Self { vals }
} }
/// Removes all values from the map /// Removes all values from the map
pub fn clear(&mut self) { pub fn clear(&mut self) {
self.vals.clear(); self.vals.clear();
} }
/// Get a reference to the value assigned to a key /// Get a reference to the value assigned to a key
pub fn get(&self, k: &K) -> Option<&V> { pub fn get(&self, k: &K) -> Option<&V> {
match self.vals.binary_search_by(|(k2, _, _)| k2.cmp(k)) { match self.vals.binary_search_by(|(k2, _, _)| k2.cmp(k)) {
@ -114,6 +145,16 @@ where
Err(_) => None, Err(_) => None,
} }
} }
/// Get the timestamp of the value assigned to a key, or 0 if
/// no value is assigned
pub fn get_timestamp(&self, k: &K) -> u64 {
match self.vals.binary_search_by(|(k2, _, _)| k2.cmp(k)) {
Ok(i) => self.vals[i].1,
Err(_) => 0,
}
}
/// Gets a reference to all of the items, as a slice. Usefull to iterate on all map values. /// Gets a reference to all of the items, as a slice. Usefull to iterate on all map values.
/// In most case you will want to ignore the timestamp (second item of the tuple). /// In most case you will want to ignore the timestamp (second item of the tuple).
pub fn items(&self) -> &[(K, u64, V)] { pub fn items(&self) -> &[(K, u64, V)] {
@ -138,24 +179,7 @@ where
{ {
fn merge(&mut self, other: &Self) { fn merge(&mut self, other: &Self) {
for (k, ts2, v2) in other.vals.iter() { for (k, ts2, v2) in other.vals.iter() {
match self.vals.binary_search_by(|(k2, _, _)| k2.cmp(k)) { self.merge_raw(k, *ts2, v2);
Ok(i) => {
let (_, ts1, _v1) = &self.vals[i];
match ts2.cmp(ts1) {
Ordering::Greater => {
self.vals[i].1 = *ts2;
self.vals[i].2 = v2.clone();
}
Ordering::Equal => {
self.vals[i].2.merge(v2);
}
Ordering::Less => (),
}
}
Err(i) => {
self.vals.insert(i, (k.clone(), *ts2, v2.clone()));
}
}
} }
} }
} }

5
src/util/time.rs
View file

@ -15,6 +15,11 @@ pub fn increment_logical_clock(prev: u64) -> u64 {
std::cmp::max(prev + 1, now_msec()) std::cmp::max(prev + 1, now_msec())
} }
/// Increment two logical clocks
pub fn increment_logical_clock_2(prev: u64, prev2: u64) -> u64 {
std::cmp::max(prev2 + 1, std::cmp::max(prev + 1, now_msec()))
}
/// Convert a timestamp represented as milliseconds since UNIX Epoch to /// Convert a timestamp represented as milliseconds since UNIX Epoch to
/// its RFC3339 representation, such as "2021-01-01T12:30:00Z" /// its RFC3339 representation, such as "2021-01-01T12:30:00Z"
pub fn msec_to_rfc3339(msecs: u64) -> String { pub fn msec_to_rfc3339(msecs: u64) -> String {

2
src/web/web_server.rs
View file

@ -99,7 +99,7 @@ async fn serve_file(garage: Arc<Garage>, req: Request<Body>) -> Result<Response<
.filter(|b| { .filter(|b| {
b.state b.state
.as_option() .as_option()
.map(|x| *x.website_access.get()) .map(|x| x.website_config.get().is_some())
.unwrap_or(false) .unwrap_or(false)
}) })
.ok_or(Error::NotFound)?; .ok_or(Error::NotFound)?;