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

Make very long server names possible #1484

Merged
merged 4 commits into from Mar 18, 2019

Conversation

Projects
None yet
5 participants
@janhelke
Copy link
Contributor

janhelke commented Mar 5, 2019

The Problem/Issue/Bug:

Having long server names causes the ddev-router to cease working.
Error message during start:
nginx: [emerg] could not build server_names_hash, you should increase server_names_hash_bucket_size: 64

How this PR Solves The Problem:

This patch increases server_names_hash_bucket_size to the maximum possible size (128).

Manual Testing Instructions:

Create a server name longer than 32 characters and you should see, that none of your projects will start again.

Make very long server names possible
## The Problem/Issue/Bug:

Having long server names causes the ddev-router to cease working.
Error message during start:
```nginx: [emerg] could not build server_names_hash, you should increase server_names_hash_bucket_size: 64```

## How this PR Solves The Problem:

This patch increases server_names_hash_bucket_size to the maximum possible size (128).

## Manual Testing Instructions:

Create a server name longer than 32 characters and you should see, that none of your projects will start again.
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Mar 5, 2019

CLA assistant check
All committers have signed the CLA.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 5, 2019

Thanks for this! How long are your server names? :)

@janhelke

This comment has been minimized.

Copy link
Contributor Author

janhelke commented Mar 5, 2019

Currently? 45 chars. But as you might know that will grow :-)

@janhelke

This comment has been minimized.

Copy link
Contributor Author

janhelke commented Mar 5, 2019

Anything I can do about the failing tests?

@kaystrobach

This comment has been minimized.

Copy link

kaystrobach commented Mar 5, 2019

Jan i thnik you need to sign the sla ... then it will be fine.

@andrewfrench

This comment has been minimized.

Copy link
Member

andrewfrench commented Mar 5, 2019

@janhelke I'm looking into the test failures, they don't seem to be related to your changes.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 5, 2019

Appveyor isn't yet supported in master (and may not be) so it's no-worries.

I pushed a web container version with this in it: drud/ddev-webserver:20190305_longer_nginx_server_names

It can now be tested with that by altering the config.yaml.

We need an edit in pkg/version/version.go pointing to that tag though please (20190305_longer_nginx_server_names) . Just edit

var WebTag = "v1.6.0" // Note that this can be overridden by make
and push another commit.

All we need after that is a successful test run (not including appveyor of course) and somebody reporting back on manual testing.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 6, 2019

I took the liberty of pushing in that one commit to push the new docker container version tag, hope that's OK.

@rfay rfay added this to the v1.6.1 milestone Mar 6, 2019

@rfay

rfay approved these changes Mar 6, 2019

Copy link
Member

rfay left a comment

This looks totally fine to me, thanks! When it's passed automated testing, and when manual testing has confirmed it working correctly, it can be pulled. It can already be tested by using webimage: drud/ddev-webserver:20190305_longer_nginx_server_names in .ddev/config.yaml. I'll post the ddev artifacts when the build finishes.

@janhelke

This comment has been minimized.

Copy link
Contributor Author

janhelke commented Mar 6, 2019

I can confirm that after inserting the modified webimage in the affected config.yaml, shutting down all containers and restart, everything works fine.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 6, 2019

What hostname did you test with? If there's space enoiin the comment section!

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 6, 2019

Hmm, trying to test using name: a1234567890b1234567890c1234567890d1234567890e1234567890f1234567890 in .ddev/config.yaml, and I note that while curl, ping, and telnet can handle the resultant hostname, both Chrome and Safari (on macOS) refuse to resolve it. So that could be a problem.

@rfay
Copy link
Member

rfay left a comment

I messed this up completely, should have pushed the ddev-router as a new container tag, not the web container.

I wonder though: Does this fix need to be applied in the web container?

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 6, 2019

Note that testing this PR requires a ddev rm -a before the ddev start with the new version; otherwise the new router isn't generated.

@janhelke

This comment has been minimized.

Copy link
Contributor Author

janhelke commented Mar 9, 2019

Sorry. My bad. I tried the webcontainer change and was under the impression, that this would help. But it doesn't. So from my point of view no change on the webcontainer is needed, only the one on the router.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 9, 2019

It was my bad. I have now pushed the router, and will post the artifacts for ddev shortly (you have to use the ddev version from artifacts to test). But again, I find the results in #1484 (comment) - If you want to use Chrome or Safari on macOS to use huge hostnames it doesn't seem to work (curl works fine). This article has links to all the necessary info on URL lengths, and implies that there should be no problem, but I sure have a problem.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 9, 2019

Artifacts for testing are at https://circleci.com/gh/drud/ddev/10617#artifacts/containers/0 - I would imagine that just configuring a simple project with a long hostname is good enough. So download the ddev you need, ddev rm -a and ddev start your long-name project.

@rfay

This comment has been minimized.

Copy link
Member

rfay commented Mar 13, 2019

I tested this with name: a1234567890b1234567890c1234567890d1234567890 (resulting in a 44+5+6=56 character URL) and it seemed to work everywhere.

However, this will need to be tested and approved by the OP to get in, we need to make sure it does what you need, otherwise it will be closed. (That means you @janhelke !)

  • Configure a long, problematic name
  • ddev rm -a
  • ddev start
  • Confirm adequate behavior for your use case.

Artifacts for testing are in https://circleci.com/gh/drud/ddev/10617#artifacts/containers/0

@janhelke

This comment has been minimized.

Copy link
Contributor Author

janhelke commented Mar 14, 2019

I can confirm, that using the provided artifacts our projects (even the one with the very long names) work like a charm. Thanks for all you afford and sorry for being so late with my response.

@rfay

rfay approved these changes Mar 18, 2019

@rfay rfay merged commit 671e1b1 into drud:master Mar 18, 2019

7 of 8 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
buildkite/ddev-containers-windows Build #1493 passed (24 minutes, 15 seconds)
Details
buildkite/ddev-windows-apache-fpm Build #1060 passed (1 hour, 32 minutes, 30 seconds)
Details
buildkite/ddev-windows-dockerforwindows Build #2031 passed (1 hour, 44 minutes, 35 seconds)
Details
buildkite/ddev-windows-dockerforwindows-nfs Build #177 passed (2 hours, 5 minutes, 29 seconds)
Details
buildkite/ddev-windows-dockertoolbox Build #1637 passed (50 minutes, 10 seconds)
Details
buildkite/ddev-windows-dockertoolbox-nfs Build #173 passed (50 minutes, 42 seconds)
Details
license/cla Contributor License Agreement is signed.
Details

@rfay rfay referenced this pull request Apr 1, 2019

Closed

v1.7.0 Release Checklist Due 2019-04-02 #1454

8 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.