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 support for AWS STS endpoint in the Bucket API #1552

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

matheuscscp
Copy link
Collaborator

@matheuscscp matheuscscp commented Jul 19, 2024

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
Copy link
Collaborator Author

Back to draft, adding tests for proxy

@matheuscscp matheuscscp marked this pull request as draft July 20, 2024 16:46
@matheuscscp matheuscscp marked this pull request as ready for review July 20, 2024 17:21
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
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
@stefanprodan stefanprodan added area/bucket Bucket related issues and pull requests area/api API related issues and pull requests labels Aug 2, 2024
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 🏅

pkg/minio/minio.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the bucket-sts-endpoint branch 3 times, most recently from cd95833 to 4e5e838 Compare August 6, 2024 17:25
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
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
api/v1/condition_types.go Outdated Show resolved Hide resolved
Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
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.

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
Collaborator Author

matheuscscp commented Aug 8, 2024

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
Labels
area/api API related issues and pull requests area/bucket Bucket related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for custom AWS STS endpoint for bucket/S3 source
3 participants