Filebeat: Have filesets disabled unless explicitly configured #27526

adriansr · 2021-08-20T10:38:41Z

What does this PR do?

This changes fileset loading so that only filesets that are explicitly defined in the configuration are enabled.

Why is it important?

Until now, an enabled module will have all its filesets enabled unless they are explicitly disabled, which makes for a bad user experience for modules that contain a lot of filesets.

This will break existing configurations where a module is enabled without explicitly enabling any fileset. For example the following configuration suggested by our docs:

The following example shows a configuration that runs the nginx,mysql, and system modules:

filebeat.modules:
- module: nginx
- module: mysql
- module: system

This is updated to:

filebeat.modules:
- module: nginx
  access:
  error:
- module: mysql
  slowlog:
- module: system
  auth:

To alert users when a broken configuration like this is detected, the following warning log will be emitted:

2021-08-20T13:10:34.587+0200 INFO beater/filebeat.go:113 Enabled modules/filesets: nginx (), mysql (), system (auth), ()
2021-08-20T13:10:34.587+0200 WARN beater/filebeat.go:124 Module nginx is enabled but has no enabled filesets
2021-08-20T13:10:34.587+0200 WARN beater/filebeat.go:124 Module mysql is enabled but has no enabled filesets

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.

Related issues

Closes #17256
Related to #11943

elasticmachine · 2021-08-20T10:38:43Z

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

elasticmachine · 2021-08-20T10:42:49Z

Pinging @elastic/integrations (Team:Integrations)

elasticmachine · 2021-08-20T11:31:14Z

💚 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

Start Time: 2021-09-13T17:07:41.090+0000
Duration: 103 min 15 sec
Commit: f90f9fa

Test stats 🧪

Test	Results
Failed	0
Passed	15302
Skipped	2314
Total	17616

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	15302
Skipped	2314
Total	17616

jsoriano · 2021-08-20T11:43:05Z

I love this change, but it can be breaking in some cases. Does this change target 8.0, or you propose to backport it for 7.x?

adriansr · 2021-08-20T13:33:41Z

@jsoriano this targets 8.0

adriansr · 2021-08-23T10:10:01Z

/test

aspacca · 2021-08-23T16:31:23Z

@adriansr
what's the plan for the files on modules.d?
they currently contain all the filesets, usually enabled, and all the available var.* to configure the fileset.

kaiyan-sheng · 2021-08-23T19:47:51Z

Thank you @adriansr for working on this change! With this change, do we still require enabled: false for all disabled filesets? Maybe we should add a test with enabled: false as well? For example:

filebeat.modules:
- module: nginx
  access:
    enabled: false
  error:
    enabled: false

adriansr · 2021-08-30T16:44:38Z

@aspacca that's a good question.

I've been going through the default configurations and applied the following:

For heavy modules with a lot of independent filesets (i.e. cisco), made all disabled by default.
For modules with "active" filesets which query an external API and usually require auth configuration, made them disabled by default (i.e. threatintel).
If one fileset needs to be disabled by default, make all the module's filesets disabled.

This results in the following modules having all filesets disabled in their default config:

azure: Queries an external API (1 fileset).
cisco: Heavy module which is unlikely an user will need more than one or two filesets enabled (7 filesets).
fortinet: Heavy module which is unlikely an user will need more than one or two filesets enabled (4 filesets).
gcp: Queries external API (3 filesets).
google_workspace: Queries external API (6 filesets).
googlecloud: Queries external API (3 filesets).
gsuite: Queries external API (6 filesets).
juniper: Heavy module which is unlikely an user will need more than one or two filesets enabled (3 filesets).
microsoft: Queries external API (3 filesets).
misp: Queries external API (1 fileset).
o365: Queries external API (1 fileset).
okta: Queries external API (1 fileset).
snyk: Queries external API (2 filesets).
threatintel: Queries external API (8 filesets).

All other modules keep their filesets enabled by default.

Maybe this is too confusing and we should make everything disabled by default?

/cc @andrewkroh @kaiyan-sheng

aspacca · 2021-09-02T08:51:50Z

@adriansr

I would rather disabled everything by default (unless part of module enabled by default): I see that if users are enabling a module probably they have to configure it and marking the related filesets as enabled as a part of the configuration

That's mainly because an enabled fileset that's not properly configured, while not breaking Filebeat (preventing it to start for example) will probably produces some logs about the misconfiguration that could be confusing for the users: see #27612 for an use case we just found

mergify · 2021-09-03T07:53:20Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fb_disable_filesets upstream/fb_disable_filesets
git merge upstream/master
git push upstream fb_disable_filesets

This changes fileset loading so that only filesets that are explicitly defined in the configuration are enabled. Until now, an enabled module will have all its filesets enabled unless explicitly disabled, which makes for a bad user experience with modules that contain a lot of filesets. Closes elastic#17256

adriansr · 2021-09-06T10:13:14Z

I've removed the changes to the default configuration files and will follow up with a separate PR that changes all modules' configurations to enabled: false.

aspacca · 2021-09-06T14:47:31Z

filebeat/fileset/modules.go

-		}
-
-		// check that no extra filesets are configured
-		for filesetName, fcfg := range mcfg.Filesets {
 			if fcfg.Enabled != nil && !(*fcfg.Enabled) {


due to https://github.com/elastic/beats/pull/27526/files#diff-515d023121b7ec77d64c2a0cfcfe3dc6f9b46e19166e71ac34e5f5cf74fddfe6L74-L76 we could have a case where fcfg.Enabled = nil and the fileset was not skipped.
now fcfc comes from defined filesets: https://github.com/elastic/beats/pull/27526/files#diff-515d023121b7ec77d64c2a0cfcfe3dc6f9b46e19166e71ac34e5f5cf74fddfe6R72, but still it can be the case that enabled: false is not defined.
I'd change this to if fcfg.Enabled == nil || !(*fcfg.Enabled)

what do you think @adriansr ?

This PR maintains the current behavior of the enabled flag: If it's not defined, it defaults to true. So enabled==nil || *enabled is the true check, and enabled!=nil && !(*enabled) is the false check.

got it @adriansr, but what do you think about changing this behaviour?

cc @exekias this is related to what we discussed in #27612

We could address in another PR in case, just opening the discussion

The problem with this change is that it introduces another breaking change, and will potentially break existing configurations for our users. Also it makes configuring filesets from the command line more complicated as it forces users to always include an enabled flag -M mymodule.mydataset.enabled=true.

I understand your concern, it is actually a breaking change.

The problem I see in enabling by default if not explicitly disabled is that we miss the chance to enforce stricter validation on filesets where there is not a default not failing config (see #27612 ).
In the specific case there's no option to either prevent filebeat from starting if we introduce the validation, or let it start with a misconfigured explicitly enabled fileset that will stop as input.

Also what we have as current behaviour is the possibility of filesets starting and stopping because "implicitly" enabled and not properly configured, producing noisy logs that could be confusing.

Hard choice to take, indeed

mergify · 2021-09-10T14:59:10Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fb_disable_filesets upstream/fb_disable_filesets
git merge upstream/master
git push upstream fb_disable_filesets

adriansr · 2021-09-13T17:07:08Z

/test

What does this PR do? Changes the default configuration for Filebeat's filesets to make them disabled by default (enabled: false). Adds a check to the config and update targets of mage to check that default configurations have an explicit disable: > error: in file 'modules.d/checkpoint.yml.disabled': checkpoint module > dataset firewall must be explicitly disabled (needs enabled: false) Why is it important? The previous default of having all filesets enabled, paired with the configuration loader enabling all non-explicitly-disabled filesets (changed in #27526) has been causing trouble for our users for quite some time.

…c#27526) This changes fileset loading so that only filesets that are explicitly defined in the configuration are enabled. Until now, an enabled module will have all its filesets enabled unless explicitly disabled, which makes for a bad user experience with modules that contain a lot of filesets. Closes elastic#17256

…c#27762) What does this PR do? Changes the default configuration for Filebeat's filesets to make them disabled by default (enabled: false). Adds a check to the config and update targets of mage to check that default configurations have an explicit disable: > error: in file 'modules.d/checkpoint.yml.disabled': checkpoint module > dataset firewall must be explicitly disabled (needs enabled: false) Why is it important? The previous default of having all filesets enabled, paired with the configuration loader enabling all non-explicitly-disabled filesets (changed in elastic#27526) has been causing trouble for our users for quite some time.

Since elastic#27526 and elastic#27762, Filebeat will have all filesets disabled by default. To prevent user confusion, a warning message to alert the user of a configured module without any enabled filesets was added. However, due to Filebeat internals, this message will only appear for modules configured from the command-line (--modules flag). This updates the code to ensure it works also for modules configured via modules.d directory and turns the warning into a hard-error that prevents startup.

Since #27526 and #27762, Filebeat will have all filesets disabled by default. To prevent user confusion, a warning message to alert the user of a configured module without any enabled filesets was added. However, due to Filebeat internals, this message will only appear for modules configured from the command-line (--modules flag). This updates the code to ensure it works also for modules configured via modules.d directory and turns the warning into a hard-error that prevents startup.

Since #27526 and #27762, Filebeat will have all filesets disabled by default. To prevent user confusion, a warning message to alert the user of a configured module without any enabled filesets was added. However, due to Filebeat internals, this message will only appear for modules configured from the command-line (--modules flag). This updates the code to ensure it works also for modules configured via modules.d directory and turns the warning into a hard-error that prevents startup. (cherry picked from commit 707ed1d)

Since #27526 and #27762, Filebeat will have all filesets disabled by default. To prevent user confusion, a warning message to alert the user of a configured module without any enabled filesets was added. However, due to Filebeat internals, this message will only appear for modules configured from the command-line (--modules flag). This updates the code to ensure it works also for modules configured via modules.d directory and turns the warning into a hard-error that prevents startup. (cherry picked from commit 707ed1d) Co-authored-by: Adrian Serrano <[email protected]>

adriansr added review Filebeat Filebeat breaking change Team:Security-External Integrations labels Aug 20, 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 Aug 20, 2021

adriansr added the Team:Integrations Label for the Integrations team label Aug 20, 2021

masci requested review from aspacca and kaiyan-sheng August 23, 2021 14:41

adriansr force-pushed the fb_disable_filesets branch from 6c5274a to ef06c44 Compare August 30, 2021 15:59

adriansr requested a review from jsoriano September 1, 2021 16:42

kaiyan-sheng mentioned this pull request Sep 1, 2021

warn log entry and no validation failure when both queue_url and buck… #27612

Merged

6 tasks

andrewkroh mentioned this pull request Sep 2, 2021

All cisco datasets enabled by default cause poor UX #24242

Closed

adriansr force-pushed the fb_disable_filesets branch from 2282954 to 0110d24 Compare September 6, 2021 10:00

adriansr added 5 commits September 6, 2021 12:10

Warning for misconfigured modules

9d6721d

Update docs

ada39e4

Go test: test enabled: false

c408768

Fix NGINX integration test

9e3d264

Fix fileset override via -M flag

6560033

adriansr force-pushed the fb_disable_filesets branch from 0110d24 to 6560033 Compare September 6, 2021 10:10

adriansr mentioned this pull request Sep 6, 2021

Filebeat: Make all filesets disabled in default configuration #27762

Merged

6 tasks

aspacca reviewed Sep 6, 2021

View reviewed changes

aspacca mentioned this pull request Sep 6, 2021

Breaking changes for 8.0 #13911

Closed

19 tasks

Merge branch 'master' into fb_disable_filesets

f90f9fa

jsoriano mentioned this pull request Sep 13, 2021

Filebeat modules merge behaviour does weird things #27889

Closed

aspacca approved these changes Sep 15, 2021

View reviewed changes

adriansr merged commit 4fbd87e into elastic:master Sep 15, 2021

aspacca mentioned this pull request Oct 13, 2021

Add deprecation warning for filebeat disabled modules #28393

Merged

4 tasks

aspacca mentioned this pull request Oct 27, 2021

not setting queue_url bucket_arn or non_aws_bucket_name returns error #28666

Merged

2 tasks

adriansr mentioned this pull request Nov 4, 2021

Filebeat: Error on startup for unconfigured module #28818

Merged

2 tasks

tetianakravchenko mentioned this pull request Feb 1, 2022

Multiple filesets using hints autodiscovery #30132

Closed

faec mentioned this pull request Apr 4, 2022

Inconsistencies on filebeat modules configuration load #11943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filebeat: Have filesets disabled unless explicitly configured #27526

Filebeat: Have filesets disabled unless explicitly configured #27526

adriansr commented Aug 20, 2021 •

edited by jsoriano

Loading

elasticmachine commented Aug 20, 2021

elasticmachine commented Aug 20, 2021

elasticmachine commented Aug 20, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

jsoriano commented Aug 20, 2021

adriansr commented Aug 20, 2021

adriansr commented Aug 23, 2021

aspacca commented Aug 23, 2021

kaiyan-sheng commented Aug 23, 2021 •

edited

Loading

adriansr commented Aug 30, 2021

aspacca commented Sep 2, 2021 •

edited

Loading

mergify bot commented Sep 3, 2021

adriansr commented Sep 6, 2021

aspacca Sep 6, 2021 •

edited

Loading

adriansr Sep 6, 2021

aspacca Sep 6, 2021

adriansr Sep 8, 2021

aspacca Sep 8, 2021

mergify bot commented Sep 10, 2021

adriansr commented Sep 13, 2021

Filebeat: Have filesets disabled unless explicitly configured #27526

Filebeat: Have filesets disabled unless explicitly configured #27526

Conversation

adriansr commented Aug 20, 2021 • edited by jsoriano Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Aug 20, 2021

elasticmachine commented Aug 20, 2021

elasticmachine commented Aug 20, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

jsoriano commented Aug 20, 2021

adriansr commented Aug 20, 2021

adriansr commented Aug 23, 2021

aspacca commented Aug 23, 2021

kaiyan-sheng commented Aug 23, 2021 • edited Loading

adriansr commented Aug 30, 2021

aspacca commented Sep 2, 2021 • edited Loading

mergify bot commented Sep 3, 2021

adriansr commented Sep 6, 2021

aspacca Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

adriansr Sep 6, 2021

Choose a reason for hiding this comment

aspacca Sep 6, 2021

Choose a reason for hiding this comment

adriansr Sep 8, 2021

Choose a reason for hiding this comment

aspacca Sep 8, 2021

Choose a reason for hiding this comment

mergify bot commented Sep 10, 2021

adriansr commented Sep 13, 2021

adriansr commented Aug 20, 2021 •

edited by jsoriano

Loading

elasticmachine commented Aug 20, 2021 •

edited by jenkins-beats-ci bot

Loading

kaiyan-sheng commented Aug 23, 2021 •

edited

Loading

aspacca commented Sep 2, 2021 •

edited

Loading

aspacca Sep 6, 2021 •

edited

Loading