From 97d710ce9346f45ed21f8941924513e7ea656d5e Mon Sep 17 00:00:00 2001 From: Vlad Volodkin Date: Thu, 2 Jan 2025 19:23:00 +0000 Subject: [PATCH 1/3] Add an expected bucket owner test Signed-off-by: Vlad Volodkin --- mountpoint-s3/tests/common/cache.rs | 11 ++++ mountpoint-s3/tests/common/s3.rs | 11 ++++ mountpoint-s3/tests/fuse_tests/cache_test.rs | 64 +++++++++++++++++++- 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/mountpoint-s3/tests/common/cache.rs b/mountpoint-s3/tests/common/cache.rs index 481fc0c0f..d885f529d 100644 --- a/mountpoint-s3/tests/common/cache.rs +++ b/mountpoint-s3/tests/common/cache.rs @@ -25,6 +25,8 @@ struct CacheTestWrapperInner { get_block_invalid_count: AtomicU64, /// Number of times the `put_block` was completed put_block_count: AtomicU64, + /// Number of times the `put_block` was completed with a failure + put_block_fail_count: AtomicU64, } impl Clone for CacheTestWrapper { @@ -43,6 +45,7 @@ impl CacheTestWrapper { get_block_hit_count: AtomicU64::new(0), get_block_invalid_count: AtomicU64::new(0), put_block_count: AtomicU64::new(0), + put_block_fail_count: AtomicU64::new(0), }), } } @@ -74,6 +77,11 @@ impl CacheTestWrapper { pub fn put_block_count(&self) -> u64 { self.inner.put_block_count.load(Ordering::SeqCst) } + + /// Number of times the `put_block` was completed with failure + pub fn put_block_fail_count(&self) -> u64 { + self.inner.put_block_fail_count.load(Ordering::SeqCst) + } } #[async_trait] @@ -118,6 +126,9 @@ impl DataCache for CacheTestWrapper String { std::env::var("S3_REGION").expect("Set S3_REGION to run integration tests") } +/// Account ID owning buckets specified in `S3_BUCKET_NAME` and `S3_EXPRESS_ONE_ZONE_BUCKET_NAME` +pub fn get_bucket_owner() -> String { + std::env::var("S3_BUCKET_OWNER").expect("Set S3_BUCKET_OWNER to run integration tests") +} + +/// A name of an S3 Express bucket which is owned by a different account (different to `S3_BUCKET_OWNER`) +pub fn get_external_express_bucket() -> String { + std::env::var("S3_EXPRESS_ONE_ZONE_BUCKET_NAME_EXTERNAL") + .expect("Set S3_EXPRESS_ONE_ZONE_BUCKET_NAME_EXTERNAL to run integration tests") +} + /// Optional config for testing against a custom endpoint url pub fn get_test_endpoint_url() -> Option { if cfg!(feature = "s3express_tests") { diff --git a/mountpoint-s3/tests/fuse_tests/cache_test.rs b/mountpoint-s3/tests/fuse_tests/cache_test.rs index 806561503..39fd0e9c3 100644 --- a/mountpoint-s3/tests/fuse_tests/cache_test.rs +++ b/mountpoint-s3/tests/fuse_tests/cache_test.rs @@ -1,12 +1,17 @@ use crate::common::cache::CacheTestWrapper; use crate::common::fuse::create_fuse_session; use crate::common::fuse::s3_session::create_crt_client; -use crate::common::s3::{get_test_bucket, get_test_prefix}; +use crate::common::s3::{ + get_bucket_owner, get_external_express_bucket, get_test_bucket, get_test_endpoint_config, get_test_prefix, +}; +use futures::executor::block_on; use mountpoint_s3::data_cache::{DataCache, DiskDataCache, DiskDataCacheConfig}; use mountpoint_s3::object::ObjectId; use mountpoint_s3::prefetch::caching_prefetch; -use mountpoint_s3_client::S3CrtClient; +use mountpoint_s3_client::config::S3ClientConfig; +use mountpoint_s3_client::error::ObjectClientError; +use mountpoint_s3_client::{S3CrtClient, S3RequestError}; use fuser::BackgroundSession; use rand::{Rng, RngCore, SeedableRng}; @@ -288,6 +293,61 @@ where assert!(block.is_none()); } +/// A test that checks that data is not written to the bucket owned by an unexpected account +#[test_case(get_express_bucket(), true, true; "bucket owned by the expected account")] +#[test_case(get_external_express_bucket(), true, false; "bucket owned by another account")] +#[test_case(get_external_express_bucket(), false, false; "bucket owned by another account, not checked")] +fn express_cache_expected_bucket_owner(cache_bucket: String, owner_checked: bool, owner_matches: bool) { + let bucket_owner = get_bucket_owner(); + // Configure the client to enforce the bucket owner + let mut client_config = S3ClientConfig::default() + .part_size(CLIENT_PART_SIZE) + .endpoint_config(get_test_endpoint_config()) + .read_backpressure(true) + .initial_read_window(CLIENT_PART_SIZE); + if owner_checked { + client_config = client_config.bucket_owner(&bucket_owner); + } + let client = S3CrtClient::new(client_config).unwrap(); + + // Create cache and mount a bucket + let bucket = get_standard_bucket(); + let prefix = get_test_prefix("express_expected_bucket_owner"); + let cache = ExpressDataCache::new(client.clone(), Default::default(), &bucket, &cache_bucket); + let cache_valid = block_on(cache.verify_cache_valid()); + if owner_checked && !owner_matches { + match cache_valid { + Err(ObjectClientError::ClientError(S3RequestError::Forbidden(..))) => (), + _ => panic!("expected S3RequestError::Forbidden, got: {:?}", cache_valid), + } + } else { + cache_valid.unwrap(); // Ok(()) is expected + } + + let cache = CacheTestWrapper::new(cache); + let (mount_point, _session) = mount_bucket(client, cache.clone(), &bucket, &prefix); + + // Write an object, no caching happens yet + let key = get_random_key(&prefix, "key", 100); + let path = mount_point.path().join(&key); + let written = random_binary_data(CACHE_BLOCK_SIZE as usize); + fs::write(&path, &written).expect("write should succeed"); + + // First read should be from the source bucket and be cached + let put_block_count = cache.put_block_count(); + let read = fs::read(&path).expect("read should succeed"); + assert_eq!(read, written); + + // Cache population is async, wait for it to happen + cache.wait_for_put(Duration::from_secs(10), put_block_count); + + if owner_checked && !owner_matches { + assert_eq!(cache.put_block_fail_count(), cache.put_block_count()); + } else { + assert_eq!(cache.put_block_fail_count(), 0); + } +} + /// Generates random data of the specified size fn random_binary_data(size_in_bytes: usize) -> Vec { let seed = rand::thread_rng().gen(); From fdf37198181f929c9a0e31e2dbb50b06d67d7496 Mon Sep 17 00:00:00 2001 From: Vlad Volodkin Date: Wed, 22 Jan 2025 16:33:27 +0000 Subject: [PATCH 2/3] Add s3express_tests feature flag Signed-off-by: Vlad Volodkin --- mountpoint-s3/tests/fuse_tests/cache_test.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/mountpoint-s3/tests/fuse_tests/cache_test.rs b/mountpoint-s3/tests/fuse_tests/cache_test.rs index 39fd0e9c3..152917ce3 100644 --- a/mountpoint-s3/tests/fuse_tests/cache_test.rs +++ b/mountpoint-s3/tests/fuse_tests/cache_test.rs @@ -1,17 +1,12 @@ use crate::common::cache::CacheTestWrapper; use crate::common::fuse::create_fuse_session; use crate::common::fuse::s3_session::create_crt_client; -use crate::common::s3::{ - get_bucket_owner, get_external_express_bucket, get_test_bucket, get_test_endpoint_config, get_test_prefix, -}; +use crate::common::s3::{get_test_bucket, get_test_prefix}; -use futures::executor::block_on; use mountpoint_s3::data_cache::{DataCache, DiskDataCache, DiskDataCacheConfig}; use mountpoint_s3::object::ObjectId; use mountpoint_s3::prefetch::caching_prefetch; -use mountpoint_s3_client::config::S3ClientConfig; -use mountpoint_s3_client::error::ObjectClientError; -use mountpoint_s3_client::{S3CrtClient, S3RequestError}; +use mountpoint_s3_client::S3CrtClient; use fuser::BackgroundSession; use rand::{Rng, RngCore, SeedableRng}; @@ -22,7 +17,9 @@ use tempfile::TempDir; use test_case::test_case; #[cfg(feature = "s3express_tests")] -use crate::common::s3::{get_express_bucket, get_standard_bucket}; +use crate::common::s3::{ + get_bucket_owner, get_express_bucket, get_external_express_bucket, get_standard_bucket, get_test_endpoint_config, +}; #[cfg(feature = "s3express_tests")] use mountpoint_s3::data_cache::{build_prefix, get_s3_key, BlockIndex, ExpressDataCache}; #[cfg(feature = "s3express_tests")] @@ -297,7 +294,13 @@ where #[test_case(get_express_bucket(), true, true; "bucket owned by the expected account")] #[test_case(get_external_express_bucket(), true, false; "bucket owned by another account")] #[test_case(get_external_express_bucket(), false, false; "bucket owned by another account, not checked")] +#[cfg(feature = "s3express_tests")] fn express_cache_expected_bucket_owner(cache_bucket: String, owner_checked: bool, owner_matches: bool) { + use futures::executor::block_on; + use mountpoint_s3_client::config::S3ClientConfig; + use mountpoint_s3_client::error::ObjectClientError; + use mountpoint_s3_client::S3RequestError; + let bucket_owner = get_bucket_owner(); // Configure the client to enforce the bucket owner let mut client_config = S3ClientConfig::default() From 605ea4592ff081e5682130a01032b9abf93d2b16 Mon Sep 17 00:00:00 2001 From: Vlad Volodkin Date: Thu, 23 Jan 2025 18:48:24 +0000 Subject: [PATCH 3/3] Make the test optional Signed-off-by: Vlad Volodkin --- mountpoint-s3/tests/fuse_tests/cache_test.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mountpoint-s3/tests/fuse_tests/cache_test.rs b/mountpoint-s3/tests/fuse_tests/cache_test.rs index 152917ce3..78760407b 100644 --- a/mountpoint-s3/tests/fuse_tests/cache_test.rs +++ b/mountpoint-s3/tests/fuse_tests/cache_test.rs @@ -16,10 +16,10 @@ use std::time::Duration; use tempfile::TempDir; use test_case::test_case; +#[cfg(all(feature = "s3express_tests", feature = "second_account_tests"))] +use crate::common::s3::{get_bucket_owner, get_external_express_bucket, get_test_endpoint_config}; #[cfg(feature = "s3express_tests")] -use crate::common::s3::{ - get_bucket_owner, get_express_bucket, get_external_express_bucket, get_standard_bucket, get_test_endpoint_config, -}; +use crate::common::s3::{get_express_bucket, get_standard_bucket}; #[cfg(feature = "s3express_tests")] use mountpoint_s3::data_cache::{build_prefix, get_s3_key, BlockIndex, ExpressDataCache}; #[cfg(feature = "s3express_tests")] @@ -294,7 +294,7 @@ where #[test_case(get_express_bucket(), true, true; "bucket owned by the expected account")] #[test_case(get_external_express_bucket(), true, false; "bucket owned by another account")] #[test_case(get_external_express_bucket(), false, false; "bucket owned by another account, not checked")] -#[cfg(feature = "s3express_tests")] +#[cfg(all(feature = "s3express_tests", feature = "second_account_tests"))] fn express_cache_expected_bucket_owner(cache_bucket: String, owner_checked: bool, owner_matches: bool) { use futures::executor::block_on; use mountpoint_s3_client::config::S3ClientConfig; @@ -324,7 +324,7 @@ fn express_cache_expected_bucket_owner(cache_bucket: String, owner_checked: bool _ => panic!("expected S3RequestError::Forbidden, got: {:?}", cache_valid), } } else { - cache_valid.unwrap(); // Ok(()) is expected + cache_valid.expect("should succeed if not enforcing bucket owner"); } let cache = CacheTestWrapper::new(cache);