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 NGINX regression Docker Expose #2762

Merged
merged 1 commit into from
May 30, 2022

Conversation

Alkarex
Copy link
Contributor

@Alkarex Alkarex commented May 30, 2022

#2721 introduced a regression by not properly declaring the exposed port in Dockerfile.
The Apache version did it correctly:
https://github.com/docker-library/php/blob/faf8864e3845ced80780c03eefc66c022e2f9ac1/8.1/buster/apache/Dockerfile#L289

The consequence is that other systems, e.g. Traefik, could not know what port to route to.

RSS-Bridge#2721 introduced a regression by not properly declaring the exposed port in Dockerfile.
The Apache version did it correctly:
https://github.com/docker-library/php/blob/faf8864e3845ced80780c03eefc66c022e2f9ac1/8.1/buster/apache/Dockerfile#L289

The consequence is that other systems, e.g. Traefik, could not know what port to route to.
@dvikan
Copy link
Contributor

dvikan commented May 30, 2022

Why can't Traefik forward requests to 127.0.0.1:80? I don't understand the purpose of EXPOSE. As far as I can tell its purpose is internal container communication?

Also keep in mind that we have a configuration setting for changing the port that nginx binds to.

In summary, maybe our docker setup isn't properly written for real production use.

@Alkarex
Copy link
Contributor Author

Alkarex commented May 30, 2022

Of course it can be done manually and/or overridden but with a default EXPOSE, routing can be done automatically without having to hunt in the documentation which port each service is using internally.

You can for instance try a docker ps with and without the EXPOSE instruction and observe the difference.

Not related but just for the record, 127.0.0.1 would not work because localhost in a container is not the same localhost in another container.

@Alkarex
Copy link
Contributor Author

Alkarex commented May 30, 2022

Reference for Traefik, but it is not specific to it:

https://doc.traefik.io/traefik/providers/docker/#port-detection

image

Copy link
Contributor

@yamanq yamanq left a comment

Choose a reason for hiding this comment

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

Good catch, thanks for contributing! I won't merge this yet in case @dvikan has some comments.

@Bockiii
Copy link
Contributor

Bockiii commented May 30, 2022

You are correct. I'm running this in docker-compose with defined ports, but in a helm scenario, the automated port assigning wouldnt work.

I'm approving this change, but there will probably be another change in the near future because I want to switch to the lsio nginx container. So thanks!

@Bockiii Bockiii merged commit 05cd1c0 into RSS-Bridge:master May 30, 2022
@Alkarex Alkarex deleted the fix-nginx-regression branch May 30, 2022 19:09
senrou added a commit to senrou/rss-bridge that referenced this pull request May 31, 2022
Kwbmm pushed a commit to Kwbmm/rss-bridge that referenced this pull request Jun 17, 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