Allow "altis.dev" TLD to be configured #237

joehoyle · 2020-11-29T19:16:11Z

With the "HTTPS required" natire of .dev domains, the fact that we
currently have to issue a fully signed wildcard cert (.dev doesn't
support self signed certs), and then the fact that we can't support
subdomain installs in local-server; I think the days of altis.dev are
probably numbered. This PR atleast makes this TLD fully configurable, to
something more .local or .altis.local.

In doing so, I also added an option to set secure to true / false,
so HTTP-only can then be supported with TLDs other than .dev.

At this point I don't think we need to publicly document this
neccesarily, but I think we are going to need to move away from this
hardcoding of altis.dev. If nothing else, this makes the code more
configurable in special use cases.

With the "HTTPS required" natire of `.dev` domains, the fact that we currently have to issue a fully signed wildcard cert (`.dev` doesn't support self signed certs), and then the fact that we can't support subdomain installs in local-server; I think the days of `altis.dev` are probably numbered. This PR atleast makes this TLD fully configurable, to something more `.local` or `.altis.local`. In doing so, I also added an option to set `secure` to `true` / `false`, so HTTP-only can then be supported with TLDs other than `.dev`. At this point I don't think we need to publicly document this neccesarily, but I think we are going to need to move away from this hardcoding of altis.dev. If nothing else, this makes the code more configurable in special use cases.

joehoyle · 2020-11-29T19:17:25Z

docker/conf/traefik.toml

@@ -153,7 +153,6 @@ address = ":8080"
 # Optional
 # Default: ""
 #
-domain = "altis.dev"


@roborourke I couldn't actually see where this was used. The docs say it's a default, do you know impact of removing this?

We could maybe generate that file too. Honestly best off asking @nathanielks but if it all works without it then that’s ok

AFAIK this won't impact anything. As you say, it's a default and I'm assuming the value would already be set by the Host header.

Docs for posterity

roborourke

Thanks @joehoyle!

There are a couple of little bugs in there that I've commented on inline, plus I also just merged in something that adds a 3rd parameter to the docker file generator so there's a conflict there. Should be easy resolve, I'm just passing in an array of extra 'stuff' now so you could add the TLD to that.

Are there any issues with the built in certificates being mounted in the traefik container when not using the .altis.dev TLD?

I think what's maybe missing here is a way to add your own certs to the Traefik proxy if you do change the TLD. How would I use something like mkcert to create self signed certs and use them with local server? That's something I'd like to see docs for and potentially a command that can automate the use of mkcert, or show installation instructions if missing.

Last thing is why not document this now? I don't think this is something we would backport (though could make a preview release of course). I'd like to avoid adding things without documenting them without at least creating a follow up issue to address it.

roborourke · 2020-12-02T17:44:22Z

inc/composer/class-command.php

+		$config = $this->get_composer_config();
+
+		if ( isset( $config['tld'] ) ) {
+			$project_name = $config['tld'];


Do we need some validation here? Guess it would quickly become apparent if something is wrong but we may want to pass an error back if the value won't work as a TLD

Hmm yeah I'd be for die( $message ) to make it clear, though we might want to bubble that error yeah.

roborourke · 2020-12-02T17:48:55Z

inc/composer/class-docker-compose-generator.php

@@ -55,11 +55,11 @@ class Docker_Compose_Generator {
 	 * @param string $project_name The docker compose project name.
 	 * @param string $root_dir The project root directory.
 	 */
-	public function __construct( string $project_name, string $root_dir ) {
+	public function __construct( string $project_name, string $root_dir, string $tld ) {


Sorry, I already added a 3rd argument here more generally for $args in a PR to fix the xdebug functionality #240, could update this to pass the tld and any other config options that way. Probably the whole config would be useful with some defaults.

As it happens, I think I can remove. We already pull the project config in this class anyway, so we can just read TLD from there

inc/namespace.php

inc/composer/class-command.php

Co-authored-by: Robert O'Rourke <[email protected]>

joehoyle · 2020-12-04T14:08:46Z

Are there any issues with the built in certificates being mounted in the traefik container when not using the .altis.dev TLD?

Yeah, so atm, you'd need to manually replace those in vendor/altis/local-server which is a hack. I was using secure: false myself to test / use this.

I think what's maybe missing here is a way to add your own certs to the Traefik proxy if you do change the TLD. How would I use something like mkcert to create self signed certs and use them with local server? That's something I'd like to see docs for and potentially a command that can automate the use of mkcert, or show installation instructions if missing.

Yeah would love to use mkcert for exactly that. I don't know about how to update traefik to support multiple projects, all using their own certs. it might be that you can just list them all out and it will cleverly use the one matching the request host. There is traefik/traefik#2233 which I was looking at. I've been following along with traefik/traefik#5507 (comment) to support dynamic certs from labels, which would be ideal.

Last thing is why not document this now? I don't think this is something we would backport (though could make a preview release of course). I'd like to avoid adding things without documenting them without at least creating a follow up issue to address it.

Specifically because we haven't solved SSL yet. I'd recommend on merging, and have it as an undocumented feature until we solve SSL too.

nathanielks · 2020-12-04T17:00:01Z

I don't know about how to update traefik to support multiple projects

In v2, Traefik's config has been split into static configuration (eg traefik.yml) and dynamic configuration. The dynamic config is a directory Traefik watches and will reload when files are added/modified/etc in the specific directory. My thought when I was playing around was we'd have a ~/.altis-local-server directory that's mounted into the Traefik proxy and each individual proxy would create a file for its SSL config in that directory.

joehoyle · 2021-09-06T09:41:32Z

@rmccue do you have anything to commit here in your trials with Codespaces. E.g. I think you accounted for no-told (localhost)

rmccue · 2021-09-06T09:43:03Z

Yeah, work under way in https://github.com/humanmade/altis-local-server/tree/support-codespaces to support tld-less configurations, but Codespaces needs a few more things which I'll need to push up too.

joehoyle · 2021-10-21T09:49:25Z

@roborourke could I get a re-review on this? IMO we should merge this mostly as is, and then #341 is largely a follow-up.

rmccue · 2021-10-21T10:15:55Z

@joehoyle S3 was entirely broken with this which I fixed in #341, so imo we should just do #341.

roborourke · 2021-10-21T11:16:00Z

Yeah looks like #341 has all the same stuff so we can close this in favour of that one. It'll need to be rebased on master or at least have the merge conflicts resolved.

joehoyle · 2021-10-25T09:22:55Z

Ok, closing this one out in that case

hm-linter bot approved these changes Nov 29, 2020

View reviewed changes

joehoyle requested a review from roborourke November 29, 2020 19:16

joehoyle commented Nov 29, 2020

View reviewed changes

roborourke requested changes Dec 2, 2020

View reviewed changes

joehoyle and others added 2 commits December 4, 2020 09:01

Update inc/namespace.php

0fdce48

Co-authored-by: Robert O'Rourke <[email protected]>

Update inc/composer/class-command.php

ba2827f

Co-authored-by: Robert O'Rourke <[email protected]>

jennybeaumont added the should have Should be done, medium priority for now label Jan 14, 2021

PriyaBhatia0210 mentioned this pull request Mar 18, 2021

Allow "altis.dev" TLD to be configured #277

Closed

Merge branch 'master' into configure-tld

f165a39

joehoyle and others added 3 commits September 27, 2021 10:12

Merge branch 'master' into configure-tld

4e6181a

Merge branch 'master' into configure-tld

9788978

Docs for $tld var

168a733

joehoyle requested a review from roborourke October 21, 2021 09:48

joehoyle closed this Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow "altis.dev" TLD to be configured #237

Allow "altis.dev" TLD to be configured #237

joehoyle commented Nov 29, 2020

joehoyle Nov 29, 2020

roborourke Nov 29, 2020

nathanielks Dec 2, 2020

nathanielks Dec 2, 2020

roborourke left a comment

roborourke Dec 2, 2020

joehoyle Dec 4, 2020

roborourke Dec 2, 2020

joehoyle Dec 4, 2020

joehoyle commented Dec 4, 2020

nathanielks commented Dec 4, 2020

joehoyle commented Sep 6, 2021

rmccue commented Sep 6, 2021

joehoyle commented Oct 21, 2021

rmccue commented Oct 21, 2021

roborourke commented Oct 21, 2021

joehoyle commented Oct 25, 2021

Allow "altis.dev" TLD to be configured #237

Allow "altis.dev" TLD to be configured #237

Conversation

joehoyle commented Nov 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roborourke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehoyle commented Dec 4, 2020

nathanielks commented Dec 4, 2020

joehoyle commented Sep 6, 2021

rmccue commented Sep 6, 2021

joehoyle commented Oct 21, 2021

rmccue commented Oct 21, 2021

roborourke commented Oct 21, 2021

joehoyle commented Oct 25, 2021