diff options
Diffstat (limited to 'tvix/castore/src/blobservice')
| -rw-r--r-- | tvix/castore/src/blobservice/from_addr.rs | 96 | ||||
| -rw-r--r-- | tvix/castore/src/blobservice/grpc.rs | 73 | ||||
| -rw-r--r-- | tvix/castore/src/blobservice/memory.rs | 62 | ||||
| -rw-r--r-- | tvix/castore/src/blobservice/mod.rs | 6 | ||||
| -rw-r--r-- | tvix/castore/src/blobservice/sled.rs | 102 |
5 files changed, 98 insertions, 241 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) + } +} diff --git a/tvix/castore/src/blobservice/grpc.rs b/tvix/castore/src/blobservice/grpc.rs index 8d3bdfa8fa25..85863c29197f 100644 --- a/tvix/castore/src/blobservice/grpc.rs +++ b/tvix/castore/src/blobservice/grpc.rs @@ -38,18 +38,6 @@ impl GRPCBlobService { #[async_trait] impl BlobService for GRPCBlobService { - /// Constructs a [GRPCBlobService] from the passed [url::Url]: - /// - scheme has to match `grpc+*://`. - /// 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. - fn from_url(url: &url::Url) -> Result<Self, crate::Error> { - let channel = crate::channel::from_url(url)?; - Ok(Self::from_client( - proto::blob_service_client::BlobServiceClient::new(channel), - )) - } - #[instrument(skip(self, digest), fields(blob.digest=%digest))] async fn has(&self, digest: &B3Digest) -> Result<bool, crate::Error> { let mut grpc_client = self.grpc_client.clone(); @@ -251,66 +239,12 @@ mod tests { use crate::blobservice::MemoryBlobService; use crate::fixtures; + use crate::proto::blob_service_client::BlobServiceClient; use crate::proto::GRPCBlobServiceWrapper; use super::BlobService; use super::GRPCBlobService; - /// This uses the wrong scheme - #[test] - fn test_invalid_scheme() { - let url = url::Url::parse("http://foo.example/test").expect("must parse"); - - assert!(GRPCBlobService::from_url(&url).is_err()); - } - - /// This uses the correct scheme for a unix socket. - /// The fact that /path/to/somewhere doesn't exist yet is no problem, because we connect lazily. - #[tokio::test] - async fn test_valid_unix_path() { - let url = url::Url::parse("grpc+unix:///path/to/somewhere").expect("must parse"); - - assert!(GRPCBlobService::from_url(&url).is_ok()); - } - - /// This uses the correct scheme for a unix socket, - /// but sets a host, which is unsupported. - #[tokio::test] - async fn test_invalid_unix_path_with_domain() { - let url = - url::Url::parse("grpc+unix://host.example/path/to/somewhere").expect("must parse"); - - assert!(GRPCBlobService::from_url(&url).is_err()); - } - - /// This uses the correct scheme for a HTTP server. - /// The fact that nothing is listening there is no problem, because we connect lazily. - #[tokio::test] - async fn test_valid_http() { - let url = url::Url::parse("grpc+http://localhost").expect("must parse"); - - assert!(GRPCBlobService::from_url(&url).is_ok()); - } - - /// This uses the correct scheme for a HTTPS server. - /// The fact that nothing is listening there is no problem, because we connect lazily. - #[tokio::test] - async fn test_valid_https() { - let url = url::Url::parse("grpc+https://localhost").expect("must parse"); - - assert!(GRPCBlobService::from_url(&url).is_ok()); - } - - /// This uses the correct scheme, but also specifies - /// an additional path, which is not supported for gRPC. - /// The fact that nothing is listening there is no problem, because we connect lazily. - #[tokio::test] - async fn test_invalid_http_with_path() { - let url = url::Url::parse("grpc+https://localhost/some-path").expect("must parse"); - - assert!(GRPCBlobService::from_url(&url).is_err()); - } - /// This ensures connecting via gRPC works as expected. #[tokio::test] async fn test_valid_unix_path_ping_pong() { @@ -353,7 +287,10 @@ mod tests { let grpc_client = { let url = url::Url::parse(&format!("grpc+unix://{}", socket_path.display())) .expect("must parse"); - GRPCBlobService::from_url(&url).expect("must succeed") + let client = + BlobServiceClient::new(crate::channel::from_url(&url).expect("must succeed")); + + GRPCBlobService::from_client(client) }; let has = grpc_client diff --git a/tvix/castore/src/blobservice/memory.rs b/tvix/castore/src/blobservice/memory.rs index 383127344a17..8f4e50aa37c5 100644 --- a/tvix/castore/src/blobservice/memory.rs +++ b/tvix/castore/src/blobservice/memory.rs @@ -17,22 +17,6 @@ pub struct MemoryBlobService { #[async_trait] impl BlobService for MemoryBlobService { - /// Constructs a [MemoryBlobService] from the passed [url::Url]: - /// - scheme has to be `memory://` - /// - there may not be a host. - /// - there may not be a path. - fn from_url(url: &url::Url) -> Result<Self, Error> { - if url.scheme() != "memory" { - return Err(crate::Error::StorageError("invalid scheme".to_string())); - } - - if url.has_host() || !url.path().is_empty() { - return Err(crate::Error::StorageError("invalid url".to_string())); - } - - Ok(Self::default()) - } - #[instrument(skip(self, digest), fields(blob.digest=%digest))] async fn has(&self, digest: &B3Digest) -> Result<bool, Error> { let db = self.db.read().unwrap(); @@ -148,49 +132,3 @@ impl BlobWriter for MemoryBlobWriter { } } } - -#[cfg(test)] -mod tests { - use super::BlobService; - use super::MemoryBlobService; - - /// This uses a wrong scheme. - #[test] - fn test_invalid_scheme() { - let url = url::Url::parse("http://foo.example/test").expect("must parse"); - - assert!(MemoryBlobService::from_url(&url).is_err()); - } - - /// This correctly sets the scheme, and doesn't set a path. - #[test] - fn test_valid_scheme() { - let url = url::Url::parse("memory://").expect("must parse"); - - assert!(MemoryBlobService::from_url(&url).is_ok()); - } - - /// This sets the host to `foo` - #[test] - fn test_invalid_host() { - let url = url::Url::parse("memory://foo").expect("must parse"); - - assert!(MemoryBlobService::from_url(&url).is_err()); - } - - /// This has the path "/", which is invalid. - #[test] - fn test_invalid_has_path() { - let url = url::Url::parse("memory:///").expect("must parse"); - - assert!(MemoryBlobService::from_url(&url).is_err()); - } - - /// This has the path "/foo", which is invalid. - #[test] - fn test_invalid_path2() { - let url = url::Url::parse("memory:///foo").expect("must parse"); - - assert!(MemoryBlobService::from_url(&url).is_err()); - } -} diff --git a/tvix/castore/src/blobservice/mod.rs b/tvix/castore/src/blobservice/mod.rs index 5ecf25ac1337..3267116e404e 100644 --- a/tvix/castore/src/blobservice/mod.rs +++ b/tvix/castore/src/blobservice/mod.rs @@ -24,12 +24,6 @@ pub use self::sled::SledBlobService; /// close funtion, to finalize a blob and get its digest. #[async_trait] pub trait BlobService: Send + Sync { - /// Create a new instance by passing in a connection URL. - /// TODO: check if we want to make this async, instead of lazily connecting - fn from_url(url: &url::Url) -> Result<Self, Error> - where - Self: Sized; - /// Check if the service has the blob, by its content hash. async fn has(&self, digest: &B3Digest) -> Result<bool, Error>; diff --git a/tvix/castore/src/blobservice/sled.rs b/tvix/castore/src/blobservice/sled.rs index 101ae78698fd..12391ed72a86 100644 --- a/tvix/castore/src/blobservice/sled.rs +++ b/tvix/castore/src/blobservice/sled.rs @@ -31,34 +31,6 @@ impl SledBlobService { #[async_trait] impl BlobService for SledBlobService { - /// Constructs a [SledBlobService] from the passed [url::Url]: - /// - scheme has to be `sled://` - /// - there may not be a host. - /// - a path to the sled needs to be provided (which may not be `/`). - fn from_url(url: &url::Url) -> Result<Self, Error> { - if url.scheme() != "sled" { - return Err(crate::Error::StorageError("invalid scheme".to_string())); - } - - if url.has_host() { - return Err(crate::Error::StorageError(format!( - "invalid host: {}", - url.host().unwrap() - ))); - } - - // TODO: expose compression and other parameters as URL parameters, drop new and new_temporary? - if url.path().is_empty() { - Self::new_temporary().map_err(|e| Error::StorageError(e.to_string())) - } else if url.path() == "/" { - Err(crate::Error::StorageError( - "cowardly refusing to open / with sled".to_string(), - )) - } else { - Self::new(url.path().into()).map_err(|e| Error::StorageError(e.to_string())) - } - } - #[instrument(skip(self), fields(blob.digest=%digest))] async fn has(&self, digest: &B3Digest) -> Result<bool, Error> { match self.db.contains_key(digest.as_slice()) { @@ -173,77 +145,3 @@ impl BlobWriter for SledBlobWriter { } } } - -#[cfg(test)] -mod tests { - use tempfile::TempDir; - - use super::BlobService; - use super::SledBlobService; - - /// This uses a wrong scheme. - #[test] - fn test_invalid_scheme() { - let url = url::Url::parse("http://foo.example/test").expect("must parse"); - - assert!(SledBlobService::from_url(&url).is_err()); - } - - /// This uses the correct scheme, and doesn't specify a path (temporary sled). - #[test] - fn test_valid_scheme_temporary() { - let url = url::Url::parse("sled://").expect("must parse"); - - assert!(SledBlobService::from_url(&url).is_ok()); - } - - /// This sets the path to a location that doesn't exist, which should fail (as sled doesn't mkdir -p) - #[test] - fn test_nonexistent_path() { - let tmpdir = TempDir::new().unwrap(); - - let mut url = url::Url::parse("sled://foo.example").expect("must parse"); - url.set_path(tmpdir.path().join("foo").join("bar").to_str().unwrap()); - - assert!(SledBlobService::from_url(&url).is_err()); - } - - /// This uses the correct scheme, and specifies / as path (which should fail - // for obvious reasons) - #[test] - fn test_invalid_path_root() { - let url = url::Url::parse("sled:///").expect("must parse"); - - assert!(SledBlobService::from_url(&url).is_err()); - } - - /// This uses the correct scheme, and sets a tempdir as location. - #[test] - fn test_valid_scheme_path() { - let tmpdir = TempDir::new().unwrap(); - - let mut url = url::Url::parse("sled://").expect("must parse"); - url.set_path(tmpdir.path().to_str().unwrap()); - - assert!(SledBlobService::from_url(&url).is_ok()); - } - - /// This sets a host, rather than a path, which should fail. - #[test] - fn test_invalid_host() { - let url = url::Url::parse("sled://foo.example").expect("must parse"); - - assert!(SledBlobService::from_url(&url).is_err()); - } - - /// This sets a host AND a valid path, which should fail - #[test] - fn test_invalid_host_and_path() { - let tmpdir = TempDir::new().unwrap(); - - let mut url = url::Url::parse("sled://foo.example").expect("must parse"); - url.set_path(tmpdir.path().to_str().unwrap()); - - assert!(SledBlobService::from_url(&url).is_err()); - } -} |