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.

Sign up for GitHub

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

Jump to bottom

Allow S3 Storage Provider to support sts_endpoint #6990

Merged
merged 19 commits into from
Jan 9, 2024
Merged

Allow S3 Storage Provider to support sts_endpoint #6990

merged 19 commits into from
Jan 9, 2024

Conversation

TimKotowski
Copy link
Contributor

@TimKotowski TimKotowski commented Dec 23, 2023 •
edited

What this PR does

Minio has added sts support to work with the k8s service account JWTs. Allow STS Endpoint support for S3 storage provider by providing it in Mimirs s3Config.

Which issue(s) this PR fixes or relates to

#6172

Checklist

  • Tests updated.
  • NA no Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • NA no about-versioning.md updated with experimental features.

@CLAassistant
Copy link

CLAassistant commented Dec 23, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@TimKotowski TimKotowski marked this pull request as ready for review December 23, 2023 16:24
@TimKotowski TimKotowski requested a review from a team as a code owner December 23, 2023 16:24
@TimKotowski TimKotowski mentioned this pull request Dec 23, 2023
Closed
@TimKotowski TimKotowski marked this pull request as draft December 23, 2023 16:38
@TimKotowski TimKotowski marked this pull request as ready for review December 24, 2023 16:57
@TimKotowski TimKotowski marked this pull request as draft December 25, 2023 17:14
dimitarvdimitrov
dimitarvdimitrov reviewed Dec 26, 2023
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the contribution.

I left a few minor comments/questions. I also see some of the linting and unit tests are failing. The messages within CI should have instructions on how to resolve them. Let me know if you need any help.

pkg/storage/bucket/client_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
pkg/storage/bucket/s3/config.go Outdated Show resolved Hide resolved
pkg/storage/bucket/s3/config.go Outdated Show resolved Hide resolved
@TimKotowski TimKotowski marked this pull request as ready for review December 26, 2023 18:24
@TimKotowski TimKotowski requested a review from a team as a code owner December 26, 2023 18:24
dimitarvdimitrov
dimitarvdimitrov reviewed Jan 2, 2024
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

The linter is complaining about import statement ordering, and I left a couple of minor comments. Other than that I think this is ready for a merge.

pkg/storage/bucket/s3/config.go Outdated Show resolved Hide resolved
pkg/util/url.go Outdated Show resolved Hide resolved
pkg/util/url_test.go Outdated Show resolved Hide resolved
dimitarvdimitrov
dimitarvdimitrov approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, appreciate the time you spent!

@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) January 8, 2024 10:47
@dimitarvdimitrov
Fix linter errors
ef17344 
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov
Copy link
Contributor

I pushed a commit to address the linter errors. PR should be good to merge now

@TimKotowski
Copy link
Contributor Author

@dimitarvdimitrov ty for fixing the lint, was super busy yesterday. Hopefully when I am off my windows machine, when my new mac comes I can address these linting issues with the specified make commands. Appreciate you fixing

@TimKotowski
Copy link
Contributor Author

Safe to merge then once all pipelines pass?

@dimitarvdimitrov dimitarvdimitrov merged commit b533458 into grafana:main Jan 9, 2024
28 checks passed
@narqo narqo mentioned this pull request Jan 19, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimitarvdimitrov dimitarvdimitrov dimitarvdimitrov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TimKotowski @CLAassistant @dimitarvdimitrov