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 custom endpoints in S3 snapstore #431

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

sibucan
Copy link
Contributor

@sibucan sibucan commented Feb 8, 2022

What this PR does / why we need it:
Adds the capability to specify a different S3 endpoint within the S3 snapstore logic. This allows usage of etcd-backup-restore with non-AWS S3-compliant providers, such as min.io or Linode Object Storage. It also adds a way of setting S3ForcePathStyle, which is needed for etcd-backup-restore to be able to save the snapshot in these S3-compatible providers.

Note that by default when using the AWS SDK client, providing an empty string in the Endpoint field of the aws.Config{} struct is functionally similar to what was being done before (which was keeping it nil), and will generate the default endpoint, see: https://pkg.go.dev/github.com/aws/aws-sdk-go@v1.22.0/aws?utm_source=gopls#Config

Last but not least, I understand there's at least one PR that may modify the logic in this one. I am willing to rebase this PR if necessary if that one gets merged first.

Which issue(s) this PR fixes:
Fixes #416

Special notes for your reviewer:

Release note:

* Added support for non-AWS S3-compatible providers by specifying a custom endpoint.

@sibucan sibucan requested a review from a team as a code owner February 8, 2022 18:02
@gardener-robot
Copy link

@sibucan Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Feb 8, 2022
@gardener-robot-ci-2
Copy link
Contributor

Thank you @sibucan for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

Copy link
Member

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. One comment from my side, the rest looks good to me.

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Feb 21, 2022
@timuthy timuthy added this to the v0.15.0 milestone Feb 21, 2022
@timuthy timuthy added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 21, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 21, 2022
@timuthy
Copy link
Member

timuthy commented Feb 21, 2022

@ishan16696 are you fine with rebasing #428, so that we can merge this one first?

@ishan16696
Copy link
Member

I think we should go with #435 first then #436 followed by this PR as this PR also required to update the doc as well as Plamen and I don’t have to retest #435 PR with 5 different storage providers.

@timuthy
Copy link
Member

timuthy commented Feb 22, 2022

@sibucan kindly asking you to rebase this PR 🙂

@ishan16696
Copy link
Member

@sibucan can you also update the document for storage providers accordingly.
Thanks !!

Also adds support for setting `S3ForcePathStyle`, which is required for
some S3-compatible providers as well.
@sibucan
Copy link
Contributor Author

sibucan commented Feb 22, 2022

@timuthy I've rebased and updated the document as requested -- one thing to note is that I have not changed my code so that the AWS endpoint can be configured via a file, since this functionality is freshly-merged and I wasn't sure if my changes should also follow suit (meaning they should also be configurable via a file). I guess it can also be added in a subsequent PR if deemed necessary.

@timuthy timuthy added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 23, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 23, 2022
@ishan16696 ishan16696 requested a review from timuthy February 24, 2022 12:11
Copy link
Member

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Feb 24, 2022
@timuthy timuthy merged commit d5462fb into gardener:master Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure custom S3 endpoint
6 participants