Enable AWS SDK for go authentication to fix IMDSv1 issue

[friedrichg]

What this PR does:
Introduces a boolean flag to enable AWS SDK for go auth
Enables AWS SDK for go auth

Which issue(s) this PR fixes:
Enable IMDSv1 again #4896 and most likely other use cases

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[alvinlin123]

Looks good to me.

[alanprot]

default should be true?

If this is false we are changing the behavior as far as i understood?

[alvinlin123]

I think this is default is false in thanos

[alvinlin123]

actually, after looking at Thanos' code a bit I am wondering if we should just "hardcode" AWSSDKAuth: true and not to have yet-another-configuration. Let me take a closer look to see if there is any cred resolution strategy that is not covered by AWSSDKAuth before its introduction.

[alvinlin123]

After reading thanos-io/thanos#4667 and the code I think it's safe to just use AWSSDKAuth: true and don't expose yet-another-config. I am not sure what would be the benefit of using the the cred providers from minio over aws go sdk v2; I think minio didn't use aws sdk to minimize dependencies?

Also, we will need to have some logic to set AWSSDKAuth: false if AccessKeyID is defined; they can't co-exist.

[friedrichg]

I agree these options are hard to decide by users and we should try to avoid them. I am happy you jump on this quick :)

But here is the thing: s3 provider in objstore supports AWSSDKAuth: false and AccessKey: "".

https://github.com/thanos-io/objstore/blob/cec51c61948b0c04b60c035fd372cc83a540881b/providers/s3/s3.go#L231-L242

this is the code path my configuration was using in AWS and it was just by chance working due to their bug.

This makes no sense to AWS, but it makes sense to minio. Because minio has support for access without AccessKey that sort of works like AWS but doesn't use AWS libraries. See https://min.io/docs/minio/linux/developers/security-token-service.html

So I think the other possible solution to this is to rename our current s3 provider to minio provider and have a native s3 provider that sets AWSSDKAuth: true or even better don't use any minio at all. In that sense, Thanos should also do the same, I think.

I like this option better because it simplifies the use case for AWS and still enables minio.

Let me know what you think @alvinlin123 @alanprot @yeya24

[alvinlin123]

I think the resolver chain from Thanos at https://github.com/thanos-io/objstore/blob/cec51c61948b0c04b60c035fd372cc83a540881b/providers/s3/s3.go#L231-L242 is a subset of what the AWS SDK does.

In other words, the following cred resolving order implemented in Minio is covered by the AWS SDK; AWS SDK supports assume role with web identity, env var, loading shared profile etc.

chain = []credentials.Provider{
			wrapCredentialsProvider(&credentials.EnvAWS{}),
			wrapCredentialsProvider(&credentials.FileAWSCredentials{}),
			wrapCredentialsProvider(&credentials.IAM{
				Client: &http.Client{
					Transport: http.DefaultTransport,
				},
				Endpoint: config.STSEndpoint,
			}),
		}

So what works previously, AWSSDKAuth: false AccessKeyID:"", should be equivalent to AWSSDKAuth: true AccessKeyID:"" with the current version of Thanos and Minio.

Would you be able give AWSSDKAuth: true AccessKeyID:"" a try and see if it work on some of your test cluster?

[friedrichg]

I see. AWS SDK might also do the right thing with minio in all the use cases, even without AccessKey.
Then we can skip this code path completely.

My use case is solved with AWSDKAuth: true, yes. Let me implement the logic you mentioned before about nonempty AccessKey.

I have second thought on exposing new config

[friedrichg]

Tested this minio using access_key_id and secret_access_key and see no problem.

Click to expand!

brew install minio
minio server /tmp/minio --address 127.0.0.1:9000

mini.yml

auth_enabled: false
blocks_storage:
  s3:
    access_key_id: minioadmin
    bucket_name: mybucket
    endpoint: 127.0.0.1:9000
    insecure: true
    secret_access_key: minioadmin
ingester:
  lifecycler:
    address: 127.0.0.1
    min_ready_duration: 0s
    ring:
      kvstore:
        store: inmemory
      replication_factor: 1
target: compactor,ingester,distributor

./cortex --config.file=mini.yaml

We also have integration tests based on minio.
If there is anything else that should be tested with minio, there should be a test for it.

Ready for review