[Filebeat] Check if processor is supported by ES version #23763

P1llus · 2021-01-29T15:30:15Z

What does this PR do?

A new function to make it easy to set minimum versions for processors. This allows modules to have pipelines that includes newer processors, while having a fallback if the ES version is too old.

Compared to existing functions which checks for specific options inside a processor, this checks for processors as a whole.

This PR is needed to close #22768 .

Why is it important?

Allows Filebeat modules to use newer ingest pipeline processors while not having to worry about backwards compatibility with the processor itself.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
~~- [ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.~~

elasticmachine · 2021-01-29T15:30:17Z

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

elasticmachine · 2021-01-29T15:30:17Z

Pinging @elastic/integrations (Team:Integrations)

filebeat/fileset/pipelines.go

andrewkroh

Is there a good reason to put this validation into beats? Or said another way, why not rely on the errors coming back from Elasticsearch?

If we cannot do anything to fix the errors then I don't see a lot value from maintaining a list of the supported ES versions for particular processors.

P1llus · 2021-01-29T16:03:25Z

Is there a good reason to put this validation into beats? Or said another way, why not rely on the errors coming back from Elasticsearch?

If we cannot do anything to fix the errors then I don't see a lot value from maintaining a list of the supported ES versions for particular processors.

It's because the beat will fail to install the pipeline on older versions if a single processor does not exist on that ES version. The reason this is implemented is to instead of failing it will remove the unsupported processors to allow the pipelines to be installed.
The pipeline itself then needs a fallback plan, that checks if the values usually populated by that processor is not filled, and if not then it will do it in a way that is supported by older versions.

I don't see any other place we can add this check than beats @andrewkroh ?

andrewkroh · 2021-01-29T16:06:07Z

I got confused by the method name, sorry, I skimmed it and thought it was just returning an error ("check" which is passive) whereas it's filtering incompatible processors.

P1llus · 2021-01-29T16:07:11Z

I got confused by the method name, sorry, I skimmed it and thought it was just returning an error ("check") whereas it's filtering incompatible processors.

No worries!

The idea is to iterate over the list of processors and remove the processors that needs a version higher than the current ES version. Since this is a slice I would need to create a new list of processors and overwrite the current one with the new one (called newProcessors)

elasticmachine · 2021-01-29T16:20:30Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #23763 updated
Start Time: 2021-02-14T14:26:47.118+0000
Duration: 48 min 44 sec
Commit: 68f14b7

Test stats 🧪

Test	Results
Failed	0
Passed	13033
Skipped	2033
Total	15066

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	13033
Skipped	2033
Total	15066

andrewkroh · 2021-01-29T16:23:29Z

It makes sense for processors that can degrade gracefully without affecting the rest of the pipeline. But the result could be confusing:

Developers must be aware that this feature exists because they cannot write follow-on processors that depend on the output of the removed processor.
If you upgrade ES after installing the pipeline it will not automatically upgrade the pipeline definition.

So I'm a little worried that feature like this could make working with pipelines and debugging problems harder. Having a hard failure based on ES version might be simpler to reason about because it's an invariant. But of course this has trade-offs with out-of-box usability.

P1llus · 2021-01-29T16:28:57Z

It makes sense for processors that can degrade gracefully without affecting the rest of the pipeline. But the result could be confusing:

Developers must be aware that this feature exists because they cannot write follow-on processors that depend on the output of the removed processor.

If you upgrade ES after installing the pipeline it will not automatically upgrade the pipeline definition.

So I'm a little worried that feature like this could make working with pipelines and debugging problems harder. Having a hard failure based on ES version might be simpler to reason about because it's an invariant. But of course this has trade-offs with out-of-box usability.

I think that the pipelines that wants to use new processors should only do so if the pipelines can fully work without them. In the case of uri_parts processor there should be a simple fallback to handle urls as we would usually do in the older versions.

The points you mention above is certainly valid, but we are already doing this for processor arguments/options in the same file that this PR modifies, and I feel that we either have to not use any new processors before 8.x or use something like this as a middleground.

Users that are upgrading ES/beats would have needed to reinstall their pipeline to get updates either way right?

elasticmachine · 2021-01-29T20:40:59Z

Pinging @elastic/agent (Team:Agent)

filebeat/fileset/pipelines.go

These should have been require.NoError otherwise panics occur.

Errors should not begin with capitals. Avoid double logging errors (both logging and returning them). Fix usage of deprecated logp functions. Remove unused code (ML module). Removed unneeded import alias. Avoid allocating empty slices. Add missing error check for config Unpack.

* adding possibility to remove processor if its unsupported * removing unused function while testing * was initially testing with a separate function, removed that for now * moving it to its own function * rewriting SetProcessors logic to be more extendable * added testcases for uri parts * stashing changes * Fix assert.NoError usages These should have been require.NoError otherwise panics occur. * Package cleanup Errors should not begin with capitals. Avoid double logging errors (both logging and returning them). Fix usage of deprecated logp functions. Remove unused code (ML module). Removed unneeded import alias. Avoid allocating empty slices. Add missing error check for config Unpack. * Add changelog Co-authored-by: Andrew Kroh <[email protected]> (cherry picked from commit 9dbfd44)

… by ES version (#24044) * [Filebeat] Check if processor is supported by ES version (#23763) * adding possibility to remove processor if its unsupported * removing unused function while testing * was initially testing with a separate function, removed that for now * moving it to its own function * rewriting SetProcessors logic to be more extendable * added testcases for uri parts * stashing changes * Fix assert.NoError usages These should have been require.NoError otherwise panics occur. * Package cleanup Errors should not begin with capitals. Avoid double logging errors (both logging and returning them). Fix usage of deprecated logp functions. Remove unused code (ML module). Removed unneeded import alias. Avoid allocating empty slices. Add missing error check for config Unpack. * Add changelog Co-authored-by: Andrew Kroh <[email protected]> (cherry picked from commit 9dbfd44) * Update CHANGELOG.next.asciidoc * adding missing variable declaration and mage fmt update

…-arm * upstream/master: [Metricbeat][Kubernetes] Extend state_node with more conditions (elastic#23905) [CI] googleStorageUploadExt step (elastic#24048) Check fields are documented for aws metricsets (elastic#23887) Update go-concert to 0.1.0 (elastic#23770) [Libbeat][New Processor] XML Decode (elastic#23678) Fix: bad substitution of API key (elastic#24036) [Filebeat] Add Pensando DFW Module (elastic#21063) [Filebeat] Check if processor is supported by ES version (elastic#23763) Syslog system tests: be more forgiving (elastic#24021)

P1llus added 4 commits January 29, 2021 16:14

adding possibility to remove processor if its unsupported

921b115

removing unused function while testing

b924adb

was initially testing with a separate function, removed that for now

4eae0d4

moving it to its own function

a51985f

P1llus added enhancement Filebeat Filebeat Team:Integrations Label for the Integrations team Team:Security-External Integrations labels Jan 29, 2021

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jan 29, 2021

marc-gr reviewed Jan 29, 2021

View reviewed changes

filebeat/fileset/pipelines.go Outdated Show resolved Hide resolved

andrewkroh reviewed Jan 29, 2021

View reviewed changes

andresrc added the Team:Elastic-Agent Label for the Agent team label Jan 29, 2021

P1llus added 4 commits February 9, 2021 11:36

rewriting SetProcessors logic to be more extendable

e8cfd13

added testcases for uri parts

04a2b07

Merge branch 'master' into filebeat_check_if_processor_is_supported

c46e685

Merge branch 'master' into filebeat_check_if_processor_is_supported

df9f156

andrewkroh reviewed Feb 10, 2021

View reviewed changes

P1llus and others added 3 commits February 11, 2021 17:34

stashing changes

e26a9d1

Fix assert.NoError usages

9a89fa9

These should have been require.NoError otherwise panics occur.

Add changelog

68f14b7

andrewkroh approved these changes Feb 14, 2021

View reviewed changes

marc-gr approved these changes Feb 15, 2021

View reviewed changes

P1llus merged commit 9dbfd44 into elastic:master Feb 15, 2021

P1llus mentioned this pull request Feb 15, 2021

Cherry-pick #23763 to 7.x: [Filebeat] Check if processor is supported by ES version #24044

Merged

5 tasks

P1llus added the v7.12.0 label Feb 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Filebeat] Check if processor is supported by ES version #23763

[Filebeat] Check if processor is supported by ES version #23763

P1llus commented Jan 29, 2021 •

edited

Loading

elasticmachine commented Jan 29, 2021

elasticmachine commented Jan 29, 2021

andrewkroh left a comment

P1llus commented Jan 29, 2021

andrewkroh commented Jan 29, 2021 •

edited

Loading

P1llus commented Jan 29, 2021

elasticmachine commented Jan 29, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

andrewkroh commented Jan 29, 2021

P1llus commented Jan 29, 2021

elasticmachine commented Jan 29, 2021

[Filebeat] Check if processor is supported by ES version #23763

[Filebeat] Check if processor is supported by ES version #23763

Conversation

P1llus commented Jan 29, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

elasticmachine commented Jan 29, 2021

elasticmachine commented Jan 29, 2021

andrewkroh left a comment

Choose a reason for hiding this comment

P1llus commented Jan 29, 2021

andrewkroh commented Jan 29, 2021 • edited Loading

P1llus commented Jan 29, 2021

elasticmachine commented Jan 29, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

andrewkroh commented Jan 29, 2021

P1llus commented Jan 29, 2021

elasticmachine commented Jan 29, 2021

P1llus commented Jan 29, 2021 •

edited

Loading

andrewkroh commented Jan 29, 2021 •

edited

Loading

elasticmachine commented Jan 29, 2021 •

edited by jenkins-beats-ci bot

Loading