Merge pull request 'Propose ETag fix' (#23) from bug/etag into master

Reviewed-on: Deuxfleurs/garage#23

Cargo.lock

@@ -461,6 +461,7 @@ dependencies = [
  "log",
  "md-5",
  "percent-encoding",
+ "rand",
  "roxmltree",
  "sha2",
  "tokio",

src/api/Cargo.toml

@@ -27,6 +27,7 @@ md-5 = "0.9.1"
 sha2 = "0.8"
 hmac = "0.7"
 crypto-mac = "0.7"
+rand = "0.7"
 
 futures = "0.3"
 futures-util = "0.3"

src/api/s3_get.rs

@@ -24,10 +24,13 @@ fn object_headers(
 			"Content-Type",
 			version_meta.headers.content_type.to_string(),
 		)
-		.header("ETag", version_meta.etag.to_string())
 		.header("Last-Modified", date_str)
 		.header("Accept-Ranges", format!("bytes"));
 
+	if !version_meta.etag.is_empty() {
+		resp = resp.header("ETag", format!("\"{}\"", version_meta.etag));
+	}
+
 	for (k, v) in version_meta.headers.other.iter() {
 		resp = resp.header(k, v.to_string());
 	}

src/api/s3_put.rs

@@ -428,6 +428,21 @@ pub async fn handle_complete_multipart_upload(
 		_ => unreachable!(),
 	};
 
+	// ETag calculation: we produce ETags that have the same form as
+	// those of S3 multipart uploads, but we don't use their actual
+	// calculation for the first part (we use random bytes). This
+	// shouldn't impact compatibility as the S3 docs specify that
+	// the ETag is an opaque value in case of a multipart upload.
+	// See also: https://teppen.io/2018/06/23/aws_s3_etags/
+	let num_parts = version.blocks().last().unwrap().part_number
+		- version.blocks().first().unwrap().part_number
+		+ 1;
+	let etag = format!(
+		"{}-{}",
+		hex::encode(&rand::random::<[u8; 16]>()[..]),
+		num_parts
+	);
+
 	// TODO: check that all the parts that they pretend they gave us are indeed there
 	// TODO: when we read the XML from _req, remember to check the sha256 sum of the payload
 	//       against the signed x-amz-content-sha256
@@ -442,7 +457,7 @@ pub async fn handle_complete_multipart_upload(
 		ObjectVersionMeta {
 			headers,
 			size: total_size,
-			etag: "".to_string(), // TODO
+			etag: etag,
 		},
 		version.blocks()[0].hash,
 	));

src/table/table.rs

@@ -391,7 +391,8 @@ where
 			let (old_entry, new_entry) = self.store.transaction(|db| {
 				let (old_entry, new_entry) = match db.get(&tree_key)? {
 					Some(prev_bytes) => {
-						let old_entry = self.decode_entry(&prev_bytes)
+						let old_entry = self
+							.decode_entry(&prev_bytes)
 							.map_err(sled::ConflictableTransactionError::Abort)?;
 						let mut new_entry = old_entry.clone();
 						new_entry.merge(&update);