From db751af40c3973f330beda6e1c0368ebc729cc87 Mon Sep 17 00:00:00 2001 From: DylanGuedes Date: Fri, 17 Oct 2025 16:08:06 -0300 Subject: [PATCH 1/2] fix how the url is built --- pkg/storage/chunk/client/aws/config.go | 5 +- pkg/storage/chunk/client/aws/config_test.go | 71 +++++++++++++++++++ .../chunk/client/aws/s3_storage_client.go | 6 +- 3 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 pkg/storage/chunk/client/aws/config_test.go diff --git a/pkg/storage/chunk/client/aws/config.go b/pkg/storage/chunk/client/aws/config.go index 5512d0645f4d2..f1d4469d604ca 100644 --- a/pkg/storage/chunk/client/aws/config.go +++ b/pkg/storage/chunk/client/aws/config.go @@ -16,7 +16,6 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/dynamodb" - "github.com/pkg/errors" ) // DynamoConfigFromURL returns AWS config from given URL. It expects escaped @@ -83,5 +82,7 @@ func CredentialsFromURL(awsURL *url.URL) (key, secret, session string, err error return username, password, "", nil } } - return "", "", "", errors.New("Unable to build AWS credentials from URL") + // Return empty credentials instead of error to allow AWS SDK to use default credential chain + // (environment variables, IAM roles, etc.) + return "", "", "", nil } diff --git a/pkg/storage/chunk/client/aws/config_test.go b/pkg/storage/chunk/client/aws/config_test.go new file mode 100644 index 0000000000000..610453e7b32d1 --- /dev/null +++ b/pkg/storage/chunk/client/aws/config_test.go @@ -0,0 +1,71 @@ +package aws + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCredentialsFromURL(t *testing.T) { + tests := []struct { + name string + urlStr string + expectedKey string + expectedSecret string + expectedSession string + expectError bool + }{ + { + name: "URL with username and password", + urlStr: "s3://mykey:mysecret@us-east-1", + expectedKey: "mykey", + expectedSecret: "mysecret", + expectedSession: "", + expectError: false, + }, + { + name: "URL with only username", + urlStr: "s3://mykey@us-east-1", + expectedKey: "mykey", + expectedSecret: "", + expectedSession: "", + expectError: false, + }, + { + name: "URL without credentials (should not error)", + urlStr: "s3://us-east-1", + expectedKey: "", + expectedSecret: "", + expectedSession: "", + expectError: false, + }, + { + name: "URL with endpoint without credentials", + urlStr: "s3://s3.amazonaws.com", + expectedKey: "", + expectedSecret: "", + expectedSession: "", + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := url.Parse(tt.urlStr) + require.NoError(t, err) + + key, secret, session, err := CredentialsFromURL(u) + + if tt.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.expectedKey, key) + require.Equal(t, tt.expectedSecret, secret) + require.Equal(t, tt.expectedSession, session) + } + }) + } +} + diff --git a/pkg/storage/chunk/client/aws/s3_storage_client.go b/pkg/storage/chunk/client/aws/s3_storage_client.go index 317b5d91aed3e..dacc1fda938b3 100644 --- a/pkg/storage/chunk/client/aws/s3_storage_client.go +++ b/pkg/storage/chunk/client/aws/s3_storage_client.go @@ -219,7 +219,11 @@ func buildS3Client(cfg S3Config, hedgingCfg hedging.Config, hedging bool) (*s3.C if err != nil { return nil, err } - s3Options.Credentials = credentials.NewStaticCredentialsProvider(key, secret, session) + // Only set credentials if they were provided in the URL + // Otherwise, let AWS SDK use the default credential chain + if key != "" || secret != "" { + s3Options.Credentials = credentials.NewStaticCredentialsProvider(key, secret, session) + } } else { s3Options.Region = "dummy" } From a51dacfbc43f94b7e71d93098d0d8d9f0fa70ab2 Mon Sep 17 00:00:00 2001 From: DylanGuedes Date: Mon, 20 Oct 2025 09:59:07 -0300 Subject: [PATCH 2/2] remove error from return --- pkg/storage/chunk/client/aws/config.go | 6 +++--- pkg/storage/chunk/client/aws/config_test.go | 20 ++++--------------- .../chunk/client/aws/s3_storage_client.go | 6 ++---- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/pkg/storage/chunk/client/aws/config.go b/pkg/storage/chunk/client/aws/config.go index f1d4469d604ca..918beecd4490b 100644 --- a/pkg/storage/chunk/client/aws/config.go +++ b/pkg/storage/chunk/client/aws/config.go @@ -72,17 +72,17 @@ func DynamoConfigFromURL(awsURL *url.URL) (*dynamodb.Options, error) { return &config, nil } -func CredentialsFromURL(awsURL *url.URL) (key, secret, session string, err error) { +func CredentialsFromURL(awsURL *url.URL) (key, secret, session string) { if awsURL.User != nil { username := awsURL.User.Username() password, _ := awsURL.User.Password() // We request at least the username or password being set to enable the static credentials. if username != "" || password != "" { - return username, password, "", nil + return username, password, "" } } // Return empty credentials instead of error to allow AWS SDK to use default credential chain // (environment variables, IAM roles, etc.) - return "", "", "", nil + return "", "", "" } diff --git a/pkg/storage/chunk/client/aws/config_test.go b/pkg/storage/chunk/client/aws/config_test.go index 610453e7b32d1..ce016592cc1f9 100644 --- a/pkg/storage/chunk/client/aws/config_test.go +++ b/pkg/storage/chunk/client/aws/config_test.go @@ -14,7 +14,6 @@ func TestCredentialsFromURL(t *testing.T) { expectedKey string expectedSecret string expectedSession string - expectError bool }{ { name: "URL with username and password", @@ -22,7 +21,6 @@ func TestCredentialsFromURL(t *testing.T) { expectedKey: "mykey", expectedSecret: "mysecret", expectedSession: "", - expectError: false, }, { name: "URL with only username", @@ -30,7 +28,6 @@ func TestCredentialsFromURL(t *testing.T) { expectedKey: "mykey", expectedSecret: "", expectedSession: "", - expectError: false, }, { name: "URL without credentials (should not error)", @@ -38,7 +35,6 @@ func TestCredentialsFromURL(t *testing.T) { expectedKey: "", expectedSecret: "", expectedSession: "", - expectError: false, }, { name: "URL with endpoint without credentials", @@ -46,7 +42,6 @@ func TestCredentialsFromURL(t *testing.T) { expectedKey: "", expectedSecret: "", expectedSession: "", - expectError: false, }, } @@ -55,17 +50,10 @@ func TestCredentialsFromURL(t *testing.T) { u, err := url.Parse(tt.urlStr) require.NoError(t, err) - key, secret, session, err := CredentialsFromURL(u) - - if tt.expectError { - require.Error(t, err) - } else { - require.NoError(t, err) - require.Equal(t, tt.expectedKey, key) - require.Equal(t, tt.expectedSecret, secret) - require.Equal(t, tt.expectedSession, session) - } + key, secret, session := CredentialsFromURL(u) + require.Equal(t, tt.expectedKey, key) + require.Equal(t, tt.expectedSecret, secret) + require.Equal(t, tt.expectedSession, session) }) } } - diff --git a/pkg/storage/chunk/client/aws/s3_storage_client.go b/pkg/storage/chunk/client/aws/s3_storage_client.go index dacc1fda938b3..45efcebe80939 100644 --- a/pkg/storage/chunk/client/aws/s3_storage_client.go +++ b/pkg/storage/chunk/client/aws/s3_storage_client.go @@ -215,10 +215,8 @@ func buildS3Client(cfg S3Config, hedgingCfg hedging.Config, hedging bool) (*s3.C // if an s3 url is passed use it to initialize the s3Config and then override with any additional params if cfg.S3.URL != nil { - key, secret, session, err := CredentialsFromURL(cfg.S3.URL) - if err != nil { - return nil, err - } + key, secret, session := CredentialsFromURL(cfg.S3.URL) + // Only set credentials if they were provided in the URL // Otherwise, let AWS SDK use the default credential chain if key != "" || secret != "" {