Graylog2 / graylog-docker

Official Graylog Docker image
https://hub.docker.com/r/graylog/graylog/
Apache License 2.0
367 stars 133 forks source link

fix: ensure termination signals are forwarded to java process #88

Closed knksmith57 closed 4 years ago

knksmith57 commented 5 years ago

Add Tini and use it as the default entrypoint. Also, add gnupg2 dependency to downloader layer to enable gpg verification of Tini executable before copying to final layer.

Fixes #87

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

knksmith57 commented 5 years ago

hm, not sure what's going on with CI build; looks like maybe some env vars aren't properly configured in Travis?

$ git clone https://github.com/Graylog2/graylog-docker.git graylog-docker \
  && cd $_
Cloning into 'graylog-docker'...
remote: Enumerating objects: 25, done.
remote: Counting objects: 100% (25/25), done.
remote: Compressing objects: 100% (16/16), done.
remote: Total 506 (delta 9), reused 18 (delta 3), pack-reused 481
Receiving objects: 100% (506/506), 154.99 KiB | 1.57 MiB/s, done.
Resolving deltas: 100% (259/259), done.

$ git fetch origin pull/88/head:pr-88
remote: Enumerating objects: 3, done.
remote: Counting objects: 100% (3/3), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 3 (delta 0), reused 1 (delta 0), pack-reused 0
Unpacking objects: 100% (3/3), done.
From https://github.com/Graylog2/graylog-docker
 * [new ref]         refs/pull/88/head -> pr-88

$ git checkout pr-88
Switched to branch 'pr-88'

$ make &>/dev/null

$ echo $?
0
tklovett commented 5 years ago

Any update on this from Graylog maintainers? We'd love to have this fix merged in.

On Mon, Aug 12, 2019, 11:07 AM Kyle Smith notifications@github.com wrote:

@knksmith57 commented on this pull request.

In Dockerfile https://github.com/Graylog2/graylog-docker/pull/88#discussion_r313004060 :

@@ -64,6 +77,8 @@ ARG GRAYLOG_UID=1100

ARG GRAYLOG_GROUP=graylog

ARG GRAYLOG_GID=1100

+COPY --from=graylog-downloader /opt/tini /tini

good call-out 👌; I thought about this end ended up intentionally making /tini the location in the final layer because that's the location in the Tini docs https://github.com/krallin/tini#using-tini and also the one I see most often in the wild.

I use /opt/tini in the downloader layer to conform with the existing convention of using /opt/ for downloaded executables.

I'm 100% open to any location and happy to update to accommodate --just took my best shot in the dark to remain idiomatic 😄

good feedback!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Graylog2/graylog-docker/pull/88?email_source=notifications&email_token=AAJCXQI7FRGIXSFMU7GB2BDQEGDC7A5CNFSM4IKYCO52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBI4REI#discussion_r313004060, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJCXQMQAYDHEOVRESQH45LQEGDC7ANCNFSM4IKYCO5Q .

knksmith57 commented 5 years ago

definitely let me know if there are any changes you'd like made --I'm happy to do so. Thanks!

bernd commented 4 years ago

@mehrenreich @mariussturm @jalogisch Can one of you take a look at this? Thank you! :smiley:

knksmith57 commented 4 years ago

thanks for taking a look, @jalogisch!

as it turns out, tini is available directly via apt-get :sweat_smile:. this is breaking news to me and very exciting.

I just pushed up a refactor to install from there instead of using the brittle and error prone curl + copy strategy originally proposed.

@jalogisch, I think this addresses all of your outstanding concerns but one: whether or not tini is safe to pull in. IMO, to the extent that it's safe to blindly download and run any software from a stranger on the internet, we should be fine with this one as it's already bundled with docker itself. happy to discuss more though if you feel it appropriate!

Please let me know if there's anything else you'd like addressed!

knksmith57 commented 4 years ago

also, for posterity:

if you'd like to build + run locally, you can lift the example docker-compose.yml from the docs then build + tag the local image as graylog/graylog:3.1 before spinning up the stack:

$ make &>/dev/null \
    && docker tag graylog graylog/graylog:3.1

$ docker-compose up
knksmith57 commented 4 years ago

Do you have that already running in your environment with that change?

I verified this change by standing up a local stack and stopping the containers using various techniques; however, I don't have a durable environment where graylog is constantly running.

I do, however, have countless apps spanning many programming languages and base images running in production environments via tini (as does much of the docker-running world).

If there's anything I can do to help assist with the QA process, please don't hesitate to ask!

Thanks! 😄

jalogisch commented 4 years ago

I have tested this build for some while in my environment and now just pushed it. Will start a new docker image build once my other changes are in that too.

jalogisch commented 4 years ago

thank you @knksmith57