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

Codespaces support, custom domain support, ssl cert support, what else? #466

Closed
wants to merge 20 commits into from

Conversation

shadyvb
Copy link
Contributor

@shadyvb shadyvb commented Apr 23, 2022

Refresh of #341

Rebased on top of SSL changes in #465 ( to make a clearer view of changes once that is merged )

Ref #277

shadyvb and others added 17 commits April 21, 2022 23:01
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.
Rather than letting AWS generate the domain, override to set explicit to
the Minio domain. In the process, also changes the Minio domain to be
fixed.
@shadyvb shadyvb force-pushed the support-codespaces-2 branch from b3180d2 to ec51c9a Compare April 23, 2022 03:49
@shadyvb shadyvb marked this pull request as draft April 23, 2022 18:54
@shadyvb shadyvb requested a review from roborourke April 23, 2022 18:54
@shadyvb shadyvb mentioned this pull request Apr 23, 2022
Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

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

Couple of notes inline.

DNS resolution is a pain, I was hoping as traefik runs on localhost:80 that might just work. Perhaps the Chassis approach with avahi might work https://github.com/damonmorgan/docker-mdns

(at least with docker we can ensure it's running!)

docker/conf/traefik.toml Show resolved Hide resolved
@@ -131,17 +132,17 @@ protected function get_php_reusable() : array {
'ELASTICSEARCH_HOST' => 'elasticsearch',
'ELASTICSEARCH_PORT' => 9200,
'AWS_XRAY_DAEMON_HOST' => 'xray',
'S3_UPLOADS_ENDPOINT' => "https://{$this->tld}/",
'S3_UPLOADS_ENDPOINT' => $this->set_url_scheme( "http://s3-{$this->hostname}.localhost/" ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Might have some related comments on the other PR about this, but using just the TLD was required due to the way the SDK generates URLs based on the configured region etc... and the way the Mnio/S3 server handles them to route appropriately.

We probably need to keep it pointing to $this->tld rather than localhost. Don't suppose you remember why this was @rmccue ?

Copy link
Contributor

Choose a reason for hiding this comment

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

See this comment before making any changes #466 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yes; I fixed this separately, see line I commented on in inc/namespace.php

@@ -557,7 +558,7 @@ protected function get_service_tachyon() : array {
'environment' => [
'AWS_REGION' => 'us-east-1',
'AWS_S3_BUCKET' => "s3-{$this->project_name}",
'AWS_S3_ENDPOINT' => "https://{$this->tld}/",
'AWS_S3_ENDPOINT' => $this->set_url_scheme( "http://{$this->tld}/" ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah maybe this was the problem re. the S3 endpoint environment variable in the PHP container config. It might just need to match the new value.

inc/namespace.php Outdated Show resolved Hide resolved
@@ -28,13 +28,17 @@ function bootstrap() {
add_filter( 's3_uploads_s3_client_params', function ( $params ) {
if ( defined( 'S3_UPLOADS_ENDPOINT' ) && S3_UPLOADS_ENDPOINT ) {
$params['endpoint'] = S3_UPLOADS_ENDPOINT;
$params['bucket_endpoint'] = true;
Copy link
Member

Choose a reason for hiding this comment

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

@roborourke This fixes the S3 SDK for the issue you mentioned.

Co-authored-by: Robert O'Rourke <[email protected]>
@shadyvb shadyvb requested a review from roborourke April 26, 2022 01:26
@shadyvb
Copy link
Contributor Author

shadyvb commented May 3, 2022

Closing down in favor of separating concerns between SSL and Codespaces.

@shadyvb shadyvb closed this May 3, 2022
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.

4 participants