Fix AWS auth credentials chain and upgrade minio-go #3006
Conversation
|
Looks like this is replacing #2958 -- which is fine with me. The non-mocked test is doable if you have an AWS account that you can setup with GitHub and setup an Identity Provider in IAM. If not, no biggie. +1 to all this, please merge as soon as possible. |
|
See comment for test images: #2958 (comment) |
|
I can confirm the docker image works as expected. AWS IRSA authentication is good. |
mapno
requested review from
jdbaldry,
knylander-grafana,
joe-elliott,
annanay25,
mdisibio,
yvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
|
Sorry for the delay but I can confirm that |
CHANGELOG.md
Outdated
| @@ -78,6 +78,8 @@ defaults: | |||
| * [BUGFIX] Moved empty root span substitution from `querier` to `query-frontend`. [#2671](https://github.com/grafana/tempo/issues/2671) (@galalen) | |||
| * [BUGFIX] Correctly propagate ingester errors on the query path [#2935](https://github.com/grafana/tempo/issues/2935) (@joe-elliott) | |||
| * [BUGFIX] Fix cases where empty filter {} wouldn't return expected results [#2498](https://github.com/grafana/tempo/issues/2498) (@mdisibio) | |||
| * [BUGFIX] Reorder S3 credential chain and upgrade minio-go [#3006](https://github.com/grafana/tempo/pull/3006) (@ekristen, @mapno) | |||
| Removes config option `native_aws_auth_enabled`. If you were using this option, remove it from your config. | |||
There was a problem hiding this comment.
is this a breaking change? if yes, can we please mark it as breaking change here?
There was a problem hiding this comment.
Removing native_aws_auth_enabled if left in the config will cause a break due to how the config is parsed. I would suggest that perhaps we leave the config option in and write a log message if it's set that it's deprecated and remove it in the next major release.
There was a problem hiding this comment.
Leaving it in place and saying it's deprecated and is not used wouldn't hurt anything to prevent this from being a breaking change.
There was a problem hiding this comment.
Went with that approach, is nicer for users. Thanks!
There was a problem hiding this comment.
Going to approve to unblock @mapno. I do agree with the comments that we need to mark a breaking change and it would be nice to parse the field and log a warning, but other than that this LGTM.
Thank you to @ekristen @coufalja @sberz and everyone else who helped us resolve this! Who knew that aws auth was so complicated :)
| wrapCredentialsProvider(&credentials.Static{ | ||
| Value: credentials.Value{ | ||
| AccessKeyID: cfg.AccessKey, | ||
| SecretAccessKey: cfg.SecretKey.String(), |
There was a problem hiding this comment.
@joe-elliott I was able to provide the session token via code. now this isn't longer possible.
What this PR does:
Fixes AWS authentication for IRSA.
PR #2889 was suppose to fix this but it didn't. Simply switching the chain flow up a bit resolves this issue without the need of if statements makes this all work again, which is what this PR does.
Removes config option
native_aws_auth_enabled, as it's no longer necessary. If you were using this option, remove it from your config.There are images publised for amd64 and arm64 architectures.
Which issue(s) this PR fixes:
Fixes #2888
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]