search

SteveLTN / https-portal

A fully automated HTTPS server powered by Nginx, Let's Encrypt and Docker.
MIT License
4.46k stars 295 forks source link

Mounting /etc/nginx/conf.d seems to really break things #268

Open gdbjohnson opened 3 years ago

gdbjohnson commented 3 years ago

I mounted the /etc/nginx/conf.d folder a while ago while debugging your solution, and left it in there forever. I upgraded the image recently, and it seems that if the conf file already exists for a site when the logic doesn't expect it to really breaks things. Since I'd mounted it, the files created in earlier runs persisted, which permanently broke recent logic. This had the unfortunate effect of causing the image to constantly spin, and I was rate limited by letsencrypt for all my domains.

You may want to put a surgeon general warning on this, or improve the logic around this piece.

SteveLTN commented 3 years ago

HI, sorry for your incident.

I think I have a warning against rate limiting, saying you should always use staging to try things out. Besides, I never mentioned in the document that you should mount /etc/nginx/conf.d. If you poke around the system by yourself, I think you are responsible for the possible outcome. I cannot possibly think of all the unexpected things the users will do to the container and put warning on all of them.

If you want to use your own configuration, should mount and override /var/lib/nginx-conf/default.ssl.conf.erb instead.

Anyway, the rate limit should be lifted in a week.

gdbjohnson commented 3 years ago

I'm not complaining, just think it's something you may want to add to your readme. I guess I could PR it, if you're ok with that.

SteveLTN commented 3 years ago

No, don't worry. I'm not complaining either. Just saying it's hard to consider everything situation one might encounter if they start mounting volumes. Hey, what if next time someone mounts /bin/setup?

If you can come up with some good, generic wording that can warn people about the situation, send a PR please. But no need to do a PR to fix the code, because we do have version planning, that will actually delete old Nginx configuration files. But only <domain>.conf and <domain.ssl.conf>, so if you add another files that we don't know of, we of course can't deal with it.

MarcelWaldvogel commented 3 years ago

Doesn't #261 cause conf.d files to be recreated more often, including at startup, so the problem should be gone when that is released?

SteveLTN commented 3 years ago

@MarcelWaldvogel Yes, it does. That's why I mentioned above: don't do a PR to fix the code.