fix(s3): remove dynamic force-path-style addressing (#1648)#2193
fix(s3): remove dynamic force-path-style addressing (#1648)#2193Anshgrover23 wants to merge 6 commits intoconda:mainfrom
Conversation
|
@baszalmstra Could I get a review on this one whenever you get a chance? Thanks. |
Anshgrover23
force-pushed
the
fix/1648-remove-dynamic-s3-path-style
branch
from
adbbab9 to
78d0d9d
Compare
| # Create rattler config with S3 options for MinIO (path-style addressing) | ||
| let config_content = $"[s3-options.($bucket_name)] | ||
| endpoint-url = \"http://localhost:9000\" | ||
| force-path-style = true |
There was a problem hiding this comment.
while we're at it, i think we should rename this to addressing-style: path | virtual-host. we should print a warning when force-path-style is still used
| --region "us-east-1" | ||
| --endpoint-url "http://localhost:9000" | ||
| --addressing-style path |
There was a problem hiding this comment.
is changing this necessary? CLI args should already result in S3CredentialsOpts being set, no?
| AWS_ACCESS_KEY_ID: $root_user | ||
| AWS_SECRET_ACCESS_KEY: $root_password | ||
| AWS_REGION: "us-east-1" |
There was a problem hiding this comment.
we should be using the S3_ variants (that are not loaded through aws-sdk: https://github.com/pavelzw/rattler/blob/a8364172f90f752eed7e49f3be73c28878240631/crates/rattler_s3/src/clap.rs#L52
passing them as CLI flags is also fine
| if let Some(s3_config) = s3_config { | ||
| credentials.addressing_style = if s3_config.force_path_style { | ||
| rattler_s3::clap::S3AddressingStyleOpts::Path | ||
| } else { | ||
| rattler_s3::clap::S3AddressingStyleOpts::VirtualHost | ||
| }; | ||
| } |
There was a problem hiding this comment.
we should be using credentials.addressing_style.or(...).
this also means that https://github.com/pavelzw/rattler/blob/a8364172f90f752eed7e49f3be73c28878240631/crates/rattler_s3/src/clap.rs#L61 should be a Option<S3AddressingStyleOpts>.
@baszalmstra was there a reason why you made this not an Option back then?
2 participants
Description
Removes dynamic
force-path-styleaddressing and adds explicit S3 configuration viarattler_config.Changes:
[s3-options.bucket]withforce-path-style,endpoint-url, andregion.--configto load rattler_config and pass S3 options to the S3 middleware.--configis used, applyforce_path_stylefrom rattler_config to credentials.--configfor bothrattler-indexandrattler create.Fixes #1648
How Has This Been Tested?
pixi run cargo nextest run -p rattler_networking --features s3 s3_middleware(unit tests)pixi run e2e-s3-minio(MinIO e2e)Checklist