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

Base images should rarely expose ports #68

Closed
wants to merge 1 commit into from

Conversation

rbellamy
Copy link
Contributor

Given that it is unlikely that the alpine-nginx image would be deployed
as-is, it should not expose ports. Instead that should be left to the
image that uses alpine-nginx as its base.

This is especially true because, unlike CMD or ENTRYPOINT a derived
image cannot override the EXPOSE command of a base image. This means
that should a derived image want nginx to not expose ports 80 or 443,
they cannot use this as a base image.

Also, nginx logs should always be redirected to stdout and stderr, and
therefore the commands to make this happen should remain the
responsibility of alpine-nginx base image.

Given that it is unlikely that the alpine-nginx image would be deployed
as-is, it should not expose ports. Instead that should be left to the
image that uses alpine-nginx as its base.

This is especially true because, unlike `CMD` or `ENTRYPOINT` a derived
image _cannot_ override the `EXPOSE` command of a base image. This means
that should a derived image want nginx to _not_ expose ports 80 or 443,
they cannot use this as a base image.

Also, nginx logs should always be redirected to stdout and stderr, and
therefore the commands to make this happen should remain the
responsibility of alpine-nginx base image.
@smebberson
Copy link
Owner

@rbellamy, yeah I agree on the ports issue. Let me think about the logs issue a little more...

@rbellamy
Copy link
Contributor Author

@smebberson I'd be interested in hearing a use-case where you would NOT want the nginx logs forwarded to stdout or stderr in a running Docker image.

@smebberson
Copy link
Owner

@rbellamy, I run multiple instances of alpine-nginx and alpine-nodejs and therefore anything going to stdout and stderr is cumbersome to get to. I push all logs centrally to a remote log aggregator and I can view everything that happened from there.

Much nicer than trying to get stdout for a particular container, when you're running many instances.

A quick Google search gives you plenty of mixed opinions on this matter. I would say the majority log to stdout and stderr while pushing data out to aggregators.

I mean, I guess all of the other services here do log to stdout and stderr as a default. s6-overlay has some logging utilities too https://github.com/just-containers/s6-overlay#logging

@rbellamy
Copy link
Contributor Author

rbellamy commented Oct 29, 2016

From my admittedly limited experience and reading, the "best practice" for log aggregation seems to be to offer that as a service within kubernetes, not within the docker image itself (e.g. https://github.com/deis/fluentd).

While I (obviously) understand the need and use of the s6-overlay within the docker image, I think it's important to guard against a leaky abstraction, and not fall into the trap of expecting more from the docker image than is absolutely necessary. My 2¢.

@rbellamy rbellamy mentioned this pull request Feb 15, 2017
@rbellamy
Copy link
Contributor Author

I'm considering the best way to allow a downstream image to modify the nginx config - and debating with myself whether overwriting either /etc/nginx/nginx.conf or /etc/nginx/conf.d/default.conf is best, or some kind of structural solution with included confs (like the include /etc/nginx/conf.d/*.conf; stanza in nginx.conf).

@rbellamy
Copy link
Contributor Author

An example of the use this could be put to is removing the main logging stanza from the nginx.conf and putting a standard one in the conf.d directory. This would make a "sane" choice for the upstream image (pending the result of our current discussion) but would also allow a downstream image to alter nginx logging behavior.

I see a similar need in configuring the server section.

@rbellamy
Copy link
Contributor Author

One last thing and I'll stop spamming you - in order to get the http_realip_module module we'll need the newest version - our current image doesn't have it. That module is pretty much required for nginx to properly resolve real vs proxied IP addresses - something pretty important for containers that are running behind proxies (probably almost all?).

@smebberson
Copy link
Owner

I'm closing this, because I think the PR is a little premature and more discussion needs to take place. I've created #81 and #82 to continue the discussion there.

@smebberson smebberson closed this Mar 29, 2017
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

2 participants