Make the Docker build more re-usable in Cloud#50277
Make the Docker build more re-usable in Cloud#50277pugnascotia merged 13 commits intoelastic:masterfrom
Conversation
|
cc @mieciu |
Closes elastic#49926 and elastic#46166. * Add a mini-init process (copied from Cloud) * Don't use root at all when running the container * Add an explicity TERM handler * Ensure no files in the image have the setuid flag Also improve dependency tracking in the build.
Remove the my_init script in favour of tini, which meant that docker-entrypoint.sh could be cleaned up significantly.
fac91ba to
91a1ac1
Compare
|
@jordansissel I've replaced the Regarding SIGCHLD, I believe I'm correct in saying that ES treats unexpected termination of ML nodes as a problem, and doesn't restart them. IOW, ES doesn't expect to have to do any child reaping in the normal course of things. It might be worth a conversation with the ML team, my gut feeling is that it's perhaps orthogonal to this change? I still need to make docs changes if we're broadly OK with these changes. |
|
Pinging @elastic/es-core-infra (:Core/Infra/Packaging) |
|
@elasticmachine update branch |
dliappis
left a comment
There was a problem hiding this comment.
Thank you!
This is wonderful work and cleans things up so much.
I asked for a change because we need to update the docs to reflect the removal of TAKE_FILE_OWNERSHIP.
Additionally I think @droberts195 needs to review this to verify that the order of killing is correct (as discussed in #49926 (comment))
droberts195
left a comment
There was a problem hiding this comment.
From https://github.com/krallin/tini#process-group-killing I see that:
By default, Tini only kills its immediate child process.
So LGTM from the ML perspective.
|
Aye, I went digging for that exact information before 👍 |
dliappis
left a comment
There was a problem hiding this comment.
LGTM.
I mulled over whether we should mark the removal of TAKE_FILE_OWNERSHIP as a breaking change (reference/migration/migrate_8_0.asciidoc) but it really was meant to be used as a last resort as it mutated the bind mount permissions from within the container, so possibly not a big deal.
This is to mitigate "stackclash" attacks. This is a a very small partial backport from elastic#50277.
Based on elastic/elasticsearch#50277 Related to elastic#52450 Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
This is to mitigate "stackclash" attacks. This is a a very small partial backport from #50277.
Firstly, backport the use of tini as the Docker entrypoint. This was supposed to have been done following #50277, but was missed. It isn't a direct backport as this branch will continue using root as the initial Docker user. Secondly, backport #55491 to use the official checksums when downloading tini.
Firstly, backport the use of tini as the Docker entrypoint. This was supposed to have been done following #50277, but was missed. It isn't a direct backport as this branch will continue using root as the initial Docker user. Secondly, backport #55491 to use the official checksums when downloading tini.
As part of elastic#50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
As part of #50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
As part of #50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
As part of elastic#50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
Backport of #88502. As part of #50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
As part of elastic#50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
As part of #50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
As part of #50277, we removed the `TAKE_FILE_OWNERSHIP` option from the Docker entrypoint script and the associated chroot calls, and instead just defaulted to running the image as `elasticsearch` instead of `root`. However, we didn't check that it was still possible to pass CLI options to Elasticsearch via CLI arguments, and broke this by mistake. This is probably an uncommon pattern, versus environment variables or a config file. Nevertheless, it is supposed to be possible and is mentioned in the documentation. Fix the problem by suppling the missing positional params when calling Elasticsearch, and add a test case so that we don't break it again.
Closes #49926. Closes #46166.
Also improve dependency tracking in the build.