Skip to content

Make the Docker build more re-usable in Cloud#50277

Merged
pugnascotia merged 13 commits intoelastic:masterfrom
pugnascotia:49926-docker-changes-for-ess
Jan 23, 2020
Merged

Make the Docker build more re-usable in Cloud#50277
pugnascotia merged 13 commits intoelastic:masterfrom
pugnascotia:49926-docker-changes-for-ess

Conversation

@pugnascotia
Copy link
Copy Markdown
Contributor

@pugnascotia pugnascotia commented Dec 17, 2019

Closes #49926. Closes #46166.

  • Add a mini-init process
  • Don't use root at all when running the container
  • Ensure no files in the image have the setuid flag

Also improve dependency tracking in the build.

@pugnascotia
Copy link
Copy Markdown
Contributor Author

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.
@pugnascotia pugnascotia force-pushed the 49926-docker-changes-for-ess branch from fac91ba to 91a1ac1 Compare January 7, 2020 11:10
@pugnascotia
Copy link
Copy Markdown
Contributor Author

pugnascotia commented Jan 7, 2020

@jordansissel I've replaced the my_init script with tini, and removed a lot of the dancing around that the entrypoint was doing.

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.

@pugnascotia pugnascotia changed the title WIP - Make the Docker build more re-usable in Cloud Make the Docker build more re-usable in Cloud Jan 7, 2020
@pugnascotia pugnascotia requested a review from dliappis January 7, 2020 11:17
@pugnascotia pugnascotia added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 labels Jan 7, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Packaging)

@pugnascotia pugnascotia marked this pull request as ready for review January 7, 2020 11:18
@pugnascotia
Copy link
Copy Markdown
Contributor Author

@elasticmachine update branch

Copy link
Copy Markdown
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pugnascotia
Copy link
Copy Markdown
Contributor Author

Aye, I went digging for that exact information before 👍

Copy link
Copy Markdown
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pugnascotia pugnascotia merged commit 8a6d68b into elastic:master Jan 23, 2020
@pugnascotia pugnascotia deleted the 49926-docker-changes-for-ess branch January 23, 2020 10:58
@pugnascotia pugnascotia removed the request for review from jordansissel January 23, 2020 11:02
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Feb 4, 2020
This is to mitigate "stackclash" attacks. This is a a very small partial
backport from elastic#50277.
tylersmalley added a commit to tylersmalley/kibana that referenced this pull request Feb 5, 2020
Based on elastic/elasticsearch#50277
Related to elastic#52450

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
pugnascotia added a commit that referenced this pull request Feb 13, 2020
This is to mitigate "stackclash" attacks. This is a a very small partial
backport from #50277.
pugnascotia added a commit that referenced this pull request Apr 24, 2020
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.
pugnascotia added a commit that referenced this pull request Apr 24, 2020
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.
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Jul 13, 2022
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.
pugnascotia added a commit that referenced this pull request Jul 14, 2022
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.
pugnascotia added a commit that referenced this pull request Jul 14, 2022
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.
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Jul 14, 2022
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.
pugnascotia added a commit that referenced this pull request Jul 18, 2022
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.
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Jul 18, 2022
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.
pugnascotia added a commit that referenced this pull request Jul 19, 2022
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.
pugnascotia added a commit that referenced this pull request Jul 19, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >enhancement Team:Delivery Meta label for Delivery team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make Docker images suitable for ESS/ECE Pin USER in Dockerfile complying with Docker best practices

7 participants