Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add capability to use custom AWS STS Endpoint #4736

Merged
merged 9 commits into from Oct 19, 2021
Merged

Conversation

MioOgbeni
Copy link
Contributor

@MioOgbeni MioOgbeni commented Oct 5, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Added capability to use custom AWS STS Endpoint. It is helpfull when you cannot communicate out of AWS network to default endpoint https://sts.amazonaws.com/

If parameter sts_endpoint will not be set, minio used default https://sts.amazonaws.com/ or another regional mutation of this endpoint. Look here

Verification

I cannot run test along our testing buckets, then I test only make test-local and make test-e2e-local. I will be happy when someone else try it on real bucket.

Review

This is my first public PR ever, then can I ask you (as maintainer of AWS/S3 client) to review this PR @bwplotka?

Signed-off-by: Tomáš Novák <tom.nov96@gmail.com>
Signed-off-by: Tomáš Novák <tom.nov96@gmail.com>
Signed-off-by: Tomáš Novák <tom.nov96@gmail.com>
@MioOgbeni MioOgbeni marked this pull request as ready for review October 5, 2021 08:57
bwplotka
bwplotka previously approved these changes Oct 18, 2021
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, great job. To the point and clean PR, thanks for contributing 👍🏽

CHANGELOG.md Outdated Show resolved Hide resolved

If you want to use IAM credential retrieved from an instance profile, Thanos needs to authenticate through AWS STS. For this purposes you can specify your own STS Endpoint.

By default Thanos will use endpoint: https://sts.amazonaws.com and AWS region coresponding endpoints.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

@@ -228,6 +229,7 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
Client: &http.Client{
Transport: http.DefaultTransport,
},
Endpoint: config.STSEndpoint,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was... easy! 🙈

@bwplotka
Copy link
Member

Looks like we might have unformatted code? Feel free to run make lint locally and commit changes (:

@bwplotka
Copy link
Member

bwplotka commented Oct 18, 2021

And ping us on #thanos-dev, sad that first contributor CI pipelines has to enabled manually ):

Signed-off-by: Tomáš Novák <tom.nov96@gmail.com>
@MioOgbeni
Copy link
Contributor Author

Yeah, there was problem with formatting. I forgotten run make format, now its seem ok, but make lint is still failing on my local, because of "constant overflow" on compact/downsample.
Here:

a.max = -math.MaxFloat64

But I do not interfered to this code.

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Tomáš Novák <tom.nov96@gmail.com>
@MioOgbeni
Copy link
Contributor Author

Commited suggestion. Now should CI pass and PR will be ready to final review and merge.

Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@bwplotka bwplotka merged commit 41d2e96 into thanos-io:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants