mdebray/tvl-depot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(tvix/castore): remove DirectoryService::from_url

Browse source 
Make directoryservice::from_addr use the more specific constructors.

Change-Id: I9fee2afed77692505988d631d9fe246d9843d25a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10029
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>
This commit is contained in:
Florian Klink 2023-11-13 13:48:30 +02:00 committed by flokli
parent 840bee1e97
commit c83841d3a1
5 changed files with 98 additions and 187 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

97
tvix/castore/src/directoryservice/from_addr.rs
View file

@ -1,6 +1,8 @@
use std::sync::Arc;
use url::Url;
use crate::{proto::directory_service_client::DirectoryServiceClient, Error};
use super::{DirectoryService, GRPCDirectoryService, MemoryDirectoryService, SledDirectoryService};
/// Constructs a new instance of a [DirectoryService] from an URI.
@ -22,11 +24,44 @@ pub fn from_addr(uri: &str) -> Result<Arc<dyn DirectoryService>, crate::Error> {
.map_err(|e| crate::Error::StorageError(format!("unable to parse url: {}", e)))?;
Ok(if url.scheme() == "memory" {
Arc::new(MemoryDirectoryService::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(MemoryDirectoryService::default())
} else if url.scheme() == "sled" {
Arc::new(SledDirectoryService::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(
SledDirectoryService::new_temporary()
.map_err(|e| Error::StorageError(e.to_string()))?,
));
}
return Ok(Arc::new(
SledDirectoryService::new(url.path().into())
.map_err(|e| Error::StorageError(e.to_string()))?,
));
} else if url.scheme().starts_with("grpc+") {
Arc::new(GRPCDirectoryService::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 = DirectoryServiceClient::new(crate::channel::from_url(&url)?);
Arc::new(GRPCDirectoryService::from_client(client))
} else {
Err(crate::Error::StorageError(format!(
"unknown scheme: {}",
@ -34,3 +69,59 @@ pub fn from_addr(uri: &str) -> Result<Arc<dyn DirectoryService>, 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)
}
}

18
tvix/castore/src/directoryservice/grpc.rs
View file

@ -35,18 +35,6 @@ impl GRPCDirectoryService {
#[async_trait]
impl DirectoryService for GRPCDirectoryService {
/// Constructs a [GRPCDirectoryService] 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::directory_service_client::DirectoryServiceClient::new(channel),
))
}
async fn get(
&self,
digest: &B3Digest,
@ -307,7 +295,7 @@ mod tests {
use crate::{
directoryservice::{DirectoryService, GRPCDirectoryService, MemoryDirectoryService},
fixtures::{self, DIRECTORY_A, DIRECTORY_B},
proto::GRPCDirectoryServiceWrapper,
proto::{directory_service_client::DirectoryServiceClient, GRPCDirectoryServiceWrapper},
utils::gen_directorysvc_grpc_client,
};
@ -476,7 +464,9 @@ mod tests {
let grpc_client = {
let url = url::Url::parse(&format!("grpc+unix://{}", socket_path.display()))
.expect("must parse");
GRPCDirectoryService::from_url(&url).expect("must succeed")
let client =
DirectoryServiceClient::new(crate::channel::from_url(&url).expect("must succeed"));
GRPCDirectoryService::from_client(client)
};
assert!(grpc_client

62
tvix/castore/src/directoryservice/memory.rs
View file

@ -16,22 +16,6 @@ pub struct MemoryDirectoryService {
#[async_trait]
impl DirectoryService for MemoryDirectoryService {
/// Constructs a [MemoryDirectoryService] 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(directory.digest = %digest))]
async fn get(&self, digest: &B3Digest) -> Result<Option<proto::Directory>, Error> {
let db = self.db.read()?;
@ -101,49 +85,3 @@ impl DirectoryService for MemoryDirectoryService {
Box::new(SimplePutter::new(self.clone()))
}
}
#[cfg(test)]
mod tests {
use super::DirectoryService;
use super::MemoryDirectoryService;
/// This uses a wrong scheme.
#[test]
fn test_invalid_scheme() {
let url = url::Url::parse("http://foo.example/test").expect("must parse");
assert!(MemoryDirectoryService::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!(MemoryDirectoryService::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!(MemoryDirectoryService::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!(MemoryDirectoryService::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!(MemoryDirectoryService::from_url(&url).is_err());
}
}

6
tvix/castore/src/directoryservice/mod.rs
View file

@ -21,12 +21,6 @@ pub use self::traverse::descend_to;
/// digest.
#[async_trait]
pub trait DirectoryService: 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;
/// Get looks up a single Directory message by its digest.
/// In case the directory is not found, Ok(None) is returned.
async fn get(&self, digest: &B3Digest) -> Result<Option<proto::Directory>, Error>;

102
tvix/castore/src/directoryservice/sled.rs
View file

@ -34,34 +34,6 @@ impl SledDirectoryService {
#[async_trait]
impl DirectoryService for SledDirectoryService {
/// Constructs a [SledDirectoryService] 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, digest), fields(directory.digest = %digest))]
async fn get(&self, digest: &B3Digest) -> Result<Option<proto::Directory>, Error> {
match self.db.get(digest.as_slice()) {
@ -137,77 +109,3 @@ impl DirectoryService for SledDirectoryService {
Box::new(SimplePutter::new(self.clone()))
}
}
#[cfg(test)]
mod tests {
use tempfile::TempDir;
use super::DirectoryService;
use super::SledDirectoryService;
/// This uses a wrong scheme.
#[test]
fn test_invalid_scheme() {
let url = url::Url::parse("http://foo.example/test").expect("must parse");
assert!(SledDirectoryService::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!(SledDirectoryService::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!(SledDirectoryService::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!(SledDirectoryService::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!(SledDirectoryService::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!(SledDirectoryService::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!(SledDirectoryService::from_url(&url).is_err());
}
}