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

give multiple pipelines all the settings #11076

Closed
wants to merge 1 commit into from

Conversation

jsvd
Copy link
Member

@jsvd jsvd commented Aug 23, 2019

Previously, when creating pipelines from pipelines.yml,
we'd only give a pipeline the settings related to pipelines.
The PipelineSettings class was used for this.
However a pipeline may need other settings like the keystore location.

For this we instead clone the settings object and merge all the pipeline
specific settings. This is accomplished with a new method that ensures
that only pipeline level settings are overwritten in the clone.

To check that this patch works:

  1. move the config/ folder to /tmp/
  2. create a keystore: bin/logstash-keystore --path.settings /tmp/config create
  3. add a secret: bin/logstash-keystore --path.settings /tmp/config add secret
  4. use that secret in a pipelines.yml config (e.g. pipelines.yml file):
- pipeline.id: test
  pipeline.workers: 1
  pipeline.batch.size: 1
  config.string: 'input { generator { message => "${secret}" } } filter { sleep { time => 1 } } output { stdout { } }'
  1. run logstash with bin/logstash --path.settings /tmp/config
  2. observe that the string doesn't get interpolated in the generator messages
  3. apply this patch
  4. restart logstash and see the secret :D

Previously we'd only give a pipeline the settings related to pipelines
The PipelineSettings class was used for this.
However a pipeline may need other settings like the keystore location.

For this we instead clone the settings object and merge all the pipeline
specific settings. This is accomplished with a new method that ensures
that only pipeline level settings are overwritten in the clone.
@jsvd jsvd force-pushed the remove_pipeline_setttings_class branch from 94aa7cf to 86e0bac Compare August 26, 2019 15:53
@danhermann danhermann self-requested a review August 26, 2019 22:05
Copy link
Contributor

@danhermann danhermann left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@danhermann
Copy link
Contributor

danhermann commented Aug 27, 2019

Note that this resolves the root cause of #11055.

@robbavey
Copy link
Member

@jsvd Does that mean that this statement that keystore is not supported when using pipeline.yml can be removed? https://github.com/elastic/logstash/blame/master/docs/static/keystore.asciidoc#L30

@jsvd
Copy link
Member Author

jsvd commented Aug 30, 2019

@jsvd Does that mean that this statement that keystore is not supported when using pipeline.yml can be removed? https://github.com/elastic/logstash/blame/master/docs/static/keystore.asciidoc#L30

For keystore/env vars to work on pipelines.yml we need #11081

@elasticsearch-bot
Copy link

Joao Duarte merged this into the following branches!

Branch Commits
master 47fc8e7
7.x cd91f9b
7.4 7488ca7
7.3 1032671

elasticsearch-bot pushed a commit that referenced this pull request Aug 30, 2019
Previously we'd only give a pipeline the settings related to pipelines
The PipelineSettings class was used for this.
However a pipeline may need other settings like the keystore location.

For this we instead clone the settings object and merge all the pipeline
specific settings. This is accomplished with a new method that ensures
that only pipeline level settings are overwritten in the clone.

Fixes #11076
elasticsearch-bot pushed a commit that referenced this pull request Aug 30, 2019
Previously we'd only give a pipeline the settings related to pipelines
The PipelineSettings class was used for this.
However a pipeline may need other settings like the keystore location.

For this we instead clone the settings object and merge all the pipeline
specific settings. This is accomplished with a new method that ensures
that only pipeline level settings are overwritten in the clone.

Fixes #11076
elasticsearch-bot pushed a commit that referenced this pull request Aug 30, 2019
Previously we'd only give a pipeline the settings related to pipelines
The PipelineSettings class was used for this.
However a pipeline may need other settings like the keystore location.

For this we instead clone the settings object and merge all the pipeline
specific settings. This is accomplished with a new method that ensures
that only pipeline level settings are overwritten in the clone.

Fixes #11076
@jsvd jsvd deleted the remove_pipeline_setttings_class branch August 30, 2019 11:21
andsel pushed a commit to andsel/logstash that referenced this pull request Sep 12, 2019
Previously we'd only give a pipeline the settings related to pipelines
The PipelineSettings class was used for this.
However a pipeline may need other settings like the keystore location.

For this we instead clone the settings object and merge all the pipeline
specific settings. This is accomplished with a new method that ensures
that only pipeline level settings are overwritten in the clone.

Fixes elastic#11076
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants