Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow enforcing SSE settings for the cache bucket #1131

Closed
wants to merge 6 commits into from
Closed

Allow enforcing SSE settings for the cache bucket #1131

wants to merge 6 commits into from

Conversation

vladem
Copy link
Contributor

@vladem vladem commented Nov 13, 2024 •
edited
Loading

Description of change

Use --sse and --sse-kms-key-id to enforce a specific SSE configuration on the shared cache bucket.

#1071 and #1132 should be merged before this.

Relevant issues:

Does this change impact existing behavior?

No.

Does this change need a changelog entry in any of the crates?

Yes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@vladem vladem temporarily deployed to PR integration tests November 13, 2024 16:11 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests November 13, 2024 16:11 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests November 13, 2024 16:11 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests November 13, 2024 16:11 — with GitHub Actions Inactive
Get SSE from GetObject, add tests for SSE in Express
e802320 
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
Remove redundant error variant
22d1130 
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
Improve CacheTestWrapper
10777a8 
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
@vladem vladem temporarily deployed to PR integration tests November 14, 2024 00:23 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests November 14, 2024 00:23 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests November 14, 2024 00:23 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests November 14, 2024 00:23 — with GitHub Actions Inactive
Remove wrong comments
0f3e5b0 
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
muddyfish
muddyfish reviewed Nov 14, 2024
View reviewed changes
mountpoint-s3-client/tests/get_object.rs
.get_object(
&bucket,
&key,
&GetObjectParams::new().checksum_mode(Some(ChecksumMode::Enabled)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need checksums enabled here right?

mountpoint-s3-client/tests/get_object.rs

let (received_sse, received_key_id) = result.get_object_sse().await.expect("should return sse settings");
assert_eq!(received_sse.expect("sse type must be some"), sse_type);
if let Some(kms_key_id) = kms_key_id {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's None, we should assert that there's no key received, or it's S3 SSE

mountpoint-s3/src/data_cache/express_data_cache.rs
// instead of returning an error because:
// 1. this error would only be reported to logs because the cache population is an async process
// 2. the reported error is severe as the object was already uploaded to S3.
std::process::exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This point should only be reached if we try to write to S3 with some set of settings, and S3 told us it ignored us?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hit this path if the bucket's encryption settings change after startup?

Copy link
Contributor Author

@vladem vladem Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S3 told us it ignored us

Yes, only in that case, which may also happen if CRT stops adding those headers to PutObject requests to S3 Express.

Can we hit this path if the bucket's encryption settings change after startup?

No, in this case we expect to bail out with PutObjectSingle error at line 157. We provide the settings that S3 is expected to enforce. If it is not able to do that (because the default for the bucket is now different), the PutObject will fail.

mountpoint-s3/tests/common/cache.rs
put_block_failed_count: Arc<AtomicU64>,
}

impl<Cache> Clone for CacheTestWrapper<Cache> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not derive clone and it just work here?

mountpoint-s3/tests/common/cache.rs
};

/// A wrapper around any type implementing [DataCache], which counts operations
pub struct CacheTestWrapper<Cache> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a test wrapper, or a metrics wrapper? It feels to me this would work fine for emitting metrics for the most part

mountpoint-s3/tests/fuse_tests/cache_test.rs
let full_key_size = full_key.as_bytes().len();
let padding_size = min_size_in_bytes.saturating_sub(full_key_size);
let padding = "0".repeat(padding_size);
format!("{last_key_part}{padding}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why last_key_part here?

mountpoint-s3/tests/fuse_tests/cache_test.rs
// Creates a random key which has a size of at least `min_size_in_bytes`
fn get_object_key(key_prefix: &str, key_suffix: &str, min_size_in_bytes: usize) -> String {
let random_suffix: u64 = rand::thread_rng().gen();
let last_key_part = format!("{key_suffix}{random_suffix}"); // part of the key after all the "/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ending in key_suffix feels weird given the name

mountpoint-s3/tests/fuse_tests/cache_test.rs
assert_eq!(express_cache.put_block_failed_count(), 0);
}

// TODO: check with sdk client that data is stored with the right settings or not stored at all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably do this as part of this PR - just noticed it's a draft

@vladem
Copy link
Contributor Author

vladem commented Nov 14, 2024

This will be addressed later.

@vladem vladem closed this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muddyfish muddyfish muddyfish left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vladem @muddyfish