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

adds support for https in ddev router #522

Merged
merged 9 commits into from
Nov 17, 2017

Conversation

tannerjfco
Copy link
Contributor

The Problem/Issue/Bug:

#232 OP - We want to support both HTTP and HTTPS for local dev without redirect.

How this PR Solves The Problem:

  • Updates to use provisional ssl tag for ddev-router, introducing https support w/ self-signed certs
  • Updates site docker-compose.yml template to define new HTTPS_EXPOSE
  • Updates router setup to ensure ports defined in HTTPS_EXPOSE are exposed on ddev-router.
  • Updates successful start message to include both HTTP and HTTPS URL options (I'm not sure if this is what we want or if we want to do something different)

Manual Testing Instructions:

This can be tested by building a binary from this PR and spinning up a test ddev site. You'll want to ensure you get a freshly-generated docker-compose for the site. The site should spin up successfully, and you should be able to access the site at both HTTP and HTTPS

Automated Testing Overview:

Related Issue Link(s):

Release/Deployment notes:

@tannerjfco tannerjfco self-assigned this Oct 25, 2017
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Shouldn't we be telling people the host-exposed port for http and https in describe? I actually can't believe we never included that for them yet. That can be done in a separate issue too (and could be done in the api/json issue; this one is likely to land sooner.)

I think it's worth thinking about how we can communicate that https is available on 443 in ddev list. I think it's important to do that.

@tannerjfco
Copy link
Contributor Author

How we communicate all of this is something we definitely need to improve. I think its worth displaying port usage, especially if the port has been customized to be different from 80 or 443. What if we displayed the port in the URLs in start message and describe, on condition of non-standard port?

@rfay
Copy link
Member

rfay commented Oct 25, 2017

I was actually talking about the bound host ports for accessing http and https. As you've pointed out before, people could actually use those directly and we should consider exposing the URL/port they would use that way; it would even give us a way to bail on bringing up the web container.

On this site:

            "Ports": {
                "443/tcp": null,
                "80/tcp": [
                    {
                        "HostIp": "0.0.0.0",
                        "HostPort": "32839"
                    }
                ],
                "8025/tcp": [
                    {
                        "HostIp": "0.0.0.0",
                        "HostPort": "32838"
                    }
                ]

Of course I can hit the website at http://localhost:32839/ as well as http://randyfay.ddev.local/ - allowing them the option of knowing about/using the host port with localhost gives them a workaround for both hostname resolution and for port 80 cleanup.

@rickmanelius
Copy link
Contributor

I'm planning on reviewing this tomorrow (Thursday). Sorry for not getting to it sooner.

Copy link
Contributor

@rickmanelius rickmanelius left a comment

Choose a reason for hiding this comment

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

Reference

Preparation

  • brew uninstall ddev (removes latest release binary)
  • git fetch --all
  • git checkout tannerjfco/router-https-support
  • make clean && make darwin
  • ddev version outputs v0.9.3-4-geb5b119a (matches hash of tannerjfco/router-https-support)
  • Shut down and remove all ddev sites (i.e. ddev rm --remove-data)
  • ddev list shows no running applications and docker ps shows no active containers.

Repositories Used

Testing

WordPress

  • Download, configure, and ddev start on WordPress 4.8.2
  • .ddev/config.yml containers:
    • webimage: drud/nginx-php-fpm-local:v0.8.0
    • dbimage: drud/mysql-local-57:v0.6.2
    • dbaimage: drud/phpmyadmin:v0.2.0
  • I can confirm ddev is pulling new Docker images.
  • Once the docker images are available, I can see the updated output, which includes:
  • Using Chrome
  • Firefox
    • Clicking HTTP brings me to http://wordpress.ddev.local/wp-admin/setup-config.php.
    • Clicking HTTPS https://wordpress.ddev.local with the big scary HTTPS warning.
    • Went to Advanced => Add Exception => Confirm Security Exception
    • I still got redirected to HTTP
    • I can go to HTTPS manually, but it doesn't load the CSS assets because they are HTTP
    • Removed app/docker images and restarted
    • Visited the HTTPS version first.
    • Confirmed the security exception and I'm redirected to HTTP
    • I can get to HTTPS version, but WordPress is still loading HTTP assets and thus the styling is breaking

Drupal 7

  • Download, configure, and ddev start on Drupal 7.56
  • .ddev/config.yml containers:
    • webimage: drud/nginx-php-fpm-local:v0.8.0
    • dbimage: drud/mysql-local-57:v0.6.2
    • dbaimage: drud/phpmyadmin:v0.2.0
  • Once the docker images are available, I can see the updated output, which includes:
  • Visiting either URL results in a "ddev web container: 500 or 501: Internal Server Error or Unimplemented" error.
  • Removing and retrying again.
  • Same issues are appearing for both the HTTP and HTTPS versions.

@tannerjfco
Copy link
Contributor Author

@rickmanelius The error message from your d7 test is from the web container, not ddev-router. I encountered it as well, and in my case it was due to a settings.php present from previous use pointing to an empty db. Once I resolved this, the site worked as expected for me.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I'm not really done, but here are some first reactions.

  • Right now, the tag here is not the same as the tip of docker.ddev-router, so not a very good test. Please update the ddev-router tag when you're ready. ( ddev-router:ssl-bump is v0.4.3-7-g305c0f2 but current tip is v0.4.3-8-g178232f)

  • I'd prefer that we name the certs $fqdn.crt and $fqdn.key - super-unambiguous. And a pretty normal practice anyway. That's mentioned over in the other PR, but mentioning here.

  • Currently, ddev-router is mounting the whole of ~/.ddev - that can be an enormous number of files, at least it is on my machine. I'm pretty sure we should just be mounting a certs directory. How about ~/.ddev/certs ? It would be better if it were associated with the site, but that seems very difficult and error-prone for ddev-router.

  • We have a number of settings.php-type problems that we'll have to understand and help people with. For D7 generate a settings.php with $base_url = http://... - I don't know of a reason we do that. Forcing the $base_url usually confuses drupal. So maybe we can just stop doing it?

I'm not really done, will come back for another round.

@tannerjfco
Copy link
Contributor Author

Right now, the tag here is not the same as the tip of docker.ddev-router, so not a very good test.

I have built/pushed v0.4.3-8-g178232f for router and updated to that in makefile

Currently, ddev-router is mounting the whole of ~/.ddev

This has been updated to mount ~/.ddev/certs instead

Forcing the $base_url usually confuses drupal. So maybe we can just stop doing it?

I'm pretty sure we can remove this, and I've done so. I think this was done to force the URL to be correct for things like drush uli, however this approach isn't valid for D8 as base_url was removed. We've since implemented a drushrc.php file to address this concern.

I also took some more time to play with sites in HTTPS, and just kept hitting mixed mode brokenness on sites. This is because we weren't setting $_SERVER['HTTPS'] = 'on'; so that PHP applications know to serve over HTTPS. I've introduced drud/docker.nginx-php-fpm-local#40 to address this issue in a way that should allow most PHP apps to serve over HTTPS correctly.

beeradb
beeradb previously requested changes Nov 2, 2017
Copy link
Contributor

@beeradb beeradb left a comment

Choose a reason for hiding this comment

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

The code here is not objectionable to me, however...

I have a concern about this which is unrelated to the code. Specifically, what's the upgrade path look like for this? We output https links in this PR, but it requires a new docker-compose.yaml file. It took me a few minutes of wondering why HTTPS wasn't responding before I put 2 and 2 together. I'm not sure how our users will fare.

Just spitballing here, but I'd suggest one of the following courses of action:

  1. Consider using the version string in our config.yaml file to help determine what capabilities configured sites should have.
  2. Check the web container after start to see what ports it exposes. Only output HTTPs links if it's exposing port 443.

Now that we have official releases and end-users, we need to key an eye on any features like this that affect our configuration and/or docker-compose files to ensure we're not breaking users.

@tannerjfco
Copy link
Contributor Author

I have updated this to conditionally display the HTTPS versions of the URL at start and in list/describe. We still have the problem of how users will update to get HTTPS on existing sites/configs, as they will need an updated web image tag and the HTTPS_EXPOSE var set in the site docker-compose.yml. However, these are issues that extend beyond the concerns of this particular feature. I think #343 could help with some of this, but either way we'll need to figure out a path forward.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I didn't have any trouble with this on D7 or D8, everything was clean as could be.

I do note a regression about handling unknown hosts - they seem to go to the default URL. So if I hit junker99.ddev.local, it lands on the first configured site. It didn't behave that way in v0.9.3, here's v0.9.3:

screenshot_11_16_17__12_49_pm

@rickmanelius
Copy link
Contributor

Next review in progress (will re-update and edit this comment with final testing and recommendations).

Reference

Preparation

  • brew uninstall ddev (removes latest release binary)
  • git fetch --all
  • git checkout tannerjfco/router-https-support
  • make clean && make darwin
  • ddev version outputs v0.9.3-19-g5e80b698 (matches hash of tannerjfco/router-https-support)
  • Shut down and remove all ddev sites (i.e. ddev rm --remove-data)
  • ddev list shows no running applications and docker ps shows no active containers.

Repositories Used

@rickmanelius rickmanelius self-requested a review November 17, 2017 16:02
Copy link
Contributor

@rickmanelius rickmanelius left a comment

Choose a reason for hiding this comment

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

Reference

Preparation

  • brew uninstall ddev (removes latest release binary)
  • git fetch --all
  • git checkout tannerjfco/router-https-support
  • make clean && make darwin
  • ddev version outputs v0.9.3-19-g5e80b698 (matches hash of tannerjfco/router-https-support)
  • Shut down and remove all ddev sites (i.e. ddev rm --remove-data)
  • ddev list shows no running applications and docker ps shows no active containers.

Repositories Used

Testing

WordPress

Drupal 7

  • Download, configure, and ddev start on Drupal 7.56
  • .ddev/config.yml containers:
    • webimage: drud/nginx-php-fpm-local:v0.8.1
    • dbimage: drud/mysql-local-57:v0.6.2
    • dbaimage: drud/phpmyadmin:v0.2.0
  • Once the docker images are available, I can see the updated output, which includes:
  • I can visit both versions independently and each works fine.
  • CONFIRMED

Other Feedback

  • I do note that ddev describe now stacks two rows to show both the HTTP and HTTPS versions. I kinda feel like pushing the HTTPS version as best practice would make more sense and make it easier to read versus using two rows per. This may ultimately become a moot point if there is a later feature to force one over the other, which will likely be necessary to keep the WordPress experience consistent given the URL rewriting. However, if we feel strongly about not pushing the HTTPS for now, I'm willing to revisit later.

@rickmanelius
Copy link
Contributor

@beeradb I know you had some open questions on upgrade path which I know is valid but probably best scoped to another issue where we discuss who maintains and manages the settings files, the docker compose files, the config.yml, etc because those are 100% valid concerns but not entirely attached to the scope of this specific ticket and something we will likely need to better address with all future updates.

@beeradb beeradb dismissed their stale review November 17, 2017 17:33

The other reviews look quite thorough.

@tannerjfco tannerjfco merged commit 97d03df into ddev:master Nov 17, 2017
@rickmanelius rickmanelius mentioned this pull request Nov 20, 2017
6 tasks
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.

None yet

4 participants