-
Notifications
You must be signed in to change notification settings - Fork 306
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
Use integration metadata to create ES actions #1155
Use integration metadata to create ES actions #1155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we have a mix of precedences.
The pipeline
and id
seem to work as-specified with explicit plugin configuration always overriding implicit discovery of the event, but index
is reversed and allows implicitly-discovered values to override explicit plugin configuration.
There are also some gaps in the spec coverage, which should cover all of:
index
:- when plugin's
index
is specified- when event metadata contains
index
- plugin's
index
is used
- plugin's
- when event metadata does not contain
index
- plugin's
index
is used
- plugin's
- when event metadata contains
- when plugin's
index
is NOT specified- when event metadata contains
index
- event metadata
index
is used
- event metadata
- when event metadata does not contain
index
- plugin's default
index
mechanism is used (including data streams variants)
- plugin's default
- when event metadata contains
- when plugin's
id
:- when plugin's
document_id
is specified- when event metadata contains
id
- plugin's
document_id
is used
- plugin's
- when event metadata does not contain
id
- plugin's
document_id
is used
- plugin's
- when event metadata contains
- when plugin's
document_id
is NOT specified- when event metadata contains
id
- event metadata
id
is used
- event metadata
- when event metadata does not contain
id
- plugin's default
id
mechanism is used (action tuple excludes anid
)
- plugin's default
- when event metadata contains
- when plugin's
pipeline
:- when plugin's
pipeline
is specified- when event metadata contains
pipeline
- plugin's
pipeline
is used
- plugin's
- when event metadata does not contain
pipeline
- plugin's
pipeline
is used
- plugin's
- when event metadata contains
- when plugin's
pipeline
is NOT specified- when event metadata contains
pipeline
- event metadata
pipeline
is used
- event metadata
- when event metadata does not contain
pipeline
- plugin's default
pipeline
mechanism is used (action tuple excludes apipeline
)
- plugin's default
- when event metadata contains
- when plugin's
@@ -271,6 +271,45 @@ | |||
end | |||
end | |||
|
|||
describe "with event integration metadata" do | |||
context "when there isn't any index setting specified and the event contains an integration metadata index" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From line 192, which appears to be the outer context group for this example context, the options
includes an index
directive.
When an index
is explicitly given in the plugin configuration, it must supercede the metadata.
As-specified, explicit configuration always takes precedence over implicit discovery.
Hi @yaauie, thank's a lot for your review. I've integrated your suggestion and the PR is ready for a second round of review 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks clean and the spec coverage makes the behaviour clear.
We need a changelog entry and a version bump, and should consider whether to document this in the user-facing docs.
Suggestion for the changelog:
- Adds support for propagating event processing metadata when this output is downstream of an Elastic Integration Filter and configured _without_ explicit `index`, `document_id`, or `pipeline` directives [#1155](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1155)
I am not strongly opposed to including a user-facing blurb in the docs, but I don't think it adds much value and could lead to abuse.
d5d8180
to
8d486fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
…a during the creation of id and index for each event
8d486fa
to
583af0e
Compare
… integration metadata and datastream is enabled (logstash-plugins#1161) During PR logstash-plugins#1155 the resolution of version and version_type ES parameters was moved from the index only event action tuple creation to the common method. This changed was due to do the intent to collect all integration-aware metadata fields in one place, but the common method is used also by the datastream part this result in populating event and event_type request parameters not only for normal index operations. During an index operation on a datastream, if one of those parameters is valued, generated an error on ES indexing, resulting in request fail. This PR move the processing and creation of event and event_type parameters, back in its original position, splitting the capture of integration metadata in 2 parts.
Release notes
Use integration metadata to interact with Elasticsearch.
What does this PR do?
Change the creation of actions that are passed down to Elasticsearch to use also the metadata fields set by an integration.
The interested fields are
id
(document_id),index
andpipeline
, the field values are taken verbatim without placeholders resolution.The index, document_id and pipeline that are configured in the plugin settings have precedence on the integration ones because manifest an explicit choice made by the user.
Why is it important/What is the impact to the user?
This PR fixes an interoperability issue with Agent's integrations, where some metadata valued by integration has to be used down to Elasticsearch.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
logstash-outpiut-elasticsearch
./tmp/defender_singleline.json
). To squash all lines in one use:or use the file defender_singleline.json
Gemfile
.bin/logstash -f "/path_to/pipeline.conf"
rm /tmp/defender_sincedb
.ds-logs-m365_defender.incident-ep-
, that only one document is present.This means that despite the 2 distinct runs, the Defender integration that generate an unique id from the Incident fields was correctly executed and used.
The proof can be done by executing the same flow above, with shipped ES output plugin, and verify that the document result duplicated, so no unique document_id is generated by the integration.
Related issues
Use cases
Screenshots
Logs