refactor(tvix/castore/blobsvc): remove BlobService::from_url r/7020

Make blobservice::from_addr use the more specific constructors.

Change-Id: Id9637e279d6910ce6d92ff0086a984be5c65a8c8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10028
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>

1 files changed, 93 insertions, 3 deletions

diff --git a/tvix/castore/src/blobservice/from_addr.rs b/tvix/castore/src/blobservice/from_addr.rs
index 2e0a30697d75..916c00442c99 100644
--- a/tvix/castore/src/blobservice/from_addr.rs
+++ b/tvix/castore/src/blobservice/from_addr.rs
@@ -1,6 +1,8 @@
 use std::sync::Arc;
 use url::Url;
 
+use crate::{proto::blob_service_client::BlobServiceClient, Error};
+
 use super::{BlobService, GRPCBlobService, MemoryBlobService, SledBlobService};
 
 /// Constructs a new instance of a [BlobService] from an URI.
@@ -16,11 +18,43 @@ pub fn from_addr(uri: &str) -> Result<Arc<dyn BlobService>, crate::Error> {
         .map_err(|e| crate::Error::StorageError(format!("unable to parse url: {}", e)))?;
 
     Ok(if url.scheme() == "memory" {
-        Arc::new(MemoryBlobService::from_url(&url)?)
+        // memory doesn't support host or path in the URL.
+        if url.has_host() || !url.path().is_empty() {
+            return Err(Error::StorageError("invalid url".to_string()));
+        }
+        Arc::new(MemoryBlobService::default())
     } else if url.scheme() == "sled" {
-        Arc::new(SledBlobService::from_url(&url)?)
+        // sled doesn't support host, and a path can be provided (otherwise
+        // it'll live in memory only).
+        if url.has_host() {
+            return Err(Error::StorageError("no host allowed".to_string()));
+        }
+
+        if url.path() == "/" {
+            return Err(Error::StorageError(
+                "cowardly refusing to open / with sled".to_string(),
+            ));
+        }
+
+        // TODO: expose compression and other parameters as URL parameters?
+
+        if url.path().is_empty() {
+            return Ok(Arc::new(
+                SledBlobService::new_temporary().map_err(|e| Error::StorageError(e.to_string()))?,
+            ));
+        }
+        return Ok(Arc::new(
+            SledBlobService::new(url.path().into())
+                .map_err(|e| Error::StorageError(e.to_string()))?,
+        ));
     } else if url.scheme().starts_with("grpc+") {
-        Arc::new(GRPCBlobService::from_url(&url)?)
+        // schemes starting with grpc+ go to the GRPCPathInfoService.
+        //   That's normally grpc+unix for unix sockets, and grpc+http(s) for the HTTP counterparts.
+        // - In the case of unix sockets, there must be a path, but may not be a host.
+        // - In the case of non-unix sockets, there must be a host, but no path.
+        // Constructing the channel is handled by tvix_castore::channel::from_url.
+        let client = BlobServiceClient::new(crate::channel::from_url(&url)?);
+        Arc::new(GRPCBlobService::from_client(client))
     } else {
         Err(crate::Error::StorageError(format!(
             "unknown scheme: {}",
@@ -28,3 +62,59 @@ pub fn from_addr(uri: &str) -> Result<Arc<dyn BlobService>, crate::Error> {
         )))?
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::from_addr;
+    use lazy_static::lazy_static;
+    use tempfile::TempDir;
+    use test_case::test_case;
+
+    lazy_static! {
+        static ref TMPDIR_SLED_1: TempDir = TempDir::new().unwrap();
+        static ref TMPDIR_SLED_2: TempDir = TempDir::new().unwrap();
+    }
+
+    /// This uses an unsupported scheme.
+    #[test_case("http://foo.example/test", false; "unsupported scheme")]
+    /// This configures sled in temporary mode.
+    #[test_case("sled://", true; "sled valid temporary")]
+    /// This configures sled with /, which should fail.
+    #[test_case("sled:///", false; "sled invalid root")]
+    /// This configures sled with a host, not path, which should fail.
+    #[test_case("sled://foo.example", false; "sled invalid host")]
+    /// This configures sled with a valid path path, which should succeed.
+    #[test_case(&format!("sled://{}", &TMPDIR_SLED_1.path().to_str().unwrap()), true; "sled valid path")]
+    /// This configures sled with a host, and a valid path path, which should fail.
+    #[test_case(&format!("sled://foo.example{}", &TMPDIR_SLED_2.path().to_str().unwrap()), false; "sled invalid host with valid path")]
+    /// This correctly sets the scheme, and doesn't set a path.
+    #[test_case("memory://", true; "memory valid")]
+    /// This sets a memory url host to `foo`
+    #[test_case("memory://foo", false; "memory invalid host")]
+    /// This sets a memory url path to "/", which is invalid.
+    #[test_case("memory:///", false; "memory invalid root path")]
+    /// This sets a memory url path to "/foo", which is invalid.
+    #[test_case("memory:///foo", false; "memory invalid root path foo")]
+    fn test_from_addr(uri_str: &str, is_ok: bool) {
+        assert_eq!(from_addr(uri_str).is_ok(), is_ok)
+    }
+
+    // the gRPC tests below don't fail, because we connect lazily.
+
+    /// Correct scheme to connect to a unix socket.
+    #[test_case("grpc+unix:///path/to/somewhere", true; "grpc valid unix socket")]
+    /// Correct scheme for unix socket, but setting a host too, which is invalid.
+    #[test_case("grpc+unix://host.example/path/to/somewhere", false; "grpc invalid unix socket and host")]
+    /// Correct scheme to connect to localhost, with port 12345
+    #[test_case("grpc+http://[::1]:12345", true; "grpc valid IPv6 localhost port 12345")]
+    /// Correct scheme to connect to localhost over http, without specifying a port.
+    #[test_case("grpc+http://localhost", true; "grpc valid http host without port")]
+    /// Correct scheme to connect to localhost over http, without specifying a port.
+    #[test_case("grpc+https://localhost", true; "grpc valid https host without port")]
+    /// Correct scheme to connect to localhost over http, but with additional path, which is invalid.
+    #[test_case("grpc+http://localhost/some-path", false; "grpc valid invalid host and path")]
+    #[tokio::test]
+    async fn test_from_addr_tokio(uri_str: &str, is_ok: bool) {
+        assert_eq!(from_addr(uri_str).is_ok(), is_ok)
+    }
+}