K2V client improvements (#307)

- [x] Better distinguish error types
- [x] Parse error messages received from server
- [x] Remove `src/` folder layer, we don't have that for other crates

Co-authored-by: Alex Auvolat <alex@adnab.me>
Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/307
Co-authored-by: Alex <alex@adnab.me>
Co-committed-by: Alex <alex@adnab.me>

Cargo.lock

@@ -1584,6 +1584,7 @@ dependencies = [
  "clap 3.1.18",
  "garage_util 0.7.0",
  "http",
+ "log",
  "rusoto_core",
  "rusoto_credential",
  "rusoto_signature",

Cargo.nix

@@ -688,7 +688,7 @@ in
     registry = "registry+https://github.com/rust-lang/crates.io-index";
     src = fetchCratesIo { inherit name version; sha256 = "59a6001667ab124aebae2a495118e11d30984c3a653e99d86d58971708cf5e4b"; };
     dependencies = {
-      ${ if hostPlatform.config == "aarch64-linux-android" || hostPlatform.parsed.cpu.name == "aarch64" && hostPlatform.parsed.kernel.name == "linux" || hostPlatform.config == "aarch64-apple-darwin" then "libc" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".libc."0.2.121" { inherit profileName; };
+      ${ if hostPlatform.config == "aarch64-linux-android" || hostPlatform.config == "aarch64-apple-darwin" || hostPlatform.parsed.cpu.name == "aarch64" && hostPlatform.parsed.kernel.name == "linux" then "libc" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".libc."0.2.121" { inherit profileName; };
     };
   });
   
@@ -2117,6 +2117,7 @@ in
       clap = rustPackages."registry+https://github.com/rust-lang/crates.io-index".clap."3.1.18" { inherit profileName; };
       garage_util = rustPackages."unknown".garage_util."0.7.0" { inherit profileName; };
       http = rustPackages."registry+https://github.com/rust-lang/crates.io-index".http."0.2.6" { inherit profileName; };
+      log = rustPackages."registry+https://github.com/rust-lang/crates.io-index".log."0.4.16" { inherit profileName; };
       rusoto_core = rustPackages."registry+https://github.com/rust-lang/crates.io-index".rusoto_core."0.48.0" { inherit profileName; };
       rusoto_credential = rustPackages."registry+https://github.com/rust-lang/crates.io-index".rusoto_credential."0.48.0" { inherit profileName; };
       rusoto_signature = rustPackages."registry+https://github.com/rust-lang/crates.io-index".rusoto_signature."0.48.0" { inherit profileName; };
@@ -5029,7 +5030,7 @@ in
       [ "default" ]
     ];
     dependencies = {
-      ${ if hostPlatform.config == "aarch64-uwp-windows-msvc" || hostPlatform.config == "aarch64-pc-windows-msvc" then "windows_aarch64_msvc" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".windows_aarch64_msvc."0.32.0" { inherit profileName; };
+      ${ if hostPlatform.config == "aarch64-pc-windows-msvc" || hostPlatform.config == "aarch64-uwp-windows-msvc" then "windows_aarch64_msvc" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".windows_aarch64_msvc."0.32.0" { inherit profileName; };
       ${ if hostPlatform.config == "i686-pc-windows-gnu" || hostPlatform.config == "i686-uwp-windows-gnu" then "windows_i686_gnu" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".windows_i686_gnu."0.32.0" { inherit profileName; };
       ${ if hostPlatform.config == "i686-uwp-windows-msvc" || hostPlatform.config == "i686-pc-windows-msvc" then "windows_i686_msvc" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".windows_i686_msvc."0.32.0" { inherit profileName; };
       ${ if hostPlatform.config == "x86_64-pc-windows-gnu" || hostPlatform.config == "x86_64-uwp-windows-gnu" then "windows_x86_64_gnu" else null } = rustPackages."registry+https://github.com/rust-lang/crates.io-index".windows_x86_64_gnu."0.32.0" { inherit profileName; };

default.nix

@@ -86,14 +86,13 @@ in let
      It speeds up the compilation (when the feature is not required) and released crates have less dependency by default (less attack surface, disk space, etc.).
      But we want to ship these additional features when we release Garage.
      In the end, we chose to exclude all features from debug builds while putting (all of) them in the release builds.
-     Currently, the only such feature of Garage is kubernetes-discovery from the garage_rpc crate.
-     The experimental feature k2v is also enabled here in all builds.
+     Currently, the only feature of Garage is kubernetes-discovery from the garage_rpc crate.
     */
     (pkgs.rustBuilder.rustLib.makeOverride {
       name = "garage_rpc";
       overrideArgs = old:
         {
-          features = if release then [ "kubernetes-discovery" "k2v" ] else [ "k2v" ];
+          features = if release then [ "kubernetes-discovery" ] else [];
         };
     })
   ];

doc/drafts/admin-api.md

@@ -12,7 +12,7 @@ when changes are introduced.
 The admin API uses two different tokens for acces control, that are specified in the config file's `[admin]` section:
 
 - `metrics_token`: the token for accessing the Metrics endpoint (if this token is not set in the config file, the Metrics endpoint can be accessed without access control);
-- `admin_token`: the token for accessing all of the other administration endpoints (if this token is not set in the config file, these endpoints can be accessed without access control).
+- `admin_token`: the token for accessing all of the other administration endpoints (if this token is not set in the config file, access to these endpoints is disabled entirely).
 
 ## Administration API endpoints
 
@@ -124,11 +124,11 @@ Example response:
 [
 	{
 		"success": true,
-		"error": null,
+		"error": null
 	},
 	{
 		"success": false,
-		"error": "Handshake error",
+		"error": "Handshake error"
 	}
 ]
 ```
@@ -490,7 +490,11 @@ OR
 	"localAlias": {
 		"accessKeyId": "GK31c2f218a2e44f485b94239e",
 		"alias": "NameOfMyBucket",
-		"allPermissions": true
+		"allow": {
+			"read": true,
+			"write": true,
+			"owner": false
+		}
 	}
 }
 ```
@@ -504,6 +508,9 @@ OR
 Creates a new bucket, either with a global alias, a local one,
 or no alias at all.
 
+Technically, you can also specify both `globalAlias` and `localAlias` and that would create
+two aliases, but I don't see why you would want to do that.
+
 #### DeleteBucket `DELETE /v0/bucket?id=<bucket id>`
 
 Deletes a storage bucket. A bucket cannot be deleted if it is not empty.
@@ -519,7 +526,7 @@ Request body format:
 ```json
 {
 	"indexDocument": "index.html",
-	"errorDocument": "404.html",
+	"errorDocument": "404.html"
 }
 ```
 

src/api/admin/bucket.rs

@@ -7,6 +7,7 @@ use serde::{Deserialize, Serialize};
 use garage_util::crdt::*;
 use garage_util::data::*;
 use garage_util::error::Error as GarageError;
+use garage_util::time::*;
 
 use garage_table::*;
 
@@ -283,13 +284,19 @@ pub async fn handle_create_bucket(
 			.bucket_helper()
 			.set_local_bucket_alias(bucket.id, &la.access_key_id, &la.alias)
 			.await?;
-		if la.all_permissions {
+
+		if la.allow.read || la.allow.write || la.allow.owner {
 			garage
 				.bucket_helper()
 				.set_bucket_key_permissions(
 					bucket.id,
 					&la.access_key_id,
-					BucketKeyPerm::ALL_PERMISSIONS,
+					BucketKeyPerm {
+						timestamp: now_msec(),
+						allow_read: la.allow.read,
+						allow_write: la.allow.write,
+						allow_owner: la.allow.owner,
+					},
 				)
 				.await?;
 		}
@@ -311,7 +318,7 @@ struct CreateBucketLocalAlias {
 	access_key_id: String,
 	alias: String,
 	#[serde(default)]
-	all_permissions: bool,
+	allow: ApiBucketKeyPerm,
 }
 
 pub async fn handle_delete_bucket(

src/k2v-client/Cargo.toml

@@ -6,6 +6,7 @@ edition = "2018"
 [dependencies]
 base64 = "0.13.0"
 http = "0.2.6"
+log = "0.4"
 rusoto_core = "0.48.0"
 rusoto_credential = "0.48.0"
 rusoto_signature = "0.48.0"
@@ -22,6 +23,10 @@ garage_util = { path = "../util", optional = true }
 [features]
 cli = ["clap", "tokio/fs", "tokio/io-std", "garage_util"]
 
+[lib]
+path = "lib.rs"
+
 [[bin]]
 name = "k2v-cli"
+path = "bin/k2v-cli.rs"
 required-features = ["cli"]

src/k2v-client/error.rs

@@ -5,6 +5,13 @@ use thiserror::Error;
 /// Errors returned by this crate
 #[derive(Error, Debug)]
 pub enum Error {
+	#[error("{0}, {1}: {2} (path = {3})")]
+	Remote(
+		http::StatusCode,
+		Cow<'static, str>,
+		Cow<'static, str>,
+		Cow<'static, str>,
+	),
 	#[error("received invalid response: {0}")]
 	InvalidResponse(Cow<'static, str>),
 	#[error("not found")]

src/k2v-client/lib.rs

@@ -4,6 +4,7 @@ use std::time::Duration;
 use http::header::{ACCEPT, CONTENT_LENGTH, CONTENT_TYPE};
 use http::status::StatusCode;
 use http::HeaderMap;
+use log::{debug, error};
 
 use rusoto_core::{ByteStream, DispatchSignedRequest, HttpClient};
 use rusoto_credential::AwsCredentials;
@@ -310,12 +311,47 @@ impl K2vClient {
 			StatusCode::NO_CONTENT => Vec::new(),
 			StatusCode::NOT_FOUND => return Err(Error::NotFound),
 			StatusCode::NOT_MODIFIED => Vec::new(),
-			_ => {
-				return Err(Error::InvalidResponse(
-					format!("invalid error code: {}", res.status).into(),
-				))
+			s => {
+				let err_body = read_body(&mut res.headers, res.body)
+					.await
+					.unwrap_or_default();
+				let err_body_str = std::str::from_utf8(&err_body)
+					.map(String::from)
+					.unwrap_or_else(|_| base64::encode(&err_body));
+
+				if s.is_client_error() || s.is_server_error() {
+					error!("Error response {}: {}", res.status, err_body_str);
+					let err = match serde_json::from_slice::<ErrorResponse>(&err_body) {
+						Ok(err) => Error::Remote(
+							res.status,
+							err.code.into(),
+							err.message.into(),
+							err.path.into(),
+						),
+						Err(_) => Error::Remote(
+							res.status,
+							"unknown".into(),
+							err_body_str.into(),
+							"?".into(),
+						),
+					};
+					return Err(err);
+				} else {
+					let msg = format!(
+						"Unexpected response code {}. Response body: {}",
+						res.status, err_body_str
+					);
+					error!("{}", msg);
+					return Err(Error::InvalidResponse(msg.into()));
+				}
 			}
 		};
+		debug!(
+			"Response body: {}",
+			std::str::from_utf8(&body)
+				.map(String::from)
+				.unwrap_or_else(|_| base64::encode(&body))
+		);
 
 		Ok(Response {
 			body,
@@ -558,6 +594,15 @@ struct BatchDeleteResponse<'a> {
 	deleted_items: u64,
 }
 
+#[derive(Deserialize)]
+struct ErrorResponse {
+	code: String,
+	message: String,
+	#[allow(dead_code)]
+	region: String,
+	path: String,
+}
+
 struct Response {
 	body: Vec<u8>,
 	status: StatusCode,