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

Allow to pass region in the custom s3 storage url #1854

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

darkdarkdragon
Copy link
Contributor

@darkdarkdragon darkdarkdragon commented Apr 28, 2021

What does this pull request do? Explain your changes. (required)
All the previous S3-compatible storages we've tested was ignoring 'region' part of configuration.
So in our code it was hardcoded as value 'ignored'.
But filebase.com does want it to be set to specific value.
This updates allows to pass region as the part of the custom URL, like this:

s3+http://user:password@example.com:9000/path/to/region-name/bucket-name

Specific updates (required)
Change parsing of custom s3 url

How did you test each of these updates (required)
unit test

Does this pull request close any open issues?

Checklist:

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Is the part of the path before the bucket name always the region name for S3 compatible APIs or is this a filebase.com specific convention? And related to that: is it possible for a path to have multiple parts before the bucket name such that none of the parts should be treated as a region name i.e. not-a-region/also-not-a-region/bucket-name?

Also, please make sure to update the pending changelog according to the contribution guide.

Copy link
Contributor

@iameli iameli left a comment

Choose a reason for hiding this comment

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

Hmmmm yeah not sure I understand what this does. If I'm running a Minio server at https://iame.li/storage/minio, will s3+https://user:password@iame.li/storage/minio still work? What's the difference between a region and a path in this context?


func TestCustomS3URLWithRegion(t *testing.T) {
assert := assert.New(t)
os, err := ParseOSURL("s3+http://user:password@example.com:9000/path/to/region-name/bucket-name", true)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to /path/to? There's no assert.Equal that has it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is ignored. It was always ignored (hosturl.Path = "")

@darkdarkdragon
Copy link
Contributor Author

Hmmmm yeah not sure I understand what this does. If I'm running a Minio server at https://iame.li/storage/minio, will s3+https://user:password@iame.li/storage/minio still work? What's the difference between a region and a path in this context?

Was it working before? We're just throwing out /storage part and not using it anywhere.

@iameli
Copy link
Contributor

iameli commented Apr 28, 2021

Hmm, it was my intent to have that work when I wrote the spec and implementation. If that's not happening, I think that's a bug. Sometimes stuff is in subdirectories.

@darkdarkdragon
Copy link
Contributor Author

Hmmmm yeah not sure I understand what this does. If I'm running a Minio server at https://iame.li/storage/minio, will s3+https://user:password@iame.li/storage/minio still work? What's the difference between a region and a path in this context?

Actually I've looked into minio's docs - it can't be ran on subpath (minio/minio#5947 (comment))

@iameli
Copy link
Contributor

iameli commented Apr 28, 2021

Oh, hah. Okay. I'm cool with this, then.

@darkdarkdragon
Copy link
Contributor Author

Is the part of the path before the bucket name always the region name for S3 compatible APIs or is this a filebase.com specific convention? And related to that: is it possible for a path to have multiple parts before the bucket name such that none of the parts should be treated as a region name i.e. not-a-region/also-not-a-region/bucket-name?

There is no convention for such URLs.
Minio doesn't allow to run it under non-root path, and I'm not aware of any S3-compatible service that does it.
Also, right now we're just ignoring anything in the path name except last part which we treat as bucket name.

@darkdarkdragon
Copy link
Contributor Author

@yondonfu added changelog

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Just one comment on a typo in the changelog, but LGTM otherwise so feel free to fix and then merge.

@@ -7,6 +7,7 @@
#### General

- \#1848 Use fee cut instead of fee share for user facing language in the CLI (@kyriediculous)
- \#1854 Allow to pass region in the custom s3 storage URL (@kdarkdarkdragon)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there is an extra k in the username

s3+http://user:password@example.com:9000/path/to/region-name/bucket-name
@darkdarkdragon darkdarkdragon merged commit 40760e2 into master Apr 28, 2021
@darkdarkdragon darkdarkdragon deleted the it/custom-s3-region branch April 28, 2021 21:54
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.

3 participants