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

First pass at enabling s3 path style config #317

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dcarbone
Copy link

Started working on this as I self-host Minio, and after updating mysql-backup I see you all switched to the aws-sdk-go-v2 lib which defaults to virtual-host style routing.

Didn't end up needing it as I instead opted to just set up wildcard dns resolution and certificates for my Minio instance, so the current impl works fine.

Opening the PR in case it may help somebody else and they want to finish it off (if it even needs further work).

@deitch
Copy link
Collaborator

deitch commented Jun 28, 2024

This looks pretty good. Any reason to keep it draft?

@dcarbone
Copy link
Author

Only that I didn't have time to sufficiently test it. For me, improving my homelab to be able to handle virtual-host routing was ultimately preferable.

I did deploy it a few times and it seemed to work, but I'm not convinced just yet.

@deitch
Copy link
Collaborator

deitch commented Jun 28, 2024

Once you move it to ready, I will kick off CI on it.

Is there any way to add it to the integration tests? Those start a fakes3 server. Is there any way to get the tests to work? Or does it depend on DNS? I guess in theory we could add a DNS server that responds there, and have the http client use that DNS server?

@dcarbone
Copy link
Author

Hmm, so I haven't had a look at the available test suite here, but effectively this enables the legacy path-based routing for s3 buckets.

For example:

New way (virtual host routing): https://my-great-bucket.s3.amazonaws.com
Old way (path-style routing): https://s3.amazonaws.com/my-great-bucket

This PR enables the user to configure the underlying s3 client to use the path-based bucket routing. To answer your question of whether this would require a DNS server, that depends entirely upon what is faking S3.

In the past, I've used Localstack to test path vs. virtual-host bucket routing config, but again I haven't had a chance to look at what's used here, so I cannot say whether the lack of DNS is blocking.

I may have a chance to look at the available test suite this weekend sometime.

@deitch
Copy link
Collaborator

deitch commented Jun 30, 2024

What is the use case for when it is necessary? You can configure the s3 region and endpoint independently. When would we need it?

@dcarbone
Copy link
Author

What is the use case for when it is necessary? You can configure the s3 region and endpoint independently. When would we need it?

When would I need what? Path style bucket routing? As mentioned it can be useful if you use a tool with an s3 compliant API that either does not support or you not want to bother with the infra required for virtual host routing.

@deitch
Copy link
Collaborator

deitch commented Jun 30, 2024

Yeah, I am trying to understand if it is not already covered? I guess if the target's type=s3, then the url needs to be of a certain style, and that style can change.

OK, I get it. I will wait to hear what you say about being able to put it into the testing.

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.

2 participants