From 6db010b21d0f696c883812dda8271d83f90fcd46 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 14:06:33 +0800 Subject: [PATCH 1/8] feat: Exit while cache is not configured correctly Signed-off-by: Xuanwo --- src/cache/cache.rs | 141 +++++++++++++++++++-------------------------- src/config.rs | 43 +++++--------- src/server.rs | 2 +- 3 files changed, 77 insertions(+), 109 deletions(-) diff --git a/src/cache/cache.rs b/src/cache/cache.rs index 42213b720..c4e13c149 100644 --- a/src/cache/cache.rs +++ b/src/cache/cache.rs @@ -379,24 +379,24 @@ fn normalize_key(key: &str) -> String { /// Get a suitable `Storage` implementation from configuration. #[allow(clippy::cognitive_complexity)] // TODO simplify! -pub fn storage_from_config(config: &Config, pool: &tokio::runtime::Handle) -> Arc { - for cache_type in config.caches.iter() { - match *cache_type { +pub fn storage_from_config( + config: &Config, + pool: &tokio::runtime::Handle, +) -> Result> { + if let Some(cache_type) = &config.cache { + match cache_type { + #[cfg(feature = "azure")] CacheType::Azure(config::AzureCacheConfig { ref connection_string, ref container, ref key_prefix, }) => { - debug!("Trying Azure Blob Store account({})", key_prefix); - #[cfg(feature = "azure")] - match AzureBlobCache::build(connection_string, container, key_prefix) { - Ok(storage) => { - trace!("Using AzureBlobCache"); - return Arc::new(storage); - } - Err(e) => warn!("Failed to create Azure cache: {:?}", e), - } + debug!("Init azure cache with container {container}, key_prefix {key_prefix}"); + let storage = AzureBlobCache::build(connection_string, container, key_prefix) + .map_err(|err| anyhow!("create azure cache failed: {err:?}"))?; + return Ok(Arc::new(storage)); } + #[cfg(feature = "gcs")] CacheType::GCS(config::GCSCacheConfig { ref bucket, ref key_prefix, @@ -405,99 +405,78 @@ pub fn storage_from_config(config: &Config, pool: &tokio::runtime::Handle) -> Ar ref service_account, ref credential_url, }) => { - debug!( - "Trying GCS bucket({}, {}, {:?})", - bucket, key_prefix, rw_mode - ); - #[cfg(feature = "gcs")] - { - let gcs_read_write_mode = match rw_mode { - config::GCSCacheRWMode::ReadOnly => RWMode::ReadOnly, - config::GCSCacheRWMode::ReadWrite => RWMode::ReadWrite, - }; - - match GCSCache::build( - bucket, - key_prefix, - cred_path.as_deref(), - service_account.as_deref(), - gcs_read_write_mode, - credential_url.as_deref(), - ) { - Ok(s) => { - trace!("Using GCSCache"); - return Arc::new(s); - } - Err(e) => warn!("Failed to create GCS Cache: {:?}", e), - } - } + debug!("Init gcs cache with bucket {bucket}, key_prefix {key_prefix}"); + + let gcs_read_write_mode = match rw_mode { + config::GCSCacheRWMode::ReadOnly => RWMode::ReadOnly, + config::GCSCacheRWMode::ReadWrite => RWMode::ReadWrite, + }; + + let storage = GCSCache::build( + bucket, + key_prefix, + cred_path.as_deref(), + service_account.as_deref(), + gcs_read_write_mode, + credential_url.as_deref(), + ) + .map_err(|err| anyhow!("create gcs cache failed: {err:?}"))?; + + return Ok(Arc::new(storage)); } + #[cfg(feature = "gha")] CacheType::GHA(config::GHACacheConfig { ref url, ref token, ref cache_to, ref cache_from, }) => { - debug!( - "Trying GHA Cache ({url}, {}***, {cache_to:?}, {cache_from:?})", - &token[..usize::min(3, token.len())] - ); - #[cfg(feature = "gha")] - match GHACache::new(url, token, cache_to.clone(), cache_from.clone()) { - Ok(s) => { - trace!("Using GHA Cache: {}", url); - return Arc::new(s); - } - Err(e) => warn!("Failed to create GHA Cache: {:?}", e), - } + debug!("Init gha cache with url {url}"); + + let storage = GHACache::new(url, token, cache_to.clone(), cache_from.clone()) + .map_err(|err| anyhow!("create gha cache failed: {err:?}"))?; + return Ok(Arc::new(storage)); } + #[cfg(feature = "memcached")] CacheType::Memcached(config::MemcachedCacheConfig { ref url }) => { - debug!("Trying Memcached({})", url); - #[cfg(feature = "memcached")] - match MemcachedCache::new(url, pool) { - Ok(s) => { - trace!("Using Memcached: {}", url); - return Arc::new(s); - } - Err(e) => warn!("Failed to create MemcachedCache: {:?}", e), - } + debug!("Init memcached cache with url {url}"); + + let storage = MemcachedCache::new(url, pool) + .map_err(|err| anyhow!("create memcached cache failed: {err:?}"))?; + return Ok(Arc::new(storage)); } + #[cfg(feature = "redis")] CacheType::Redis(config::RedisCacheConfig { ref url }) => { - debug!("Trying Redis({})", url); - #[cfg(feature = "redis")] - match RedisCache::build(url) { - Ok(s) => { - trace!("Using Redis: {}", url); - return Arc::new(s); - } - Err(e) => warn!("Failed to create RedisCache: {:?}", e), - } + debug!("Init redis cache with url {url}"); + let storage = RedisCache::build(url) + .map_err(|err| anyhow!("create redis cache failed: {err:?}"))?; + return Ok(Arc::new(storage)); } + #[cfg(feature = "s3")] CacheType::S3(ref c) => { - debug!("Trying S3Cache({:?})", c); - #[cfg(feature = "s3")] - match S3Cache::build( + debug!( + "Init s3 cache with bucket {}, endpoint {:?}", + c.bucket, c.endpoint + ); + + let storage = S3Cache::build( &c.bucket, c.region.as_deref(), &c.key_prefix, c.no_credentials, c.endpoint.as_deref(), c.use_ssl, - ) { - Ok(s) => { - trace!("Using S3Cache"); - return Arc::new(s); - } - Err(e) => warn!("Failed to create S3Cache: {:?}", e), - } + ) + .map_err(|err| anyhow!("create s3 cache failed: {err:?}"))?; + + return Ok(Arc::new(storage)); } } } - info!("No configured caches successful, falling back to default"); let (dir, size) = (&config.fallback_cache.dir, config.fallback_cache.size); - trace!("Using DiskCache({:?}, {})", dir, size); - Arc::new(DiskCache::new(&dir, size, pool)) + debug!("Init disk cache with dir {:?}, size {}", dir, size); + Ok(Arc::new(DiskCache::new(&dir, size, pool))) } #[cfg(test)] diff --git a/src/config.rs b/src/config.rs index 9073fac65..1ba495c0b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -244,9 +244,9 @@ pub struct CacheConfigs { } impl CacheConfigs { - /// Return a vec of the available cache types in an arbitrary but + /// Return cache type in an arbitrary but /// consistent ordering - fn into_vec_and_fallback(self) -> (Vec, DiskCacheConfig) { + fn into_fallback(self) -> (Option, DiskCacheConfig) { let CacheConfigs { azure, disk, @@ -257,18 +257,17 @@ impl CacheConfigs { s3, } = self; - let caches = s3 + let cache_type = s3 .map(CacheType::S3) - .into_iter() - .chain(redis.map(CacheType::Redis)) - .chain(memcached.map(CacheType::Memcached)) - .chain(gcs.map(CacheType::GCS)) - .chain(gha.map(CacheType::GHA)) - .chain(azure.map(CacheType::Azure)) - .collect(); + .or_else(|| redis.map(CacheType::Redis)) + .or_else(|| memcached.map(CacheType::Memcached)) + .or_else(|| gcs.map(CacheType::GCS)) + .or_else(|| gha.map(CacheType::GHA)) + .or_else(|| azure.map(CacheType::Azure)); + let fallback = disk.unwrap_or_default(); - (caches, fallback) + (cache_type, fallback) } /// Override self with any existing fields from other @@ -666,7 +665,7 @@ fn config_file(env_var: &str, leaf: &str) -> PathBuf { #[derive(Debug, Default, PartialEq, Eq)] pub struct Config { - pub caches: Vec, + pub cache: Option, pub fallback_cache: DiskCacheConfig, pub dist: DistConfig, pub server_startup_timeout: Option, @@ -700,9 +699,9 @@ impl Config { let EnvConfig { cache } = env_conf; conf_caches.merge(cache); - let (caches, fallback_cache) = conf_caches.into_vec_and_fallback(); + let (caches, fallback_cache) = conf_caches.into_fallback(); Self { - caches, + cache: caches, fallback_cache, dist, server_startup_timeout, @@ -990,19 +989,9 @@ fn config_overrides() { assert_eq!( Config::from_env_and_file_configs(env_conf, file_conf), Config { - caches: vec![ - CacheType::Redis(RedisCacheConfig { - url: "myotherredisurl".to_owned() - }), - CacheType::Memcached(MemcachedCacheConfig { - url: "memurl".to_owned() - }), - CacheType::Azure(AzureCacheConfig { - connection_string: String::new(), - container: String::new(), - key_prefix: String::new() - }), - ], + cache: Some(CacheType::Redis(RedisCacheConfig { + url: "myotherredisurl".to_owned() + }),), fallback_cache: DiskCacheConfig { dir: "/env-cache".into(), size: 5, diff --git a/src/server.rs b/src/server.rs index 939f44613..079768f16 100644 --- a/src/server.rs +++ b/src/server.rs @@ -399,7 +399,7 @@ pub fn start_server(config: &Config, port: u16) -> Result<()> { .build()?; let pool = runtime.handle().clone(); let dist_client = DistClientContainer::new(config, &pool); - let storage = storage_from_config(config, &pool); + let storage = storage_from_config(config, &pool)?; let res = SccacheServer::::new(port, runtime, client, dist_client, storage); let notify = env::var_os("SCCACHE_STARTUP_NOTIFY"); From fd10226dec2fe27e0fa6083572c5a4c49f30e5be Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 14:21:01 +0800 Subject: [PATCH 2/8] Make clippy happy Signed-off-by: Xuanwo --- src/cache/cache.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/cache/cache.rs b/src/cache/cache.rs index c4e13c149..38b6dae89 100644 --- a/src/cache/cache.rs +++ b/src/cache/cache.rs @@ -25,7 +25,16 @@ use crate::cache::memcached::MemcachedCache; use crate::cache::redis::RedisCache; #[cfg(feature = "s3")] use crate::cache::s3::S3Cache; -use crate::config::{self, CacheType, Config}; +use crate::config::Config; +#[cfg(any( + feature = "azure", + feature = "gcs", + feature = "gha", + feature = "memcached", + feature = "redis", + feature = "s3" +))] +use crate::config::{self, CacheType}; use std::fmt; use std::fs; use std::io::{self, Cursor, Read, Seek, Write}; @@ -326,7 +335,7 @@ pub trait Storage: Send + Sync { } /// Implement storage for operator. -#[cfg(any(feature = "s3", feature = "azure", feature = "gcs"))] +#[cfg(any(feature = "s3", feature = "azure", feature = "gcs", feature = "redis"))] #[async_trait] impl Storage for opendal::Operator { async fn get(&self, key: &str) -> Result { @@ -373,6 +382,7 @@ impl Storage for opendal::Operator { } /// Normalize key `abcdef` into `a/b/c/abcdef` +#[cfg(any(feature = "s3", feature = "azure", feature = "gcs", feature = "redis"))] fn normalize_key(key: &str) -> String { format!("{}/{}/{}/{}", &key[0..1], &key[1..2], &key[2..3], &key) } @@ -471,6 +481,15 @@ pub fn storage_from_config( return Ok(Arc::new(storage)); } + #[cfg(not(any( + feature = "azure", + feature = "gcs", + feature = "gha", + feature = "memcached", + feature = "redis", + feature = "s3" + )))] + _ => return Err(anyhow!("cache type is not enabled")), } } From 3de0c176e7add77339c28adb75bc867be7a43bdf Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 14:36:02 +0800 Subject: [PATCH 3/8] Remove not needed tests Signed-off-by: Xuanwo --- tests/sccache_cargo.rs | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/tests/sccache_cargo.rs b/tests/sccache_cargo.rs index 8f885b924..d1918bf35 100644 --- a/tests/sccache_cargo.rs +++ b/tests/sccache_cargo.rs @@ -302,24 +302,11 @@ fn test_gcp_arg_check() -> Result<()> { let mut cmd = Command::new(SCCACHE_BIN.as_os_str()); cmd.arg("--start-server") .env("SCCACHE_LOG", "debug") - .env("SCCACHE_GCS_OAUTH_URL", "http://foobar"); + .env("SCCACHE_GCS_OAUTH_URL", "http://127.0.0.1"); cmd.assert().failure().stderr(predicate::str::contains( "If setting GCS credentials, SCCACHE_GCS_BUCKET", )); - stop_sccache()?; - let mut cmd = Command::new(SCCACHE_BIN.as_os_str()); - cmd.arg("--start-server") - .env("SCCACHE_LOG", "debug") - .env("SCCACHE_GCS_BUCKET", "b") - .env("SCCACHE_GCS_OAUTH_URL", "http://foobar") - .env("SCCACHE_GCS_KEY_PATH", "foo.json"); - - // This is just a warning - cmd.assert().success().stderr(predicate::str::contains( - "SCCACHE_GCS_OAUTH_URL has been deprecated", - )); - Ok(()) } From 1196d494486c962574d0ecf36e2122fc63350b50 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 14:46:33 +0800 Subject: [PATCH 4/8] Allow normalize_key be dead Signed-off-by: Xuanwo --- src/cache/cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cache/cache.rs b/src/cache/cache.rs index 38b6dae89..cad93bf2e 100644 --- a/src/cache/cache.rs +++ b/src/cache/cache.rs @@ -382,7 +382,7 @@ impl Storage for opendal::Operator { } /// Normalize key `abcdef` into `a/b/c/abcdef` -#[cfg(any(feature = "s3", feature = "azure", feature = "gcs", feature = "redis"))] +#[allow(dead_code)] fn normalize_key(key: &str) -> String { format!("{}/{}/{}/{}", &key[0..1], &key[1..2], &key[2..3], &key) } From 79ef7d4c914b24534bfcca9a345b90d9f38f254d Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 18:37:09 +0800 Subject: [PATCH 5/8] storage error should be sent out Signed-off-by: Xuanwo --- src/cache/gcs.rs | 3 +++ src/server.rs | 21 +++++++++++++++++++-- tests/sccache_cargo.rs | 13 +++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/cache/gcs.rs b/src/cache/gcs.rs index c1fa3dc89..7fd656900 100644 --- a/src/cache/gcs.rs +++ b/src/cache/gcs.rs @@ -17,6 +17,7 @@ use crate::errors::*; use opendal::Operator; use opendal::{layers::LoggingLayer, services::gcs}; use reqsign::{GoogleBuilder, GoogleToken, GoogleTokenLoad}; +use url::Url; #[derive(Copy, Clone)] pub enum RWMode { @@ -59,6 +60,8 @@ impl GCSCache { signer_builder.credential_path(path); } if let Some(cred_url) = credential_url { + let _ = Url::parse(cred_url) + .map_err(|err| anyhow!("gcs credential url is invliad: {err:?}"))?; signer_builder.customed_token_loader(TaskClusterTokenLoader { client: reqwest::blocking::Client::default(), scope: rw_mode.to_scope().to_string(), diff --git a/src/server.rs b/src/server.rs index 079768f16..d27bfde02 100644 --- a/src/server.rs +++ b/src/server.rs @@ -399,10 +399,27 @@ pub fn start_server(config: &Config, port: u16) -> Result<()> { .build()?; let pool = runtime.handle().clone(); let dist_client = DistClientContainer::new(config, &pool); - let storage = storage_from_config(config, &pool)?; + + let notify = env::var_os("SCCACHE_STARTUP_NOTIFY"); + + let storage = match storage_from_config(config, &pool) { + Ok(storage) => storage, + Err(err) => { + error!("storage init failed for: {err:?}"); + + notify_server_startup( + ¬ify, + ServerStartup::Err { + reason: err.to_string(), + }, + )?; + + return Err(err); + } + }; + let res = SccacheServer::::new(port, runtime, client, dist_client, storage); - let notify = env::var_os("SCCACHE_STARTUP_NOTIFY"); match res { Ok(srv) => { let port = srv.port(); diff --git a/tests/sccache_cargo.rs b/tests/sccache_cargo.rs index d1918bf35..3b7258b0b 100644 --- a/tests/sccache_cargo.rs +++ b/tests/sccache_cargo.rs @@ -308,5 +308,18 @@ fn test_gcp_arg_check() -> Result<()> { "If setting GCS credentials, SCCACHE_GCS_BUCKET", )); + stop_sccache()?; + let mut cmd = Command::new(SCCACHE_BIN.as_os_str()); + cmd.arg("--start-server") + .env("SCCACHE_LOG", "debug") + .env("SCCACHE_GCS_BUCKET", "b") + .env("SCCACHE_GCS_CREDENTIALS_URL", "not_valid_url//127.0.0.1") + .env("SCCACHE_GCS_KEY_PATH", "foo.json"); + + // This is just a warning + cmd.assert() + .failure() + .stderr(predicate::str::contains("gcs credential url is invliad")); + Ok(()) } From 14b148848674df1755f4df791337ffc3f654f94f Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 18:45:45 +0800 Subject: [PATCH 6/8] Update tests/sccache_cargo.rs Co-authored-by: Sylvestre Ledru --- tests/sccache_cargo.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sccache_cargo.rs b/tests/sccache_cargo.rs index 3b7258b0b..779e3f4ea 100644 --- a/tests/sccache_cargo.rs +++ b/tests/sccache_cargo.rs @@ -319,7 +319,7 @@ fn test_gcp_arg_check() -> Result<()> { // This is just a warning cmd.assert() .failure() - .stderr(predicate::str::contains("gcs credential url is invliad")); + .stderr(predicate::str::contains("gcs credential url is invalid")); Ok(()) } From e7f4d9a23b096ea04b5f101c92b8eb78bca5a3c6 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 18:46:15 +0800 Subject: [PATCH 7/8] Fix typo Signed-off-by: Xuanwo --- src/cache/gcs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cache/gcs.rs b/src/cache/gcs.rs index 7fd656900..30324a820 100644 --- a/src/cache/gcs.rs +++ b/src/cache/gcs.rs @@ -61,7 +61,7 @@ impl GCSCache { } if let Some(cred_url) = credential_url { let _ = Url::parse(cred_url) - .map_err(|err| anyhow!("gcs credential url is invliad: {err:?}"))?; + .map_err(|err| anyhow!("gcs credential url is invalid: {err:?}"))?; signer_builder.customed_token_loader(TaskClusterTokenLoader { client: reqwest::blocking::Client::default(), scope: rw_mode.to_scope().to_string(), From 60305177fa67a0ecaf33df7e32f2e760c4fe1746 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 29 Dec 2022 22:21:11 +0800 Subject: [PATCH 8/8] Update src/cache/cache.rs Co-authored-by: Bernhard Schuster --- src/cache/cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cache/cache.rs b/src/cache/cache.rs index cad93bf2e..2a8786e61 100644 --- a/src/cache/cache.rs +++ b/src/cache/cache.rs @@ -489,7 +489,7 @@ pub fn storage_from_config( feature = "redis", feature = "s3" )))] - _ => return Err(anyhow!("cache type is not enabled")), + _ => bail!("cache type is not enabled"), } }