fix(storage/s3): default to virtual-host-style addressing

Author: plusplusjiajia

crates/iceberg/src/io/storage/config/s3.rs

@@ -69,8 +69,14 @@ pub const S3_DISABLE_CONFIG_LOAD: &str = "s3.disable-config-load";
 ///
 /// This struct contains all the configuration options for connecting to Amazon S3.
 /// Use the builder pattern via `S3Config::builder()` to construct instances.
-/// ```
-#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, TypedBuilder)]
+///
+/// Defaults follow the Iceberg `S3FileIOProperties` spec (see
+/// [`PATH_STYLE_ACCESS_DEFAULT = false`](https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java)),
+/// i.e. virtual-host-style addressing is enabled unless
+/// `s3.path-style-access=true` is explicitly set. This matches what
+/// Java clients do out of the box and is required for a number of
+/// S3-compatible stores that do not support path-style URLs.
+#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, TypedBuilder)]
 pub struct S3Config {
     /// S3 endpoint URL.
     #[builder(default, setter(strip_option, into))]
@@ -88,7 +94,9 @@ pub struct S3Config {
     #[builder(default, setter(strip_option, into))]
     pub region: Option<String>,
     /// Enable virtual host style (opposite of path style access).
-    #[builder(default)]
+    ///
+    /// Defaults to `true` to match Iceberg `S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT = false`.
+    #[builder(default = true)]
     pub enable_virtual_host_style: bool,
     /// Server side encryption type.
     #[builder(default, setter(strip_option, into))]
@@ -125,6 +133,12 @@ pub struct S3Config {
     pub disable_config_load: bool,
 }
 
+impl Default for S3Config {
+    fn default() -> Self {
+        Self::builder().build()
+    }
+}
+
 impl TryFrom<&StorageConfig> for S3Config {
     type Error = crate::Error;
 
@@ -267,6 +281,17 @@ mod tests {
         assert_eq!(s3_config.region.as_deref(), Some("eu-west-1"));
     }
 
+    #[test]
+    fn test_s3_config_default_is_virtual_host_style() {
+        // Matches Iceberg S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT = false.
+        assert!(S3Config::default().enable_virtual_host_style);
+        assert!(
+            S3Config::try_from(&StorageConfig::new())
+                .unwrap()
+                .enable_virtual_host_style
+        );
+    }
+
     #[test]
     fn test_s3_config_path_style_access() {
         let storage_config = StorageConfig::new().with_prop(S3_PATH_STYLE_ACCESS, "true");

crates/storage/opendal/src/s3.rs

@@ -37,6 +37,12 @@ use crate::utils::{from_opendal_error, is_truthy};
 /// Parse iceberg props to s3 config.
 pub(crate) fn s3_config_parse(mut m: HashMap<String, String>) -> Result<S3Config> {
     let mut cfg = S3Config::default();
+    // Match Iceberg `S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT = false`:
+    // virtual-host-style addressing is the spec default. opendal's own
+    // default is path-style, which disagrees with the Java SDK and breaks
+    // S3-compatible stores that only accept virtual-hosted-style URLs.
+    // Any explicit `s3.path-style-access` property below overrides this.
+    cfg.enable_virtual_host_style = true;
     if let Some(endpoint) = m.remove(S3_ENDPOINT) {
         cfg.endpoint = Some(endpoint);
     };
@@ -177,3 +183,28 @@ impl AwsCredentialLoad for CustomAwsCredentialLoader {
         self.0.load_credential(client).await
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::collections::HashMap;
+
+    use iceberg::io::S3_PATH_STYLE_ACCESS;
+
+    use super::s3_config_parse;
+
+    fn parse_with(prop: Option<&str>) -> bool {
+        let mut props = HashMap::new();
+        if let Some(v) = prop {
+            props.insert(S3_PATH_STYLE_ACCESS.to_string(), v.to_string());
+        }
+        s3_config_parse(props).unwrap().enable_virtual_host_style
+    }
+
+    #[test]
+    fn s3_config_parse_path_style_access() {
+        // Match Iceberg S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT = false.
+        assert!(parse_with(None));
+        assert!(parse_with(Some("false")));
+        assert!(!parse_with(Some("true")));
+    }
+}