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

Add support for AWS STS endpoint in the Bucket API #1552

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Add support for AWS STS endpoint in the Bucket API #1552

merged 1 commit into from
Aug 8, 2024

Conversation

matheuscscp
Copy link
Member

@matheuscscp matheuscscp commented Jul 19, 2024 •
edited
Loading

Fixes #1423

I was able to test this in my personal AWS account using both an interface endpoint and the public regional endpoints.

@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch 4 times, most recently from 2f84ddd to db23a9d Compare July 20, 2024 15:58
@matheuscscp matheuscscp marked this pull request as ready for review July 20, 2024 15:59
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from db23a9d to 6b0d5b4 Compare July 20, 2024 16:36
@matheuscscp
Copy link
Member Author

Back to draft, adding tests for proxy

@matheuscscp matheuscscp marked this pull request as draft July 20, 2024 16:46
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from 6b0d5b4 to 2d62c48 Compare July 20, 2024 17:20
@matheuscscp matheuscscp marked this pull request as ready for review July 20, 2024 17:21
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from 2d62c48 to d0b6994 Compare July 20, 2024 17:33
darkowlzz
darkowlzz reviewed Jul 23, 2024
View reviewed changes
pkg/minio/minio.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp changed the title Add support for STS endpoint in the Bucket API Add support for AWS STS endpoint in the Bucket API Aug 1, 2024
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from d0b6994 to 6ccf49f Compare August 1, 2024 12:49
matheuscscp
matheuscscp commented Aug 1, 2024
View reviewed changes
api/v1beta2/bucket_types.go Show resolved Hide resolved
api/v1beta2/bucket_types.go Show resolved Hide resolved
api/v1beta2/sts_types.go Outdated Show resolved Hide resolved
docs/spec/v1beta2/buckets.md Show resolved Hide resolved
@matheuscscp matheuscscp requested a review from darkowlzz August 1, 2024 12:57
@stefanprodan stefanprodan added area/bucket Bucket related issues and pull requests area/api API related issues and pull requests labels Aug 2, 2024
stefanprodan
stefanprodan approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @matheuscscp 🏅

@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from 6ccf49f to f692579 Compare August 3, 2024 08:54
matheuscscp
matheuscscp commented Aug 6, 2024
View reviewed changes
pkg/minio/minio.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from f692579 to a85bae1 Compare August 6, 2024 14:49
@matheuscscp matheuscscp mentioned this pull request Aug 6, 2024
Open
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch 3 times, most recently from cd95833 to 4e5e838 Compare August 6, 2024 17:25
@matheuscscp matheuscscp requested a review from darkowlzz August 6, 2024 19:23
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from 4e5e838 to 099226d Compare August 6, 2024 19:51
darkowlzz
darkowlzz reviewed Aug 7, 2024
View reviewed changes
pkg/minio/minio.go Outdated Show resolved Hide resolved
pkg/minio/minio.go Outdated Show resolved Hide resolved
internal/controller/bucket_controller.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch 2 times, most recently from e58988d to 984f434 Compare August 7, 2024 16:55
darkowlzz
darkowlzz reviewed Aug 7, 2024
View reviewed changes
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Other than the last few suggestions about tests, the implementation looks good to me.
The STS mock tests particularly look great.

internal/controller/bucket_controller_test.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch 3 times, most recently from 23173e2 to b036b69 Compare August 8, 2024 00:36
@matheuscscp matheuscscp requested a review from darkowlzz August 8, 2024 01:04
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from b036b69 to a01e1a6 Compare August 8, 2024 11:34
darkowlzz
darkowlzz reviewed Aug 8, 2024
View reviewed changes
api/v1/condition_types.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch from a01e1a6 to 7536ab4 Compare August 8, 2024 15:35
darkowlzz
darkowlzz approved these changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@darkowlzz darkowlzz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I did some manual testing on EKS with IRSA.

  • Without IRSA annotation, the bucket just failed due to no S3 permission.
  • With IRSA annotation and no explicit STS config, just .spec.provider: aws, it worked.
  • With explicit STS endpoint https://sts.amazonaws.com, it worked.
  • Tried using a regional endpoint https://sts.us-east-2.amazonaws.com, it worked again.
  • Tried a non-existing endpoint https://sts.us-east-5.amazonaws.com, it resulted in error Post \"https://sts.us-east-5.amazonaws.com\": dial tcp: lookup sts.us-east-5.amazonaws.com on 172.20.0.10:53: no such host.
  • Tried a regional endpoint that's inactive in the account, resulted in error No OpenIDConnect provider found in your account for <OIDC provider address>.
  • Tried http://sts.amazonaws.com and it failed with connection refused error.

Everything looks good and as expected.
Thanks for all the work on this and being patience with me 🙂

@matheuscscp
Copy link
Member Author

matheuscscp commented Aug 8, 2024 •
edited
Loading

I did some manual testing on EKS with IRSA.

  • Without IRSA annotation, the bucket just failed due to no S3 permission.
  • With IRSA annotation and no explicit STS config, just .spec.provider: aws, it worked.
  • With explicit STS endpoint https://sts.amazonaws.com, it worked.
  • Tried using a regional endpoint https://sts.us-east-2.amazonaws.com, it worked again.
  • Tried a non-existing endpoint https://sts.us-east-5.amazonaws.com, it resulted in error Post \"https://sts.us-east-5.amazonaws.com\": dial tcp: lookup sts.us-east-5.amazonaws.com on 172.20.0.10:53: no such host.
  • Tried a regional endpoint that's inactive in the account, resulted in error No OpenIDConnect provider found in your account for <OIDC provider address>.
  • Tried http://sts.amazonaws.com and it failed with connection refused error.

Everything looks good and as expected. Thanks for all the work on this and being patience with me 🙂

Thanks for your excellent diligence, it keeps Flux being the best GitOps product out there!

@matheuscscp matheuscscp merged commit c41c2d6 into fluxcd:main Aug 8, 2024
9 checks passed
@matheuscscp matheuscscp deleted the bucket-sts-endpoint branch August 8, 2024 21:21
@stefanprodan stefanprodan added this to the Bucket GA milestone Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanprodan stefanprodan stefanprodan approved these changes

@darkowlzz darkowlzz darkowlzz approved these changes

Assignees
No one assigned
Labels
area/api API related issues and pull requests area/bucket Bucket related issues and pull requests
Projects
None yet
Milestone
Bucket GA
Development

Successfully merging this pull request may close these issues.

Add support for custom AWS STS endpoint for bucket/S3 source
3 participants
@matheuscscp @darkowlzz @stefanprodan