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

Add Alpine linux Dockerfile in addition to the existing Debian one #465

Merged
merged 1 commit into from Jan 16, 2018
Merged

Add Alpine linux Dockerfile in addition to the existing Debian one #465

merged 1 commit into from Jan 16, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 6, 2017

This PR replaces original Dockerfile with a re-worked version. New version builds a docker image based on Alpine Linux, which significantly reduces image size (to ~8.5MiB).


This change is Reviewable

@nurupo
Copy link
Member

nurupo commented Feb 6, 2017

The image size reduction is good. I'm not familiar with Alpine Linux, so it might be harder for me to maintain the Dockerfile, but Alpine Linux looks simple enough and even has a web interface to look up packages, so I will give it a try.

I have only quickly glanced over the Dockerfile, still need to double-check it and test it, which I will do sometime this week.

@nurupo
Copy link
Member

nurupo commented Feb 6, 2017

other/bootstrap_daemon/docker/Dockerfile, line 61 at r1 (raw file):

	sed -i 's/enable_lan_discovery = true/enable_lan_discovery = false/' /etc/tox-bootstrapd.conf && \
# Modify relay ports, 443/tcp is often already in use
	sed -i 's/tcp_relay_ports = \[443, 3389, 33445\]/tcp_relay_ports = \[8080, 33445\]/' /etc/tox-bootstrapd.conf && \

You shouldn't remove ports 443 and 3389, we want the bootstrap node to listen on them because these ports are often open in firewalls. In particular the firewalls a user is behind when trying to connect to your bootstrap node (e.g. university firewall), not the firewall that is on the machine running the bootstrap node.


Comments from Reviewable

@ghost
Copy link
Author

ghost commented Feb 7, 2017

@nurupo, I would agree on leaving 3389 enabled, but not about 443. From https://nodes.tox.chat/json we can see that there are only 2-3 hosts out of 72, who have 443 open and have Tox network node service running on it. Quick nmap shows that 32 hosts out of those 72 use port 443/tcp for https service.

@nurupo
Copy link
Member

nurupo commented Feb 8, 2017

I think the port 443 should be used by default. Having a node running on port 443 is a big win for tox users, as they could connect to the tox network in a network with restrictive firewall. If you remove port 443 from the default configuration, how are the bootstrap node maintainers supposed to know that they should add it if they don't run a https service? But if they do run a https service, they would see during the daemon setup that the daemon uses port 443 and would remove it. Simply removing -p 443:443 from the docker run command should be sufficient to disable use of that port, I think (my Docker knowledge is somewhat limited).

@ghost
Copy link
Author

ghost commented Feb 8, 2017

OK, left with default ports.

@iphydf iphydf added this to the v0.1.7 milestone Feb 10, 2017
@nurupo
Copy link
Member

nurupo commented Feb 12, 2017

I'm having hard time finding documentation for adduser used in Alpine Linux. It doesn't seem to like any of long options.

@nurupo
Copy link
Member

nurupo commented Feb 12, 2017

Thinking about it more, I'd like to keep the image use Debian. If you want smaller size, you can change it from debian:jessie to debian:jessie-slim, which should bring the size down from 120MiB to about 80MiB.

@SkyzohKey SkyzohKey added the packaging Packaging label Feb 13, 2017
@ghost
Copy link
Author

ghost commented Feb 17, 2017

All up to you. Smaller image - smaller attack surface, smaller footprint, smaller downloads. Additional benefit - bootstrap daemon on Alpine is compiled and running with musl libc.

It doesn't seem to like any of long options.

Yes, only short options. It's BusyBox utils, not something unknown/third-party/specific: https://www.busybox.net/downloads/BusyBox.html#adduser

@iphydf
Copy link
Member

iphydf commented Mar 5, 2017

Check the checkbox that allows collaborators to push to the PR branch. Also, rebase on master.

@nurupo can you provide a rationale for your preference?

@iphydf iphydf modified the milestones: v0.1.7, v0.1.8 Mar 26, 2017
@iphydf
Copy link
Member

iphydf commented Apr 12, 2017

@romik-g can you rename your file to Dockerfile.alpine and keep the original one intact? That way, users can choose.

@iphydf
Copy link
Member

iphydf commented Apr 12, 2017

Also, squash all the commits.

@iphydf iphydf modified the milestones: v0.1.9, v0.1.8 Apr 12, 2017
@nurupo
Copy link
Member

nurupo commented Apr 13, 2017

Uh, I'm just reminding that I have said that I don't want replacing the Debian Jessie image with the Alpine one, e.g. I'm against this PR.

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2017

CLA assistant check
All committers have signed the CLA.

@iphydf
Copy link
Member

iphydf commented Jun 5, 2017

@nurupo we won't replace it, just add another Dockerfile.alpine with this one in it. @romik-g can you do that?

@iphydf iphydf modified the milestones: 0.1.10, v0.1.9 Jun 5, 2017
@robinlinden robinlinden modified the milestones: v0.1.11, v0.1.10 Aug 5, 2017
@ghost
Copy link
Author

ghost commented Oct 24, 2017

@iphydf tell me how to do that, please. Should I rename file in my repo and create another pull request to this one?

@robinlinden
Copy link
Member

@romik-g I went ahead and did it for you.
@nurupo Can you have a look at the Dockerfile now that it isn't replacing the Debian one?

@nurupo
Copy link
Member

nurupo commented Dec 13, 2017

@nurupo we won't replace it, just add another Dockerfile.alpine with this one in it.

So, who is going to maintain it? I'm committed to fixing the Debian Dockerfile if anything breaks in it, but if Alpine Dockerfile breaks it will either be left broken or will be deleted from the repository due to being broken.

@robinlinden robinlinden removed this from the v0.1.11 milestone Dec 17, 2017
@iphydf iphydf added this to the v0.2.0 milestone Jan 6, 2018
@iphydf
Copy link
Member

iphydf commented Jan 13, 2018

@nurupo it'll be left broken or deleted. If anyone cares about it, they will step up and fix it.

@iphydf iphydf changed the title Replace debian:jessie with alpine linux in Dockerfile Add Alpine linux Dockerfile in addition to the existing Debian one Jan 13, 2018
@iphydf iphydf modified the milestones: v0.2.0-RC1, v0.2.0 Jan 14, 2018
@iphydf
Copy link
Member

iphydf commented Jan 16, 2018

:lgtm_strong: I'll add a travis job for it to the nightly build later.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@iphydf iphydf merged commit 6fc0be5 into TokTok:master Jan 16, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants