A few suggested improvements

[yosifkit]

• use the full gpg fingerprint on import
• use --batch on gpg --verify (Fix suggested "gpg" usage to stop relying on deprecated and insecure behavior docker-library/official-images#1420 (comment))
• add entrypoint.sh to the PATH as docker-entrypoint.sh
• use exec so that the shell does not stay resident
• remove dumb-init
  • I might be wrong here and can easily revert this change. From consul documentation under reap, it says that it does "reaping of child processes"; was there another reason to include it? I thought that the resident sh process was getting in the way since you were missing exec (and the output of consul while testing this change seems to confirm that: [DEBUG] Automatically reaping child processes).

Feel free to tell me where I am wrong. 😄

Also, I'd like to ask about the semi-complicated dev, client, server thing. Is this what consul users would be used to seeing when running it outside of docker? There doesn't seem to be much difference in the provided config file. Would it be easier to allow a RETRY_JOIN space separated list to be passed in and use that to determine whether or not this is the initial bootstrap node or just tell users to run the server with something like docker run -d consul -retry-join 'dns-name' -retry-join 'dns2-name'

One more point: for CONSUL_BIND why not just default to 0.0.0.0? (I'm trying to understand the use case where a user would know the interface device name ahead of time, since I am not sure how deterministic joining two docker networks is.)

[slackpad]

Hi @yosifkit thanks for the feedback!

remove dumb-init

We actually want to take reaping out of Consul itself. Since Consul can manage subprocesses as part of health checks and watches, it ended up being kind of complicated to coordinate the child process reaper with these ongoing "wait for the child and get the exit code" operations. Since we did this for Docker, once we have a good image that handles reaping we are going to deprecate reaping in Consul :-)

add entrypoint.sh to the PATH as docker-entrypoint.sh

This looks like a good change. We should probably rename the file in here as "docker-entrypoint.sh" as well so it's super obvious where it comes from.

use exec so that the shell does not stay resident

👍 - can't believe I missed that.

Also, I'd like to ask about the semi-complicated dev, client, server thing. Is this what consul users would be used to seeing when running it outside of docker?

Other packagers of Consul have opted to make different client and server images, so this seemed like an improvement on that. Thinking about this, though, we do have a feature teed up in Consul 0.7 that will set that default config correctly based on client/server mode. It might be cleaner to gut those modes. Can I set multiple arguments for CMD ["dev"], something like CMD ["agent", "-dev"] - that might be all we need. For the pre-0.7 version I could just document the recommended server config using CONSUL_LOCAL_CONFIG.

One more point: for CONSUL_BIND why not just default to 0.0.0.0?

It's going to be pretty common to use --net=host with Consul, so that wouldn't be a safe default. The interface name thing came from the community and I think it's a pretty common use case to want to bind to the docker0 overlay network without knowing your IP ahead of time. We will eventually have an interface name option native as a feature of Consul so the little script bit will go away here once that's available.

[slackpad]

@yosifkit I pushed up #11 with your changes plus some simplification of the entrypoint. This seems easier to use and matches better with the container being a stand-in for the Consul binary, since now you can do docker run consul agent -server. Please take a look at that and let me know what you think.