Deuxfleurs/garage
41
56
Fork 73
Code Issues 99 Pull requests 14 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 +119 -57
14 changed files with 119 additions and 57 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 0bbb6673e7 - Show all commits

1
Cargo.lock generated
View file

@ -436,6 +436,7 @@ dependencies = [
"quick-xml",
"roxmltree",
"serde",
"serde_bytes",
"sha2",
"tokio",
"url",

1
src/api/Cargo.toml
View file

@ -41,5 +41,6 @@ hyper = { version = "0.14", features = ["server", "http1", "runtime", "tcp", "st
percent-encoding = "2.1.0"
roxmltree = "0.14"
serde = { version = "1.0", features = ["derive"] }
serde_bytes = "0.11"
quick-xml = { version = "0.21", features = [ "serialize" ] }
url = "2.1"

6
src/api/api_server.rs
View file

@ -277,10 +277,10 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
Endpoint::DeleteObjects { .. } => {
handle_delete_objects(garage, bucket_id, req, content_sha256).await
}
Endpoint::PutBucketWebsite { bucket } => {
handle_put_website(garage, bucket, req, content_sha256).await
Endpoint::PutBucketWebsite { .. } => {
handle_put_website(garage, bucket_id, req, content_sha256).await
}
Endpoint::DeleteBucketWebsite { bucket } => handle_delete_website(garage, bucket).await,
Endpoint::DeleteBucketWebsite { .. } => handle_delete_website(garage, bucket_id).await,
endpoint => Err(Error::NotImplemented(endpoint.name().to_owned())),
}
}

35
src/api/s3_website.rs
View file

@ -3,6 +3,7 @@ use std::sync::Arc;
use hyper::{Body, Request, Response, StatusCode};
use serde::{Deserialize, Serialize};
use serde_bytes::ByteBuf;
use crate::error::*;
use crate::s3_xml::{xmlns_tag, IntValue, Value};
@ -11,23 +12,22 @@ use crate::signature::verify_signed_content;
use garage_model::garage::Garage;
use garage_table::*;
use garage_util::crdt;
use garage_util::data::Hash;
use garage_util::data::*;
pub async fn handle_delete_website(
garage: Arc<Garage>,
bucket: String,
bucket_id: Uuid,
) -> Result<Response<Body>, Error> {
let mut bucket = garage
.bucket_alias_table
.get(&EmptyKey, &bucket)
.bucket_table
.get(&bucket_id, &EmptyKey)
.await?
.ok_or(Error::NotFound)?;
if let crdt::Deletable::Present(state) = bucket.state.get_mut() {
let mut new_param = state.clone();
new_param.website_access = false;
bucket.state.update(crdt::Deletable::present(new_param));
garage.bucket_alias_table.insert(&bucket).await?;
if let crdt::Deletable::Present(param) = &mut bucket.state {
param.website_access.update(false);
param.website_config.update(None);
garage.bucket_table.insert(&bucket).await?;
} else {
unreachable!();
}
@ -40,7 +40,7 @@ pub async fn handle_delete_website(
pub async fn handle_put_website(
garage: Arc<Garage>,
bucket: String,
bucket_id: Uuid,
req: Request<Body>,
content_sha256: Option<Hash>,
) -> Result<Response<Body>, Error> {
@ -48,19 +48,20 @@ pub async fn handle_put_website(
verify_signed_content(content_sha256, &body[..])?;
let mut bucket = garage
.bucket_alias_table
.get(&EmptyKey, &bucket)
.bucket_table
.get(&bucket_id, &EmptyKey)
.await?
.ok_or(Error::NotFound)?;
let conf: WebsiteConfiguration = from_reader(&body as &[u8])?;
conf.validate()?;
if let crdt::Deletable::Present(state) = bucket.state.get() {
let mut new_param = state.clone();
new_param.website_access = true;
bucket.state.update(crdt::Deletable::present(new_param));
garage.bucket_alias_table.insert(&bucket).await?;
if let crdt::Deletable::Present(param) = &mut bucket.state {
param.website_access.update(true);
param
.website_config
.update(Some(ByteBuf::from(body.to_vec())));
garage.bucket_table.insert(&bucket).await?;
} else {
unreachable!();
}

51
src/garage/admin.rs
View file

@ -104,11 +104,10 @@ impl AdminRpcHandler {
}
alias.state.update(Deletable::Present(AliasParams {
bucket_id: bucket.id,
website_access: false,
}));
alias
}
None => BucketAlias::new(name.clone(), bucket.id, false),
None => BucketAlias::new(name.clone(), bucket.id),
};
bucket
.state
@ -178,7 +177,7 @@ impl AdminRpcHandler {
for (key_id, _) in bucket.authorized_keys() {
if let Some(key) = self.garage.key_table.get(&EmptyKey, key_id).await? {
if !key.state.is_deleted() {
self.update_key_bucket(&key, bucket.id, false, false)
self.update_key_bucket(&key, bucket.id, false, false, false)
.await?;
}
} else {
@ -266,10 +265,9 @@ impl AdminRpcHandler {
}
// Checks ok, add alias
alias.state.update(Deletable::present(AliasParams {
bucket_id,
website_access: false,
}));
alias
.state
.update(Deletable::present(AliasParams { bucket_id }));
self.garage.bucket_alias_table.insert(&alias).await?;
let mut bucket_p = bucket.state.as_option_mut().unwrap();
@ -396,16 +394,17 @@ impl AdminRpcHandler {
let allow_read = query.read || key.allow_read(&bucket_id);
let allow_write = query.write || key.allow_write(&bucket_id);
let allow_owner = query.owner || key.allow_owner(&bucket_id);
let new_perm = self
.update_key_bucket(&key, bucket_id, allow_read, allow_write)
.update_key_bucket(&key, bucket_id, allow_read, allow_write, allow_owner)
.await?;
self.update_bucket_key(bucket, &key.key_id, new_perm)
.await?;
Ok(AdminRpc::Ok(format!(
"New permissions for {} on {}: read {}, write {}.",
&key.key_id, &query.bucket, allow_read, allow_write
"New permissions for {} on {}: read {}, write {}, owner {}.",
&key.key_id, &query.bucket, allow_read, allow_write, allow_owner
)))
}
@ -425,29 +424,34 @@ impl AdminRpcHandler {
let allow_read = !query.read && key.allow_read(&bucket_id);
let allow_write = !query.write && key.allow_write(&bucket_id);
let allow_owner = !query.owner && key.allow_owner(&bucket_id);
let new_perm = self
.update_key_bucket(&key, bucket_id, allow_read, allow_write)
.update_key_bucket(&key, bucket_id, allow_read, allow_write, allow_owner)
.await?;
self.update_bucket_key(bucket, &key.key_id, new_perm)
.await?;
Ok(AdminRpc::Ok(format!(
"New permissions for {} on {}: read {}, write {}.",
&key.key_id, &query.bucket, allow_read, allow_write
"New permissions for {} on {}: read {}, write {}, owner {}.",
&key.key_id, &query.bucket, allow_read, allow_write, allow_owner
)))
}
async fn handle_bucket_website(&self, query: &WebsiteOpt) -> Result<AdminRpc, Error> {
let mut bucket_alias = self
let bucket_id = self
.garage
.bucket_alias_table
.get(&EmptyKey, &query.bucket)
.bucket_helper()
.resolve_global_bucket_name(&query.bucket)
.await?
.filter(|a| !a.is_deleted())
.ok_or_message(format!("Bucket {} does not exist", query.bucket))?;
.ok_or_message("Bucket not found")?;
let mut state = bucket_alias.state.get().as_option().unwrap().clone();
let mut bucket = self
.garage
.bucket_helper()
.get_existing_bucket(bucket_id)
.await?;
let bucket_state = bucket.state.as_option_mut().unwrap();
if !(query.allow ^ query.deny) {
return Err(Error::Message(
@ -455,9 +459,8 @@ impl AdminRpcHandler {
));
}
state.website_access = query.allow;
bucket_alias.state.update(Deletable::present(state));
self.garage.bucket_alias_table.insert(&bucket_alias).await?;
bucket_state.website_access.update(query.allow);
self.garage.bucket_table.insert(&bucket).await?;
let msg = if query.allow {
format!("Website access allowed for {}", &query.bucket)
@ -545,6 +548,7 @@ impl AdminRpcHandler {
timestamp: increment_logical_clock(auth.timestamp),
allow_read: false,
allow_write: false,
allow_owner: false,
};
if !bucket.is_deleted() {
self.update_bucket_key(bucket, &key.key_id, new_perm)
@ -605,6 +609,7 @@ impl AdminRpcHandler {
bucket_id: Uuid,
allow_read: bool,
allow_write: bool,
allow_owner: bool,
) -> Result<BucketKeyPerm, Error> {
let mut key = key.clone();
let mut key_state = key.state.as_option_mut().unwrap();
@ -617,11 +622,13 @@ impl AdminRpcHandler {
timestamp: increment_logical_clock(old_perm.timestamp),
allow_read,
allow_write,
allow_owner,
})
.unwrap_or(BucketKeyPerm {
timestamp: now_msec(),
allow_read,
allow_write,
allow_owner,
});
key_state.authorized_buckets = Map::put_mutator(bucket_id, perm);

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

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

5
src/garage/cli/structs.rs
View file

@ -238,6 +238,11 @@ pub struct PermBucketOpt {
#[structopt(long = "write")]
pub write: bool,
/// Allow/deny administrative operations operations
/// (such as deleting bucket or changing bucket website configuration)
#[structopt(long = "owner")]
pub owner: bool,
/// Bucket name
pub bucket: String,
}

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

@ -11,6 +11,7 @@ pub fn print_key_info(key: &Key) {
println!("Secret key: {}", key.secret_key);
match &key.state {
Deletable::Present(p) => {
println!("Can create buckets: {}", p.allow_create_bucket.get());
println!("\nKey-specific bucket aliases:");
let mut table = vec![];
for (alias_name, _, alias) in p.local_aliases.items().iter() {
@ -25,7 +26,8 @@ pub fn print_key_info(key: &Key) {
for (b, perm) in p.authorized_buckets.items().iter() {
let rflag = if perm.allow_read { "R" } else { " " };
let wflag = if perm.allow_write { "W" } else { " " };
table.push(format!("\t{}{}\t{:?}", rflag, wflag, b));
let oflag = if perm.allow_owner { "O" } else { " " };
table.push(format!("\t{}{}{}\t{:?}", rflag, wflag, oflag, b));
}
format_table(table);
}
@ -58,7 +60,8 @@ pub fn print_bucket_info(bucket: &Bucket) {
for (k, perm) in p.authorized_keys.items().iter() {
let rflag = if perm.allow_read { "R" } else { " " };
let wflag = if perm.allow_write { "W" } else { " " };
println!("- {}{} {}", rflag, wflag, k);
let oflag = if perm.allow_owner { "O" } else { " " };
println!("- {}{}{} {}", rflag, wflag, oflag, k);
}
}
};

8
src/model/bucket_alias_table.rs
View file

@ -15,7 +15,6 @@ pub struct BucketAlias {
#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Debug, Serialize, Deserialize)]
pub struct AliasParams {
pub bucket_id: Uuid,
pub website_access: bool,
}
impl AutoCrdt for AliasParams {
@ -23,13 +22,10 @@ impl AutoCrdt for AliasParams {
}
impl BucketAlias {
pub fn new(name: String, bucket_id: Uuid, website_access: bool) -> Self {
pub fn new(name: String, bucket_id: Uuid) -> Self {
BucketAlias {
name,
state: crdt::Lww::new(crdt::Deletable::present(AliasParams {
bucket_id,
website_access,
})),
state: crdt::Lww::new(crdt::Deletable::present(AliasParams { bucket_id })),
}
}
pub fn is_deleted(&self) -> bool {

10
src/model/bucket_table.rs
View file

@ -1,4 +1,5 @@
use serde::{Deserialize, Serialize};
use serde_bytes::ByteBuf;
use garage_table::crdt::Crdt;
use garage_table::*;
@ -27,6 +28,11 @@ pub struct BucketParams {
pub creation_date: u64,
/// Map of key with access to the bucket, and what kind of access they give
pub authorized_keys: crdt::Map<String, BucketKeyPerm>,
/// Whether this bucket is allowed for website access
/// (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>>,
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).
/// Map of aliases that are or have been given to this bucket
/// in the global namespace
/// (not authoritative: this is just used as an indication to
@ -44,6 +50,8 @@ impl BucketParams {
BucketParams {
creation_date: now_msec(),
authorized_keys: crdt::Map::new(),
website_access: crdt::Lww::new(false),
website_config: crdt::Lww::new(None),
aliases: crdt::LwwMap::new(),
local_aliases: crdt::LwwMap::new(),
}
@ -53,6 +61,8 @@ impl BucketParams {
impl Crdt for BucketParams {
fn merge(&mut self, o: &Self) {
self.authorized_keys.merge(&o.authorized_keys);
self.website_access.merge(&o.website_access);
self.website_config.merge(&o.website_config);
self.aliases.merge(&o.aliases);
self.local_aliases.merge(&o.local_aliases);
}

16
src/model/key_table.rs
View file

@ -27,6 +27,7 @@ pub struct Key {
/// Configuration for a key
#[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
pub struct KeyParams {
pub allow_create_bucket: crdt::Lww<bool>,
pub authorized_buckets: crdt::Map<Uuid, BucketKeyPerm>,
pub local_aliases: crdt::LwwMap<String, crdt::Deletable<Uuid>>,
}
@ -34,6 +35,7 @@ pub struct KeyParams {
impl KeyParams {
pub fn new() -> Self {
KeyParams {
allow_create_bucket: crdt::Lww::new(false),
authorized_buckets: crdt::Map::new(),
local_aliases: crdt::LwwMap::new(),
}
@ -48,6 +50,7 @@ impl Default for KeyParams {
impl Crdt for KeyParams {
fn merge(&mut self, o: &Self) {
self.allow_create_bucket.merge(&o.allow_create_bucket);
self.authorized_buckets.merge(&o.authorized_buckets);
self.local_aliases.merge(&o.local_aliases);
}
@ -111,6 +114,19 @@ impl Key {
false
}
}
/// Check if `Key` is owner of bucket
pub fn allow_owner(&self, bucket: &Uuid) -> bool {
if let crdt::Deletable::Present(params) = &self.state {
params
.authorized_buckets
.get(bucket)
.map(|x| x.allow_owner)
.unwrap_or(false)
} else {
false
}
}
}
impl Entry<EmptyKey, String> for Key {

6
src/model/permission.rs
View file

@ -12,8 +12,12 @@ pub struct BucketKeyPerm {
/// The key can be used to read the bucket
pub allow_read: bool,
/// The key can be used to write in the bucket
/// The key can be used to write objects to the bucket
pub allow_write: bool,
/// The key can be used to control other aspects of the bucket:
/// - enable / disable website access
/// - delete bucket
pub allow_owner: bool,
}
impl Crdt for BucketKeyPerm {

11
src/util/crdt/crdt.rs
View file

@ -28,6 +28,17 @@ pub trait Crdt {
fn merge(&mut self, other: &Self);
}
impl<T> Crdt for Option<T>
where
T: Eq,
{
fn merge(&mut self, other: &Self) {
if self != other {
lx marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-16 20:28:04 +00:00
Outdated
Review
Copy link

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

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

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"
*self = None;
}
}
}
/// All types that implement `Ord` (a total order) can also implement a trivial CRDT
/// defined by the merge rule: `a ⊔ b = max(a, b)`. Implement this trait for your type
/// to enable this behavior.

16
src/web/web_server.rs
View file

@ -10,9 +10,13 @@ use hyper::{
};
use crate::error::*;
use garage_api::helpers::{authority_to_host, host_to_bucket};
use garage_api::s3_get::{handle_get, handle_head};
use garage_model::bucket_table::Bucket;
use garage_model::garage::Garage;
use garage_table::*;
use garage_util::error::Error as GarageError;
@ -84,16 +88,20 @@ async fn serve_file(garage: Arc<Garage>, req: Request<Body>) -> Result<Response<
.await?
.map(|x| x.state.take().into_option())
.flatten()
.filter(|param| param.website_access)
.map(|param| param.bucket_id)
.ok_or(Error::NotFound)?;
// Sanity check: check bucket isn't deleted
garage
// Check bucket isn't deleted and has website access enabled
let _: Bucket = garage
.bucket_table
.get(&bucket_id, &EmptyKey)
.await?
.filter(|b| !b.is_deleted())
.filter(|b| {
b.state
.as_option()
.map(|x| *x.website_access.get())
.unwrap_or(false)
})
.ok_or(Error::NotFound)?;
// Get path