zeroshade commented on code in PR #1506:
URL: https://github.com/apache/iceberg-go/pull/1506#discussion_r3647331083
##########
io/gocloud/s3.go:
##########
@@ -167,18 +169,35 @@ func resolveUsePathStyle(endpoint string, props
map[string]string) bool {
return usePathStyle
}
-func createS3Bucket(ctx context.Context, parsed *url.URL, props
map[string]string) (*blob.Bucket, error) {
+// resolveS3AWSConfig returns the AWS config for the S3 FileIO, preferring an
+// ambient context config but letting explicit s3.* credentials override it.
+func resolveS3AWSConfig(ctx context.Context, props map[string]string)
(*aws.Config, error) {
var (
- awscfg *aws.Config
- err error
+ base *aws.Config
+ err error
)
if v := utils.GetAwsConfig(ctx); v != nil {
Review Comment:
When a context AWS config is present, `ParseAWSConfig` is skipped, so
explicit `s3.region`/`client.region` and other parsed config props don't
override the context config. Endpoint/path-style are applied later, but region
precedence is wrong for context-backed setups.
##########
io/gocloud/s3.go:
##########
@@ -167,18 +169,35 @@ func resolveUsePathStyle(endpoint string, props
map[string]string) bool {
return usePathStyle
}
-func createS3Bucket(ctx context.Context, parsed *url.URL, props
map[string]string) (*blob.Bucket, error) {
+// resolveS3AWSConfig returns the AWS config for the S3 FileIO, preferring an
+// ambient context config but letting explicit s3.* credentials override it.
+func resolveS3AWSConfig(ctx context.Context, props map[string]string)
(*aws.Config, error) {
var (
- awscfg *aws.Config
- err error
+ base *aws.Config
+ err error
)
if v := utils.GetAwsConfig(ctx); v != nil {
- awscfg = v
- } else {
- awscfg, err = ParseAWSConfig(ctx, props)
- if err != nil {
- return nil, err
- }
+ base = v
+ } else if base, err = ParseAWSConfig(ctx, props); err != nil {
+ return nil, err
+ }
+
+ // Always copy so neither the credential override nor the caller's later
+ // mutations (e.g. HTTPClient) touch a shared context config.
+ cfg := *base
+ if props[io.S3AccessKeyID] != "" || props[io.S3SecretAccessKey] != "" {
Review Comment:
Partial credential override clobbers unset fields: setting only
`s3.access-key-id` installs a static provider with an empty secret;
`s3.session-token`-only is ignored (not in the condition). Unset credential
keys should preserve the ambient/context credentials.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
