add support for vhost/dns style bucket access #154

I have some doubt on the correctness of key extraction on some edge-case : if a path starts with //some-key, it gets translated to some-key. Maybe it should be /some-key instead? (stripping only the first /)

I have some doubt on the correctness of key extraction on some edge-case : if a path starts with `//some-key`, it gets translated to `some-key`. Maybe it should be `/some-key` instead? (stripping only the first /)

quentin commented

2021-11-12 14:30:23 +00:00

I have some doubt on the correctness of key extraction on some edge-case : if a path starts with //some-key, it gets translated to some-key. Maybe it should be /some-key instead? (stripping only the first /)

Indeed, I also think that we should only strip one slash. Ideally, we should do some tests with Amazon implementation / look how minio handles this case :)

> I have some doubt on the correctness of key extraction on some edge-case : if a path starts with `//some-key`, it gets translated to `some-key`. Maybe it should be `/some-key` instead? (stripping only the first /) Indeed, I also think that we should only strip one slash. Ideally, we should do some tests with Amazon implementation / look how minio handles this case :)

trinity-1686a commented

2021-11-12 22:14:37 +00:00

tested with minio. It seems to have the same behavior

trinity@anarthink ~> mcli cp /proc/cpuinfo minio/minio-test//////cpuinfo.txt
 0 B / ? ┃░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░▓┃
trinity@anarthink ~> mcli ls minio/minio-test
[2021-11-12 22:20:17 CET]     0B cpuinfo.txt
trinity@anarthink ~>

pcap capture shows PUT being diriged to /minio-test//////cpuinfo.txt, and answer to ListObject is <Key>cpuinfo.txt</Key>, so it appears all / should be stripped.

In general, multiple / may be subject to path normalisation (nginx, by default, compress multiples / in a single one), so the caller should probably not rely on / being kept

tested with minio. It seems to have the same behavior ``` trinity@anarthink ~> mcli cp /proc/cpuinfo minio/minio-test//////cpuinfo.txt 0 B / ? ┃░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░▓┃ trinity@anarthink ~> mcli ls minio/minio-test [2021-11-12 22:20:17 CET] 0B cpuinfo.txt trinity@anarthink ~> ``` pcap capture shows PUT being diriged to `/minio-test//////cpuinfo.txt`, and answer to ListObject is `<Key>cpuinfo.txt</Key>`, so it appears all `/` should be stripped. In general, multiple `/` may be subject to path normalisation (nginx, by default, compress multiples `/` in a single one), so the caller should probably not rely on `/` being kept

👍 1

quentin reviewed 2021-11-15 13:51:44 +00:00

doc/book/src/reference_manual/configuration.md Outdated

					
				@ -30,6 +30,7 @@ sled_flush_every_ms = 2000

				[s3_api]

				api_bind_addr = "[::]:3900"

				s3_region = "garage"

				root_domain = ".3.garage"

quentin commented

2021-11-15 13:51:44 +00:00

".s3.garage" ?

`".s3.garage"` ?

trinity-1686a added 1 commit 2021-11-15 16:39:41 +00:00

update doc and comments

continuous-integration/drone/pr Build is passing

Details

df4b739617

trinity-1686a commented

2021-11-15 16:43:20 +00:00

changes were tested using mcli (read and list object, write is blocked by #64) and rclone (read, write and list). Other endpoints were not tested, but should hopefully work

lx approved these changes 2021-11-16 14:41:33 +00:00

lx left a comment

Thanks for the PR :)

lx merged commit 53888995bd into main

2021-11-16 14:41:41 +00:00

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

3 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#154