Support website publishing #7

quentin · 2020-10-31T16:30:25Z

quentin commented

2020-10-31 16:30:25 +00:00

This PR is ready for review.
Fixes #6

It contains:

a new endpoint to serve buckets as websites
a new option in the bucket table to know if the website option is allowed on the bucket
a new command line option (garage bucket website --allow|--deny <bucket>)
Some tests in test-smoke.sh

This PR is ready for review. Fixes #6 It contains: - a new endpoint to serve buckets as websites - a new option in the bucket table to know if the website option is allowed on the bucket - a new command line option (`garage bucket website --allow|--deny <bucket>`) - Some tests in `test-smoke.sh`

quentin added 1 commit 2020-12-14 20:47:19 +00:00

Migrate RPC to new schema d0eb6a457f

quentin added 1 commit 2020-12-14 20:50:58 +00:00

Start to implement Website CLI a3566e49da

quentin added 1 commit 2020-12-15 12:06:47 +00:00

First implementation of the CLI 3bc4d57a0f

quentin added 1 commit 2020-12-15 12:23:48 +00:00

Push update 011ff87b5f

quentin added 1 commit 2020-12-17 19:43:57 +00:00

Web server access control 3132deca58

quentin added 1 commit 2020-12-17 20:05:22 +00:00

Update testing script 086e5be290

quentin added 1 commit 2020-12-17 20:10:42 +00:00

Merge branch 'master' into feature/website ccda9ab1ca

quentin changed title from ~~WIP: Support website publishing~~ to Support website publishing

2020-12-17 20:11:09 +00:00

quentin added 1 commit 2020-12-17 21:52:29 +00:00

Fix formatting 2f4378a9c4

lx requested changes 2021-01-13 21:21:22 +00:00

Dismissed

lx left a comment

There are a few minor changes to make.

The most important subject for discussion is concerning the root_domain configuration parameter: why do you think such a mechanism is necessary, rather than having bucket name = domain name as on AWS? (I would be against the root_domain mechanism but maybe there is an explanation I'm not aware of)

There are a few minor changes to make. The most important subject for discussion is concerning the `root_domain` configuration parameter: why do you think such a mechanism is necessary, rather than having bucket name = domain name as on AWS? (I would be against the `root_domain` mechanism but maybe there is an explanation I'm not aware of)

README.md

lx commented

2021-01-13 21:06:11 +00:00

Why break the convention that bucket name = domain name of the hosted website ? This gives more flexibility as we can host sites on diffferent tld/sld. If a website needs to move, we will copy files to the new bucket, it shouldn't be too costly (we don't expect to store TBs of data for a website...)

quentin commented

2021-01-15 11:08:41 +00:00

Please check my answer directly in the issue feed

quentin marked this conversation as resolved

src/garage/admin_rpc.rs

					
				@ -158,0 +174,4 @@

										));

									} else {

										return Err(Error::Message(format!(

											"Bucket is deleted in update_bucket_key"

lx commented

2021-01-13 21:07:30 +00:00

Probably put unreachable!() in else branch ? Because get_existing_bucket does not return a bucket that is deleted. (I think so, this should be checked)

Probably put `unreachable!()` in else branch ? Because get_existing_bucket does not return a bucket that is deleted. (I think so, this should be checked)

quentin commented

2021-01-15 14:29:06 +00:00

Checked.
Indeed, it does not return deleted buckets.

        async fn get_existing_bucket(&self, bucket: &String) -> Result<Bucket, Error> {
                self.garage
                        .bucket_table
                        .get(&EmptyKey, bucket)
                        .await?
                        .filter(|b| !b.is_deleted())
                        .map(Ok)
                        .unwrap_or(Err(Error::BadRPC(format!(
                                "Bucket {} does not exist",
                                bucket
                        ))))
        }

Checked. Indeed, it does not return deleted buckets. ```rust async fn get_existing_bucket(&self, bucket: &String) -> Result<Bucket, Error> { self.garage .bucket_table .get(&EmptyKey, bucket) .await? .filter(|b| !b.is_deleted()) .map(Ok) .unwrap_or(Err(Error::BadRPC(format!( "Bucket {} does not exist", bucket )))) } ```

quentin marked this conversation as resolved

src/garage/admin_rpc.rs

					
				@ -158,0 +176,4 @@

								} else {

									return Err(Error::Message(format!(

										"Bucket is deleted in update_bucket_key"

									)));

lx commented

2021-01-13 21:05:02 +00:00

update_bucket_key ?

`update_bucket_key` ?

quentin marked this conversation as resolved

src/web/Cargo.toml Outdated

					
				@ -0,0 +40,4 @@

				webpki = "0.21"

				roxmltree = "0.11"

lx commented

2021-01-13 21:10:45 +00:00

roxmltree, rustls, webpki, httpdate and maybe some others can probably be removed

quentin marked this conversation as resolved

src/web/error.rs

					
				@ -0,0 +4,4 @@

				use garage_util::error::Error as GarageError;

				#[derive(Debug, Error)]

				pub enum Error {

lx commented

2021-01-13 21:11:22 +00:00

are all variants used? (try commenting them to find out)

quentin commented

2021-01-15 15:28:08 +00:00

Now fixed, I commented all error types and decommented one by one according to the compiler errors. And once it compiled, I removed the remaining commented entries.

quentin marked this conversation as resolved

src/web/web_server.rs

					
				@ -0,0 +89,4 @@

					match bucket_desc.state.get() {

						BucketState::Deleted => Err(Error::NotFound),

						BucketState::Present(params) if !params.website.get() => Err(Error::NotFound),

						_ => Ok(()),

lx commented

2021-01-13 21:14:43 +00:00

Rewrite as only two cases:

BucketState::Present(params) if params.website.get() => Ok(()),
_ => Err(Error::NotFound),

Rewrite as only two cases: ``` BucketState::Present(params) if params.website.get() => Ok(()), _ => Err(Error::NotFound), ```

quentin marked this conversation as resolved

src/web/web_server.rs Outdated

					
				@ -0,0 +123,4 @@

						Ok(())

					}

					#[test]

lx commented

2021-01-13 21:17:16 +00:00

NO!! expect makes us crash (panic) if the condition is not verified, this is not acceptable (we don't want to assume that authority is well formed, maybe hyper checks it for us but maybe not and we don't want to take risks)

NO!! `expect` makes us crash (panic) if the condition is not verified, this is not acceptable (we don't want to assume that authority is well formed, maybe hyper checks it for us but maybe not and we don't want to take risks)

quentin marked this conversation as resolved

src/web/web_server.rs Outdated

					
				@ -0,0 +175,4 @@

						assert_eq!(

							host_to_bucket("john.doe.garage.tld", ".garage.tld"),

							"john.doe"

						);

lx commented

2021-01-13 21:18:51 +00:00

this case is unreachable!() thanks to the previous if

this case is `unreachable!()` thanks to the previous if

quentin marked this conversation as resolved

quentin commented

2021-01-15 11:04:15 +00:00

Thanks for your review lx,

The most important subject for discussion is concerning the root_domain configuration parameter: why do you think such a mechanism is necessary, rather than having bucket name = domain name as on AWS? (I would be against the root_domain mechanism but maybe there is an explanation I'm not aware of)

First, I would call this feature as "relative and absolute path resolving".
It tries to mimic as closely as possible Amazon S3 behavior, which in my point is an important reason if we want to follow the Principle Of Least Astonishment. So in the end, I am doing "as on AWS" ^^

Still, we might not want to implement blindly Amazon features.
I find this feature interesting as it has 2 positive side effects:

It will ease debugging/ux. Absolute domains like example.org will also be exposed as example.org.root-domain.org. As DNS propagation is slow, obtaining an HTTPS certificate may fail and hit rate limits, etc. it will enable users to test their website on our already configured endpoint (wildcard DNS+Certificate) to see if garage configuration is OK and start hacking on their website.
It enables for change in root domain name If you borrow administrators' domain name for your website, administrators must be able to change their root domain name without breaking all buckets. I know that it is not without drawbacks for users, but I think it is still less concerning than asking all your users to recreate a bucket and reconfigure their software.

Moreover, I can not see any important drawback for this feature apart from some complexity in the code.

So, in the end, I recommend keeping this feature but I would agree to drop it if you see an important drawback I have missed. Let met know what you think :)
I also plan to read and address your other comments as soon as possible ;)

Thanks for your review lx, > The most important subject for discussion is concerning the root_domain configuration parameter: why do you think such a mechanism is necessary, rather than having bucket name = domain name as on AWS? (I would be against the root_domain mechanism but maybe there is an explanation I'm not aware of) First, I would call this feature as "relative and absolute path resolving". It tries to mimic as closely as possible [Amazon S3 behavior](https://docs.aws.amazon.com/fr_fr/AmazonS3/latest/dev/WebsiteEndpoints.html), which in my point is an important reason if we want to follow the [Principle Of Least Astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment). So in the end, I am doing "as on AWS" ^^ Still, we might not want to implement blindly Amazon features. I find this feature interesting as it has 2 positive side effects: - **It will ease debugging/ux**. Absolute domains like example.org will also be exposed as example.org.root-domain.org. As DNS propagation is slow, obtaining an HTTPS certificate may fail and hit rate limits, etc. it will enable users to test their website on our already configured endpoint (wildcard DNS+Certificate) to see if garage configuration is OK and start hacking on their website. - **It enables for change in root domain name** If you borrow administrators' domain name for your website, administrators must be able to change their root domain name without breaking all buckets. I know that it is not without drawbacks for users, but I think it is still less concerning than asking all your users to recreate a bucket and reconfigure their software. Moreover, I can not see any important drawback for this feature apart from some complexity in the code. So, in the end, I recommend keeping this feature but I would agree to drop it if you see an important drawback I have missed. Let met know what you think :) I also plan to read and address your other comments as soon as possible ;)

👍 1

lx commented

2021-01-15 12:07:58 +00:00

Thank you. I understand better now. Let's keep it.

👍 1

quentin added 1 commit 2021-01-15 15:07:55 +00:00

Replace an already done check by unreachable!() f496e41ef4

quentin added 1 commit 2021-01-15 15:16:47 +00:00

Remove unused dependencies c441a358cd

quentin force-pushed feature/website from d19bb044b6 to 11a79a95dd

2021-01-15 15:26:15 +00:00

Compare

quentin added 2 commits 2021-01-15 16:04:39 +00:00

Doc tests that do not compile/work must be tagged with ignore 1e10c6a61c

Behavior problem: do not panic anymore + add tests fad7bc405b

quentin added 1 commit 2021-01-15 16:12:03 +00:00

Explicitly set code path unreachable f8a40e8c4f

quentin requested review from lx 2021-01-15 16:16:06 +00:00

quentin commented

2021-01-15 16:20:29 +00:00

So, I tried to address all your remarks, let me know if you disagree with my modifications ^^

Furthermore, cargo test + ./script/test-smoke.sh passed.
I also checked that this branch is synchronized with master.

Everything seems to be OK on my side, feel free to merge if you have no additional remarks :)

So, I tried to address all your remarks, let me know if you disagree with my modifications ^^ Furthermore, `cargo test` + `./script/test-smoke.sh` passed. I also checked that this branch is synchronized with master. Everything seems to be OK on my side, feel free to merge if you have no additional remarks :)

lx added 1 commit 2021-01-15 16:49:23 +00:00

Do not accept domains such as [hello 851893a3f2

lx merged commit 97494b4a19 into master

2021-01-15 16:49:51 +00:00

lx referenced this pull request from a commit

2021-01-15 16:49:52 +00:00

Merge pull request 'Support website publishing' (#7) from feature/website into master

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

2 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#7