Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix passing positional args to ES in Docker #88502

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

pugnascotia
Copy link
Contributor

As part of #50277, we removed the TAKE_FILE_OWNERSHIP option from the
Docker entrypoint script in 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. The
impact is apparently minor, since we only just noticed the problem, and
supplying CLI args in this way 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.

This doesn't impact the 7.x series.

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 pugnascotia added >bug :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.4.0 labels Jul 13, 2022
@pugnascotia pugnascotia requested a review from mark-vieira July 13, 2022 11:04
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Jul 13, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@elasticsearchmachine
Copy link
Collaborator

Hi @pugnascotia, I've created a changelog YAML for you.

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Good catch. LGTM.

@pugnascotia pugnascotia merged commit 9f4b32a into elastic:master Jul 14, 2022
@pugnascotia pugnascotia deleted the fix-docker-positional-params branch July 14, 2022 08:12
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
Copy link
Contributor Author

Backported to 8.3 in 3776923.

pugnascotia added a commit that referenced this pull request Jul 14, 2022
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.
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Jul 15, 2022
* upstream/master: (2974 commits)
  Reserved cluster state service (elastic#88527)
  Add transport action immutable state checks (elastic#88491)
  Remove suggest flag from index stats docs (elastic#85479)
  Polling cluster formation state for master-is-stable health indicator (elastic#88397)
  Add test execution guide in yamlRestTest asciidoc (elastic#88490)
  Add troubleshooting guide for corrupt repository (elastic#88391)
  [Transform] Finetune Schedule to be less noisy on retry and retry slower (elastic#88531)
  Updatable API keys - auto-update legacy RDs (elastic#88514)
  Fix typo in TransportForceMergeAction and TransportClearIndicesCacheA… (elastic#88064)
  Fixed NullPointerException on bulk request (elastic#88358)
  Avoid needless index metadata builders during reroute (elastic#88506)
  Set metadata on request in API key noop test (elastic#88507)
  Fix passing positional args to ES in Docker (elastic#88502)
  Improve description for task api detailed param (elastic#88493)
  Support cartesian shape with doc values (elastic#88487)
  Promote usage of Subjects in Authentication class (elastic#88494)
  Add CCx 2.0 feature flag (elastic#88451)
  Reword the watcher 'always' and 'never' condition docs (elastic#86105)
  Simplify azure discovery installation docs (elastic#88404)
  Breakup FIPS CI testing jobs
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
#	x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java
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 that referenced this pull request Jul 18, 2022
pugnascotia added a commit that referenced this pull request Jul 18, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v8.3.3 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants