From 69b37505b0f58fe102132fc10ff0afa4e2cdc30a Mon Sep 17 00:00:00 2001 From: darox Date: Mon, 3 Aug 2026 10:37:51 +0200 Subject: [PATCH] fix(relay): make Ceph ETag normalization opt-in Signed-off-by: darox --- .env.example | 3 + crates/buzz-relay/src/api/git/store.rs | 140 ++++++++++++++++++++++++- crates/buzz-relay/src/config.rs | 59 +++++++++++ crates/buzz-relay/src/state.rs | 3 +- docs/git-on-object-storage.md | 6 +- 5 files changed, 207 insertions(+), 4 deletions(-) diff --git a/.env.example b/.env.example index b9bfcada0e..e5f7590dd0 100644 --- a/.env.example +++ b/.env.example @@ -81,6 +81,9 @@ RELAY_URL=ws://localhost:3000 # BUZZ_GIT_PACK_CACHE_PATH=./repos/.pack-cache # BUZZ_GIT_PACK_CACHE_MAX_BYTES=5368709120 # BUZZ_GIT_PACK_CACHE_MAX_CONCURRENT_POPULATIONS=2 +# Preserve quoted ETags in If-Match by default (standard S3/MinIO). Set this +# only when a Ceph RGW deployment rejects the standards-compliant quoted form. +# BUZZ_GIT_S3_COMPATIBILITY=ceph-rgw # ----------------------------------------------------------------------------- # S3-Compatible Object Storage (media + Git/CAS) diff --git a/crates/buzz-relay/src/api/git/store.rs b/crates/buzz-relay/src/api/git/store.rs index bdfca8dcf2..3d3946c5ec 100644 --- a/crates/buzz-relay/src/api/git/store.rs +++ b/crates/buzz-relay/src/api/git/store.rs @@ -36,6 +36,31 @@ use sha2::{Digest, Sha256}; #[derive(Debug, Clone, PartialEq, Eq)] pub struct ETag(pub String); +/// Backend-specific compatibility applied to conditional S3 requests. +/// +/// The default preserves ETags byte-for-byte as required by HTTP and standard +/// S3 implementations. Non-standard behavior must be selected explicitly. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum S3BackendCompatibility { + /// Preserve the response ETag exactly when sending `If-Match`. + #[default] + Standard, + /// Remove one surrounding double-quote pair from `If-Match` for Ceph RGW. + CephRgw, +} + +impl S3BackendCompatibility { + fn if_match_value(self, etag: &str) -> &str { + match self { + Self::Standard => etag, + Self::CephRgw => etag + .strip_prefix('"') + .and_then(|unquoted| unquoted.strip_suffix('"')) + .unwrap_or(etag), + } + } +} + /// Precondition for `put_pointer`. #[derive(Debug, Clone)] pub enum Precond { @@ -169,6 +194,7 @@ impl From for StoreError { #[derive(Clone)] pub struct GitStore { bucket: Arc, + compatibility: S3BackendCompatibility, } impl GitStore { @@ -193,6 +219,30 @@ impl GitStore { bucket_name: &str, region: &str, addressing_style: buzz_media::config::S3AddressingStyle, + ) -> Result { + Self::new_with_compatibility( + endpoint, + access_key, + secret_key, + bucket_name, + region, + addressing_style, + S3BackendCompatibility::Standard, + ) + } + + /// Build a client with an explicit backend compatibility policy. + /// + /// Use [`S3BackendCompatibility::Standard`] unless the target backend is + /// known to require a documented compatibility exception. + pub fn new_with_compatibility( + endpoint: &str, + access_key: &str, + secret_key: &str, + bucket_name: &str, + region: &str, + addressing_style: buzz_media::config::S3AddressingStyle, + compatibility: S3BackendCompatibility, ) -> Result { let region = Region::Custom { region: region.into(), @@ -219,6 +269,7 @@ impl GitStore { }; Ok(Self { bucket: Arc::from(bucket), + compatibility, }) } @@ -495,12 +546,13 @@ impl GitStore { headers.insert(axum::http::header::IF_NONE_MATCH, "*".parse().unwrap()); } Precond::IfMatch(ETag(tag)) => { + let wire_tag = self.compatibility.if_match_value(tag); headers.insert( axum::http::header::IF_MATCH, - tag.parse().map_err(|_| { + wire_tag.parse().map_err(|_| { StoreError::Backend(S3Error::HttpFailWithBody( 400, - format!("invalid etag {tag}"), + format!("invalid etag {wire_tag}"), )) })?, ); @@ -916,6 +968,77 @@ impl GitStore { mod tests { use super::*; + async fn capture_if_match(compatibility: Option, etag: &str) -> String { + async fn handler( + axum::extract::State(seen): axum::extract::State>>>, + headers: axum::http::HeaderMap, + ) -> impl axum::response::IntoResponse { + if let Some(value) = headers.get(axum::http::header::IF_MATCH) { + seen.lock().await.push( + value + .to_str() + .expect("If-Match should be ASCII") + .to_string(), + ); + } + ( + axum::http::StatusCode::OK, + [(axum::http::header::ETAG, "\"next-etag\"")], + ) + } + + let seen = Arc::new(tokio::sync::Mutex::new(Vec::new())); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind test S3 listener"); + let address = listener.local_addr().expect("test S3 listener address"); + let app = axum::Router::new() + .fallback(handler) + .with_state(Arc::clone(&seen)); + let server = tokio::spawn(async move { + axum::serve(listener, app) + .await + .expect("serve test S3 endpoint"); + }); + + let endpoint = format!("http://{address}"); + let store = match compatibility { + Some(compatibility) => GitStore::new_with_compatibility( + &endpoint, + "access-key", + "secret-key", + "bucket", + "us-east-1", + buzz_media::config::S3AddressingStyle::Path, + compatibility, + ), + None => GitStore::new( + &endpoint, + "access-key", + "secret-key", + "bucket", + "us-east-1", + buzz_media::config::S3AddressingStyle::Path, + ), + } + .expect("build test git store"); + let outcome = store + .put_pointer( + "pointer", + br#"{"manifest":"digest"}"#, + Precond::IfMatch(ETag(etag.to_string())), + ) + .await + .expect("test pointer PUT"); + assert!(matches!(outcome, CasOutcome::Won(_))); + + server.abort(); + let _ = server.await; + let values = seen.lock().await; + assert_eq!(values.len(), 1, "expected one If-Match header"); + values[0].clone() + } + #[test] fn idx_key_uses_pack_digest_namespace() { let digest = "a".repeat(64); @@ -947,6 +1070,19 @@ mod tests { )); } + #[tokio::test] + async fn standard_s3_preserves_quoted_if_match_on_wire() { + let value = capture_if_match(None, "\"opaque-etag\"").await; + assert_eq!(value, "\"opaque-etag\""); + } + + #[tokio::test] + async fn ceph_rgw_strips_quotes_from_if_match_on_wire() { + let value = + capture_if_match(Some(S3BackendCompatibility::CephRgw), "\"opaque-etag\"").await; + assert_eq!(value, "opaque-etag"); + } + #[test] fn static_keys_build_store_with_configured_region() { let store = GitStore::new( diff --git a/crates/buzz-relay/src/config.rs b/crates/buzz-relay/src/config.rs index 85a0ca2efe..df44f38e5b 100644 --- a/crates/buzz-relay/src/config.rs +++ b/crates/buzz-relay/src/config.rs @@ -235,6 +235,12 @@ pub struct Config { pub git_repo_path: std::path::PathBuf, /// Parent directory for process-isolated immutable pack cache sessions. pub git_pack_cache_path: std::path::PathBuf, + /// Conditional-request compatibility for the git object-store backend. + /// + /// Defaults to standards-compliant, opaque ETag forwarding. Set + /// `BUZZ_GIT_S3_COMPATIBILITY=ceph-rgw` only for Ceph RGW deployments that + /// reject quoted ETags in `If-Match`. + pub git_s3_compatibility: crate::api::git::store::S3BackendCompatibility, /// Maximum pack file size for git push (bytes). Default: 500 MB. pub git_max_pack_bytes: u64, /// Maximum total bytes materialized for one git repo request. Default: 1 GB. @@ -770,6 +776,27 @@ impl Config { .map(std::path::PathBuf::from) .unwrap_or_else(|_| git_repo_path.join(".pack-cache")), )?; + let git_s3_compatibility = match std::env::var("BUZZ_GIT_S3_COMPATIBILITY") { + Err(std::env::VarError::NotPresent) => { + crate::api::git::store::S3BackendCompatibility::Standard + } + Ok(value) if value.eq_ignore_ascii_case("standard") => { + crate::api::git::store::S3BackendCompatibility::Standard + } + Ok(value) if value.eq_ignore_ascii_case("ceph-rgw") => { + crate::api::git::store::S3BackendCompatibility::CephRgw + } + Ok(value) => { + return Err(ConfigError::InvalidValue(format!( + "BUZZ_GIT_S3_COMPATIBILITY must be 'standard' or 'ceph-rgw', got {value:?}" + ))); + } + Err(std::env::VarError::NotUnicode(_)) => { + return Err(ConfigError::InvalidValue( + "BUZZ_GIT_S3_COMPATIBILITY must be valid Unicode".to_string(), + )); + } + }; let git_max_pack_bytes: u64 = std::env::var("BUZZ_GIT_MAX_PACK_BYTES") .ok() .and_then(|v| v.parse().ok()) @@ -970,6 +997,7 @@ impl Config { ephemeral_ttl_override, git_repo_path, git_pack_cache_path, + git_s3_compatibility, git_max_pack_bytes, git_max_repo_bytes, git_pack_cache_max_bytes, @@ -1051,6 +1079,37 @@ mod tests { config.huddle_audio_available, "huddle_audio_available should default to true so single-pod (N=1) keeps today's huddle behavior" ); + assert_eq!( + config.git_s3_compatibility, + crate::api::git::store::S3BackendCompatibility::Standard, + "git S3 compatibility must default to standards-compliant ETag forwarding" + ); + } + + #[test] + fn git_s3_compatibility_is_explicit_and_validated() { + let _guard = ENV_MUTEX.lock().unwrap(); + let previous = std::env::var_os("BUZZ_GIT_S3_COMPATIBILITY"); + + std::env::set_var("BUZZ_GIT_S3_COMPATIBILITY", "ceph-rgw"); + let ceph = Config::from_env().expect("Ceph compatibility config"); + assert_eq!( + ceph.git_s3_compatibility, + crate::api::git::store::S3BackendCompatibility::CephRgw + ); + + std::env::set_var("BUZZ_GIT_S3_COMPATIBILITY", "auto"); + assert!(matches!( + Config::from_env(), + Err(ConfigError::InvalidValue(message)) + if message.contains("BUZZ_GIT_S3_COMPATIBILITY") + )); + + if let Some(value) = previous { + std::env::set_var("BUZZ_GIT_S3_COMPATIBILITY", value); + } else { + std::env::remove_var("BUZZ_GIT_S3_COMPATIBILITY"); + } } #[test] diff --git a/crates/buzz-relay/src/state.rs b/crates/buzz-relay/src/state.rs index 58a869a995..7805baa5e1 100644 --- a/crates/buzz-relay/src/state.rs +++ b/crates/buzz-relay/src/state.rs @@ -691,13 +691,14 @@ impl AppState { let git_max_concurrent_ops = config.git_max_concurrent_ops; let media_max_concurrent_uploads = config.media_max_concurrent_uploads; - let git_store = crate::api::git::store::GitStore::new( + let git_store = crate::api::git::store::GitStore::new_with_compatibility( &config.media.s3_endpoint, &config.media.s3_access_key, &config.media.s3_secret_key, &config.media.s3_bucket, &config.media.s3_region, config.media.s3_addressing_style, + config.git_s3_compatibility, ) .expect("media storage was already constructed with this S3 config"); let git_pack_cache = Arc::new( diff --git a/docs/git-on-object-storage.md b/docs/git-on-object-storage.md index 1d87720edc..592cd804b3 100644 --- a/docs/git-on-object-storage.md +++ b/docs/git-on-object-storage.md @@ -366,7 +366,11 @@ conditional writes cannot admit a backend against it. 4. **ETag-token consistency.** Verify HEAD-path and GET-path ETag extraction agree byte-for-byte (quoting included). `If-Match` compares tokens literally; a quote mismatch between the read path and the write path silently tests the - wrong thing. The probe must use the exact token format the pointer write uses. + wrong thing. The standard S3/MinIO policy therefore forwards the token + byte-for-byte. Ceph RGW deployments that reject quoted `If-Match` values may + explicitly select `BUZZ_GIT_S3_COMPATIBILITY=ceph-rgw`, which removes one + surrounding quote pair only for that header. The probe must use the exact + token format selected for the pointer write. **Proof surface (explicit non-goals of the probe and the design).** The protocol depends only on conditional writes of *small single objects* (the manifest