DiUS / pact_broker-docker

'Dockerised' pact broker
http://pact.io
MIT License
76 stars 102 forks source link

New Dockerfile with user mode #87

Closed tavogel closed 5 years ago

tavogel commented 5 years ago

I've come up with an alternate approach to solving the user-mode issue than https://github.com/DiUS/pact_broker-docker/pull/84. After getting increasingly frustrated sorting out user mode in the phusion base image, I wrote a new Dockerfile that uses Passenger Standalone from a standard Ubuntu base image. In this case there are a couple of advantages... primarily it's less than half the size (478mb vs 1.07gb), ditches a ton of cruft, and no need for chmod/chgrp/chown shenanigans, or weird initializers. It also logs to stdout rather than a file, so no need for logrotate, and no worries about filling up disk space... just let your container platform handle it.

bethesque commented 5 years ago

Thanks for working on this. It looks like you've made some big improvements. I have a few questions before this can be merged.

  1. What will happen to existing users who pull this image in and expect the broker to be exposed on port 80? I will not accept any merge request that breaks backwards compatibility. Can we make this configurable (perhaps using dockerize)?
  2. What things were in the cruft that we no longer have?
  3. Where is the equivalent of the pactbroker-env.conf and webapp.conf, and why don't we need them any more?
  4. Are you running this image yourself already, in a production like setting?

Cheers.

tavogel commented 5 years ago

Hey Beth, thanks for your time & consideration on this!

  1. Ports under 1024 are privileged, and only root users can bind to them. This is going to be an inevitable problem if you are going to run your container as a user, unfortunately (see https://github.com/moby/moby/issues/8460). This is a best practice for security though (principle of least privilege + hardening for container breakout vulnerabilities), and with OpenShift and PodSecurityPolicies picking up steam, you will find fewer containers remaining as root these days. This is where I find myself today, as our Ops team rolls out PSPs; if I want to continue using Pact Broker in our CICD pipelines, I have to convert the containers to user mode, because soon the user-only policy will be enforced across the board for our org.

If people are consuming this package from :latest then it will be a breaking change for them, no matter what (changing 80:80 to 80:3000, at least). It looks like your container versions are marching in lockstep with your application versions, which makes this a bit more awkward. If the container had an independent version, you'd most likely do a major release of the container, or at least a minor. Maybe you could wait for the next major/minor increment of pact broker itself and merge this change along with that. I would not recommend a breaking change like this as a patch release.

At the end of the day you are the maintainer and can do what you are comfortable with. If we need to maintain a fork, for internal use, then we will. But we thought we'd try giving back to the community first!

  1. The "cruft" I speak of is in the Phusion base Docker images. They have an interesting policy towards containers, which aligns differently than your usual "12 factor" apps: https://github.com/phusion/baseimage-docker#whats-inside-the-image. A lot of this is IMO unnecessary, and is a hard blocker for user-mode. Particularly having logging files at all (you should just log to stdout and let your container platform handle the routing), which then requires logrotate (filling up ephemeral disk for what?), which requires cron (which requires root), etc. Also having SSH server when you can just kubectl/docker exec -it <pod/container> -- sh into containers is odd. It's almost like they intend it for folks used to "batteries included" VMs rather than ephemeral containers... which they allude to, but don't necessarily agree with, in their docs. Beyond the unusual hoops you have to jump through, the image is also huge: double the size of standalone! This means twice as long to pull, which means longer to start, which means slower feedback in CICD & longer MTTR, etc.

  2. With the new Dockerfile, I leverage what's called the Passenger "Standalone Mode". You download the Ruby Gem normally, and it can install an nginx binary, along with some opinionated configs that roughly match the pactbroker-env.conf and webapp.conf. By my testing the standalone Passenger is as performant as the standalone nginx. These steps handle the installation of nginx & configs:

    bundle exec passenger-config install-agent && \
    bundle exec passenger-config install-standalone-runtime && \
    bundle exec passenger-config build-native-support

By running bundle exec passenger start Ruby is able to spin up and manage the child nginx process, without the need for an initializer script like the Phusion base image requires, nor the separate nginx installation. This is the same way that other Ruby/Rack web servers work (e.g. Puma/Unicorn). As an aside, using Puma as the server instead of Passenger would enable multithreading (it's an Enterprise-only feature in Passenger), and you could also use an Alpine base container instead of Ubuntu, which is even smaller. I figured that might be too much scope for this PR, but I can propose another version for that if you like.

  1. We aren't using this package in our CICD at scale today. It's certainly something I can look into... we would just need to publish it as a fork, and update our pipelines.

Happy to answer any further questions or help in any way I can. TTFN!

bethesque commented 5 years ago

Thanks for your detailed response @tavogel. I'm going to put this on a branch, and give it a different tag, so we can run the existing one in parallel with yours, without breaking our current users. When we do a major release of the broker (and we've given this image a good testing) then we can merge it in to the main branch.

I'm open to using Puma, but have no experience in using it myself.

bethesque commented 5 years ago

We haven't forgotten this, just been crazy busy.

YOU54F commented 5 years ago

This is a really nice solution @tavogel

I built your dockerfile changes alongside puma and it is currently here

https://github.com/YOU54F/pact-standalone-docker-slim/blob/master/pact_broker_puma/Dockerfile

it is tiny in comparison to the current pact broker docker image, and gives us multithreading, which as you correctly stated, and enterprise level feature of passenger that current PB docker users don't benefit from.

tavogel commented 5 years ago

@bethesque @YOU54F new PR created for Puma: https://github.com/DiUS/pact_broker-docker/pull/90

bethesque commented 5 years ago

Awesome. We'll probably merge the puma one then, once we've gotten a few eyes on it. Are you running it yourself yet? Nothing like someone actually testing in prod to give confidence about big changes!

tavogel commented 5 years ago

Closed in favour of https://github.com/DiUS/pact_broker-docker/pull/90