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

Fix empty dhparam.pem #1213

Merged
merged 1 commit into from
Feb 5, 2019
Merged

Conversation

seroron
Copy link
Contributor

@seroron seroron commented Dec 18, 2018

When launch the container, the default dhparam.pem is cleared to 0 by shell redirection while creating a new dhparam.pem.

# docker exec -it 0841481db300 bash
root@0841481db300:/app# cd /etc/nginx/dhparam/
root@0841481db300:/etc/nginx/dhparam# ls -lha
total 0
drwxr-xr-x 1 root root 25 Dec 16 13:40 .
drwxr-xr-x 1 root root 35 Dec 16 12:18 ..
-rw-r--r-- 1 root root  0 Dec 16 13:40 dhparam.pem

So, nginx sometimes fails to initialize.

# docker logs 0841481db300
WARNING: /etc/nginx/dhparam/dhparam.pem was not found. A pre-generated dhparam.pem will be used for now while a new one
is being generated in the background.  Once the new dhparam.pem is in place, nginx will be reloaded.
forego     | starting dockergen.1 on port 5000
forego     | starting nginx.1 on port 5100
dockergen.1 | 2018/12/16 13:40:02 Generated '/etc/nginx/conf.d/default.conf' from 7 containers
dockergen.1 | 2018/12/16 13:40:02 Running 'nginx -s reload'
dockergen.1 | 2018/12/16 13:40:02 Error running notify command: nginx -s reload, exit status 1
dockergen.1 | 2018/12/16 13:40:02 Watching docker events
dockergen.1 | 2018/12/16 13:40:03 Contents of /etc/nginx/conf.d/default.conf did not change. Skipping notification 'nginx -s reload'

@kamermans
Copy link
Contributor

Good catch @seroron - this looks good to me @jwilder

@arneschreuder
Copy link

Can this PR be merged. Should solve #1226

kamermans added a commit to kamermans/nginx-proxy that referenced this pull request Feb 4, 2019
…beating the nginx startup. This is fixed permanently in nginx-proxy#1213, but this PR fixes the test so as not to rely on the dhparam autogen, which is tested elsewhere.
@kamermans
Copy link
Contributor

FYI: I have spent an hour or two trying to come up with a test for this (ex: using Python pyinotify with Threaded/AsyncNotifier to watch the file), but there is no clean way to write a regression test for this in my opinion.

My feeling is that this PR is implemented properly and it should be merged ASAP to avoid more problems. It seems something changed recently that causes this bug to become more prominent, either in nginx or in docker, since it didn't start appearing until recently, and now even the Travis-CI tests are failing because of it.

@jwilder jwilder merged commit 53a396f into nginx-proxy:master Feb 5, 2019
NicolasCARPi added a commit to elabftw/elabimg that referenced this pull request Jul 4, 2019
gmaclennan added a commit to gmaclennan/nginx-proxy that referenced this pull request Jan 8, 2020
* 'master' of https://github.com/jwilder/nginx-proxy: (40 commits)
  Upgrade to 1.17.5
  Fix comment about Mozilla Modern Policy and TLS1.3
  Typo
  README.md: fix version in nginx banner
  Update ssl configuration
  Use nginx 1.17.3
  Fix the test
  Use nginx latest version
  Update README.md
  Fixed tests that are now failing due to the dhparam clearing command beating the nginx startup.  This is fixed permanently in nginx-proxy#1213, but this PR fixes the test so as not to rely on the dhparam autogen, which is tested elsewhere.
  Fix empty dhparam.pem
  Upgrade to nginx 1.14.1 stable version
  Fixed nginx-proxy#1080, can't disable HSTS with noredirect
  Upgrade to nginx 1.14 stable
  Added TLSv1.3 support
  Remove old docker.list to avoid getting unstable Docker version
  Increased dependency versions to get around pip internal problem
  Fixed out-of-scope variable
  fix fastcgi bug
  fix fastcgi bug
  ...
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