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

Tolerate failure #183

Merged
merged 7 commits into from
Jan 7, 2025
Merged

Tolerate failure #183

merged 7 commits into from
Jan 7, 2025

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Oct 20, 2022

When an Event cannot be created directly from the hit, or when the
docinfo cannot be merged into a non-hash field in the hit, emit an
Event tagged with _elasticsearch_input_failure that contains the
JSON-encoded hit in [event][original] instead of crashing.

Resolves #182


Common causes are:

- When the hit result contains top-level fields that are {logstash-ref}/processing.html#reserved-fields[reserved in Logstash] but do not have the expected shape. Use the <<plugins-{type}s-{plugin}-target>> directive to avoid conflicts with the top-level namespace.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenzone can I get 👀 on this one to make sure I'm not risking breaking the docs build?

It looks like we added {logstash-ref}/processing.html#reserved-fields in 7.7 (it doesn't exist on 7.6 and before), but is present on all of the versions of the reference available in the quick picker (master, current, 8.5, 8.4, 7.17).

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we added {logstash-ref}/processing.html#reserved-fields in 7.7 (it doesn't exist on 7.6 and before), but is present on all of the versions of the reference available in the quick picker (master, current, 8.5, 8.4, 7.17).

The gemlock release file controls which plugin versions get mapped/picked up for each stack version when we run docgen. For that reason, the plugin<->stack versioning generally works itself out without any manual intervention from us.

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Left some minor wording tweaks inline for your consideration and addressed versioning question. Otherwise, LGTM

TL;DR. The versioning should work itself out and your linking in spot on.


Common causes are:

- When the hit result contains top-level fields that are {logstash-ref}/processing.html#reserved-fields[reserved in Logstash] but do not have the expected shape. Use the <<plugins-{type}s-{plugin}-target>> directive to avoid conflicts with the top-level namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we added {logstash-ref}/processing.html#reserved-fields in 7.7 (it doesn't exist on 7.6 and before), but is present on all of the versions of the reference available in the quick picker (master, current, 8.5, 8.4, 7.17).

The gemlock release file controls which plugin versions get mapped/picked up for each stack version when we run docgen. For that reason, the plugin<->stack versioning generally works itself out without any manual intervention from us.

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Signing off on the docs portion in case you're waiting for an approval to move forward.

When SECURE_INTEGRATION is speicified, the (non-secure) `:integration` specs
are excluded, so we cannot have the `:secure_integration` specs wrapped in a
context flagged as `:integration`.
In order for the `ca_trusted_fingerprint` specs to work with the CA's
fingerprint, ES needs to be configured to present a cert chain that
includes the CA.
@yaauie yaauie changed the base branch from main to 4.x December 26, 2024 22:50
@yaauie yaauie closed this Dec 26, 2024
@yaauie yaauie reopened this Dec 26, 2024
When an Event cannot be created directly from the hit, or when the
docinfo cannot be merged into a non-hash field in the hit, emit an
Event tagged with `_elasticsearch_input_failure` that contains the
JSON-encoded hit in `[event][original]` instead of crashing.
Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

Test cleanup looks good (one small suggestion on cert generation script).

Release prep looks correct (version number is good and changelog looks accurate)

Had a couple nit comments and a question about potential unused code. Other than that this looks like a good improvement.

@yaauie yaauie requested a review from donoghuc December 31, 2024 00:46
Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

LGTM. Ready release and forward port to main branch.

@yaauie yaauie merged commit 4380219 into logstash-plugins:4.x Jan 7, 2025
2 checks passed
yaauie added a commit to yaauie/logstash-input-elasticsearch that referenced this pull request Jan 7, 2025
* test setup: ensure presence of /etc/protocols

* test setup: actually run secure_integration tests

When SECURE_INTEGRATION is speicified, the (non-secure) `:integration` specs
are excluded, so we cannot have the `:secure_integration` specs wrapped in a
context flagged as `:integration`.

* test setup: regnerate test certs (and add regen script)

* test setup: give ES the full cert chain

In order for the `ca_trusted_fingerprint` specs to work with the CA's
fingerprint, ES needs to be configured to present a cert chain that
includes the CA.

* resilience: prevent failures from crashing plugin

When an Event cannot be created directly from the hit, or when the
docinfo cannot be merged into a non-hash field in the hit, emit an
Event tagged with `_elasticsearch_input_failure` that contains the
JSON-encoded hit in `[event][original]` instead of crashing.

* add link to changelog

* remove orphan method from refactor
@yaauie yaauie mentioned this pull request Jan 7, 2025
yaauie added a commit that referenced this pull request Jan 8, 2025
* Tolerate failure (#183)

* test setup: ensure presence of /etc/protocols

* test setup: actually run secure_integration tests

When SECURE_INTEGRATION is speicified, the (non-secure) `:integration` specs
are excluded, so we cannot have the `:secure_integration` specs wrapped in a
context flagged as `:integration`.

* test setup: regnerate test certs (and add regen script)

* test setup: give ES the full cert chain

In order for the `ca_trusted_fingerprint` specs to work with the CA's
fingerprint, ES needs to be configured to present a cert chain that
includes the CA.

* resilience: prevent failures from crashing plugin

When an Event cannot be created directly from the hit, or when the
docinfo cannot be merged into a non-hash field in the hit, emit an
Event tagged with `_elasticsearch_input_failure` that contains the
JSON-encoded hit in `[event][original]` instead of crashing.

* add link to changelog

* remove orphan method from refactor

* add link to PR in CHANGELOG.md
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.

Failure to create an event from the payload can crash the plugin
4 participants