add support for vhost/dns style bucket access #154

trinity-1686a · 2021-11-11T14:16:17Z

trinity-1686a commented

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

fix #137

I moved some code arround so it's not duplicated between s3 and web.

fix #137 I moved some code arround so it's not duplicated between s3 and web.

👍 1

trinity-1686a force-pushed vhost-style from f0681dea33 to 2e70257d29

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

Compare

trinity-1686a added 1 commit 2021-11-11 14:37:53 +00:00

continuous-integration/drone/pr Build is failing Details

5bd9329391 clippy

trinity-1686a added 1 commit 2021-11-12 10:16:53 +00:00

continuous-integration/drone/pr Build is passing Details

dd9ae22f03 update cargo.nix

trinity-1686a commented

2021-11-12 10:20:32 +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 /)

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

continuous-integration/drone/pr Build is passing Details

df4b739617 update doc and comments

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