From 1bcd6fabbdc0cd9dee88ba28daecb5339f2c13ec Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Mon, 3 Jan 2022 18:32:15 +0100 Subject: [PATCH] New buckets for 0.6.0: small changes - Fix bucket delete - fix merge of bucket creation date - Replace deletable with option in aliases Rationale: if two aliases point to conflicting bucket, resolving by making an arbitrary choice risks making data accessible when it shouldn't be. We'd rather resolve to deleting the alias until someone puts it back. --- src/api/api_server.rs | 4 +- src/api/s3_bucket.rs | 8 ++-- src/garage/admin.rs | 16 ++++--- src/garage/cli/cmd.rs | 4 +- src/garage/cli/util.rs | 4 +- src/model/bucket_alias_table.rs | 17 ++----- src/model/bucket_table.rs | 1 + src/model/helper/bucket.rs | 79 ++++++++++++++++++++++++--------- src/model/key_table.rs | 2 +- src/util/crdt/crdt.rs | 6 --- src/util/data.rs | 4 +- src/web/web_server.rs | 3 +- 12 files changed, 86 insertions(+), 62 deletions(-) diff --git a/src/api/api_server.rs b/src/api/api_server.rs index 42987e78..f5ebed37 100644 --- a/src/api/api_server.rs +++ b/src/api/api_server.rs @@ -7,7 +7,6 @@ use hyper::server::conn::AddrStream; use hyper::service::{make_service_fn, service_fn}; use hyper::{Body, Request, Response, Server}; -use garage_util::crdt; use garage_util::data::*; use garage_util::error::Error as GarageError; @@ -306,8 +305,7 @@ async fn resolve_bucket( .as_option() .ok_or_else(|| Error::Forbidden("Operation is not allowed for this key.".to_string()))?; - if let Some(crdt::Deletable::Present(bucket_id)) = api_key_params.local_aliases.get(bucket_name) - { + if let Some(Some(bucket_id)) = api_key_params.local_aliases.get(bucket_name) { Ok(*bucket_id) } else { Ok(garage diff --git a/src/api/s3_bucket.rs b/src/api/s3_bucket.rs index 785b89dd..24ec6b98 100644 --- a/src/api/s3_bucket.rs +++ b/src/api/s3_bucket.rs @@ -65,8 +65,8 @@ pub async fn handle_list_buckets(garage: &Garage, api_key: &Key) -> Result Result) { println!("\nKey-specific bucket aliases:"); let mut table = vec![]; for (alias_name, _, alias) in p.local_aliases.items().iter() { - if let Some(bucket_id) = alias.as_option() { + if let Some(bucket_id) = alias { table.push(format!( "\t{}\t{}\t{}", alias_name, @@ -55,7 +55,7 @@ pub fn print_key_info(key: &Key, relevant_buckets: &HashMap) { .local_aliases .items() .iter() - .filter(|(_, _, a)| a.as_option() == Some(bucket_id)) + .filter(|(_, _, a)| *a == Some(*bucket_id)) .map(|(a, _, _)| a.clone()) .collect::>() .join(", "); diff --git a/src/model/bucket_alias_table.rs b/src/model/bucket_alias_table.rs index 45807178..fce03d04 100644 --- a/src/model/bucket_alias_table.rs +++ b/src/model/bucket_alias_table.rs @@ -10,32 +10,23 @@ use garage_table::*; #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] pub struct BucketAlias { name: String, - pub state: crdt::Lww>, -} - -#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Debug, Serialize, Deserialize)] -pub struct AliasParams { - pub bucket_id: Uuid, -} - -impl AutoCrdt for AliasParams { - const WARN_IF_DIFFERENT: bool = true; + pub state: crdt::Lww>, } impl BucketAlias { - pub fn new(name: String, ts: u64, bucket_id: Uuid) -> Option { + pub fn new(name: String, ts: u64, bucket_id: Option) -> Option { if !is_valid_bucket_name(&name) { None } else { Some(BucketAlias { name, - state: crdt::Lww::raw(ts, crdt::Deletable::present(AliasParams { bucket_id })), + state: crdt::Lww::raw(ts, bucket_id), }) } } pub fn is_deleted(&self) -> bool { - self.state.get().is_deleted() + self.state.get().is_none() } pub fn name(&self) -> &str { &self.name diff --git a/src/model/bucket_table.rs b/src/model/bucket_table.rs index fef29b62..52c2316c 100644 --- a/src/model/bucket_table.rs +++ b/src/model/bucket_table.rs @@ -63,6 +63,7 @@ impl BucketParams { impl Crdt for BucketParams { fn merge(&mut self, o: &Self) { + self.creation_date = std::cmp::min(self.creation_date, o.creation_date); self.authorized_keys.merge(&o.authorized_keys); self.website_config.merge(&o.website_config); self.aliases.merge(&o.aliases); diff --git a/src/model/helper/bucket.rs b/src/model/helper/bucket.rs index 8bc54b3f..52cedb12 100644 --- a/src/model/helper/bucket.rs +++ b/src/model/helper/bucket.rs @@ -46,7 +46,7 @@ impl<'a> BucketHelper<'a> { .bucket_alias_table .get(&EmptyKey, bucket_name) .await? - .map(|x| x.state.get().as_option().map(|x| x.bucket_id)) + .map(|x| *x.state.get()) .flatten()) } } @@ -154,11 +154,11 @@ impl<'a> BucketHelper<'a> { let alias = self.0.bucket_alias_table.get(&EmptyKey, alias_name).await?; if let Some(existing_alias) = alias.as_ref() { - if let Some(p) = existing_alias.state.get().as_option() { - if p.bucket_id != bucket_id { + if let Some(p_bucket) = existing_alias.state.get() { + if *p_bucket != bucket_id { return Err(Error::BadRequest(format!( "Alias {} already exists and points to different bucket: {:?}", - alias_name, p.bucket_id + alias_name, p_bucket ))); } } @@ -176,10 +176,10 @@ impl<'a> BucketHelper<'a> { // writes are now done and all writes use timestamp alias_ts let alias = match alias { - None => BucketAlias::new(alias_name.clone(), alias_ts, bucket_id) + None => BucketAlias::new(alias_name.clone(), alias_ts, Some(bucket_id)) .ok_or_bad_request(format!("{}: {}", alias_name, INVALID_BUCKET_NAME_MESSAGE))?, Some(mut a) => { - a.state = Lww::raw(alias_ts, Deletable::present(AliasParams { bucket_id })); + a.state = Lww::raw(alias_ts, Some(bucket_id)); a } }; @@ -209,13 +209,7 @@ impl<'a> BucketHelper<'a> { .bucket_alias_table .get(&EmptyKey, alias_name) .await? - .filter(|a| { - a.state - .get() - .as_option() - .map(|x| x.bucket_id == bucket_id) - .unwrap_or(false) - }) + .filter(|a| a.state.get().map(|x| x == bucket_id).unwrap_or(false)) .ok_or_message(format!( "Internal error: alias not found or does not point to bucket {:?}", bucket_id @@ -244,7 +238,7 @@ impl<'a> BucketHelper<'a> { // ---- timestamp-ensured causality barrier ---- // writes are now done and all writes use timestamp alias_ts - alias.state = Lww::raw(alias_ts, Deletable::delete()); + alias.state = Lww::raw(alias_ts, None); self.0.bucket_alias_table.insert(&alias).await?; bucket_state.aliases = LwwMap::raw_item(alias_name.clone(), alias_ts, false); @@ -253,6 +247,51 @@ impl<'a> BucketHelper<'a> { Ok(()) } + /// Ensures a bucket does not have a certain global alias. + /// Contrarily to unset_global_bucket_alias, this does not + /// fail on any condition other than: + /// - bucket cannot be found (its fine if it is in deleted state) + /// - alias cannot be found (its fine if it points to nothing or + /// to another bucket) + pub async fn purge_global_bucket_alias( + &self, + bucket_id: Uuid, + alias_name: &String, + ) -> Result<(), Error> { + let mut bucket = self.get_internal_bucket(bucket_id).await?; + + let mut alias = self + .0 + .bucket_alias_table + .get(&EmptyKey, alias_name) + .await? + .ok_or_message(format!("Alias {} not found", alias_name))?; + + // Checks ok, remove alias + let alias_ts = match bucket.state.as_option() { + Some(bucket_state) => increment_logical_clock_2( + alias.state.timestamp(), + bucket_state.aliases.get_timestamp(alias_name), + ), + None => increment_logical_clock(alias.state.timestamp()), + }; + + // ---- timestamp-ensured causality barrier ---- + // writes are now done and all writes use timestamp alias_ts + + if alias.state.get() == &Some(bucket_id) { + alias.state = Lww::raw(alias_ts, None); + self.0.bucket_alias_table.insert(&alias).await?; + } + + if let Some(mut bucket_state) = bucket.state.as_option_mut() { + bucket_state.aliases = LwwMap::raw_item(alias_name.clone(), alias_ts, false); + self.0.bucket_table.insert(&bucket).await?; + } + + Ok(()) + } + /// Sets a new alias for a bucket in the local namespace of a key. /// This function fails if: /// - alias name is not valid according to S3 spec @@ -277,7 +316,7 @@ impl<'a> BucketHelper<'a> { let mut key_param = key.state.as_option_mut().unwrap(); - if let Some(Deletable::Present(existing_alias)) = key_param.local_aliases.get(alias_name) { + if let Some(Some(existing_alias)) = key_param.local_aliases.get(alias_name) { if *existing_alias != bucket_id { return Err(Error::BadRequest(format!("Alias {} already exists in namespace of key {} and points to different bucket: {:?}", alias_name, key.key_id, existing_alias))); } @@ -301,8 +340,7 @@ impl<'a> BucketHelper<'a> { // ---- timestamp-ensured causality barrier ---- // writes are now done and all writes use timestamp alias_ts - key_param.local_aliases = - LwwMap::raw_item(alias_name.clone(), alias_ts, Deletable::present(bucket_id)); + key_param.local_aliases = LwwMap::raw_item(alias_name.clone(), alias_ts, Some(bucket_id)); self.0.key_table.insert(&key).await?; bucket_p.local_aliases = LwwMap::raw_item(bucket_p_local_alias_key, alias_ts, true); @@ -334,8 +372,8 @@ impl<'a> BucketHelper<'a> { .unwrap() .local_aliases .get(alias_name) - .map(|x| x.as_option()) - .flatten() != Some(&bucket_id) + .cloned() + .flatten() != Some(bucket_id) { return Err(GarageError::Message(format!( "Bucket {:?} does not have alias {} in namespace of key {}", @@ -372,8 +410,7 @@ impl<'a> BucketHelper<'a> { // ---- timestamp-ensured causality barrier ---- // writes are now done and all writes use timestamp alias_ts - key_param.local_aliases = - LwwMap::raw_item(alias_name.clone(), alias_ts, Deletable::delete()); + key_param.local_aliases = LwwMap::raw_item(alias_name.clone(), alias_ts, None); self.0.key_table.insert(&key).await?; bucket_p.local_aliases = LwwMap::raw_item(bucket_p_local_alias_key, alias_ts, false); diff --git a/src/model/key_table.rs b/src/model/key_table.rs index 7afa0337..c25f2da4 100644 --- a/src/model/key_table.rs +++ b/src/model/key_table.rs @@ -31,7 +31,7 @@ pub struct Key { pub struct KeyParams { pub allow_create_bucket: crdt::Lww, pub authorized_buckets: crdt::Map, - pub local_aliases: crdt::LwwMap>, + pub local_aliases: crdt::LwwMap>, } impl KeyParams { diff --git a/src/util/crdt/crdt.rs b/src/util/crdt/crdt.rs index 00bb2e3b..06876897 100644 --- a/src/util/crdt/crdt.rs +++ b/src/util/crdt/crdt.rs @@ -1,5 +1,3 @@ -use crate::data::*; - /// Definition of a CRDT - all CRDT Rust types implement this. /// /// A CRDT is defined as a merge operator that respects a certain set of axioms. @@ -87,7 +85,3 @@ impl AutoCrdt for String { impl AutoCrdt for bool { const WARN_IF_DIFFERENT: bool = true; } - -impl AutoCrdt for FixedBytes32 { - const WARN_IF_DIFFERENT: bool = true; -} diff --git a/src/util/data.rs b/src/util/data.rs index 6b8ee527..f0744307 100644 --- a/src/util/data.rs +++ b/src/util/data.rs @@ -5,7 +5,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use std::fmt; /// An array of 32 bytes -#[derive(Default, PartialOrd, Ord, Clone, Hash, PartialEq, Copy)] +#[derive(Default, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Copy)] pub struct FixedBytes32([u8; 32]); impl From<[u8; 32]> for FixedBytes32 { @@ -20,8 +20,6 @@ impl std::convert::AsRef<[u8]> for FixedBytes32 { } } -impl Eq for FixedBytes32 {} - impl fmt::Debug for FixedBytes32 { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{}…", hex::encode(&self.0[..8])) diff --git a/src/web/web_server.rs b/src/web/web_server.rs index cc6eed57..f13f289e 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -86,9 +86,8 @@ async fn serve_file(garage: Arc, req: Request) -> Result