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

aspacca · 2021-08-26T14:42:24Z

…et_arn are not provided

Bug

What does this PR do?

we log a warn message if neither queue_url nor bucket_arn is setup for an AWS module fileset inestead of failing validation

Why is it important?

if fileset for AWS module is not defined, instead of being explicitly disabled, its config validation won't be skipped: since neither queue_url nor bucket_arn are defined validation will fail and filebeat exits

without this fix any filebeat setup with custom config where any aws module fileset is not define will prevent to start after update to 7.15

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.

Author's Checklist

[ ]

How to test this PR locally

test filebeat with AWS module enabled (similar config like #23226 (comment)) without all the filesets explicitly configured as enabled: false

Related issues

Use cases

Screenshots

Logs

2021-08-26T16:29:12.960+0200    WARN    [aws-s3]        awss3/config.go:55      neither queue_url or bucket_arn were provided, input aws-s3 will stop

will replace

2021-08-25T12:03:34.312+0200    ERROR   [input.aws-s3]  awss3/input.go:93       getRegionFromQueueURL failed: QueueURL is not in format: https://sqs.{REGION_ENDPOINT}.{ENDPOINT}/{ACCOUNT_NUMBER}/{QUEUE_NAME} {"queue_url": "<no value>"}

…et_arn are not provided

elasticmachine · 2021-08-26T14:42:57Z

Pinging @elastic/integrations (Team:Integrations)

elasticmachine · 2021-08-26T16:17:22Z

💚 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-03T16:07:06.735+0000
Duration: 96 min 2 sec
Commit: bb40813

Test stats 🧪

Test	Results
Failed	0
Passed	15216
Skipped	2315
Total	17531

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	15216
Skipped	2315
Total	17531

kaiyan-sheng · 2021-08-26T17:13:55Z

x-pack/filebeat/input/awss3/config.go

@@ -50,7 +51,8 @@ func defaultConfig() config {

 func (c *config) Validate() error {
 	if c.QueueURL == "" && c.BucketARN == "" {
-		return fmt.Errorf("queue_url or bucket_arn must provided")
+		logp.NewLogger(inputName).Warnf("neither queue_url nor bucket_arn were provided, input %s will stop", inputName)
+		return nil


when return nil here, will the input stop?

2021-08-26T19:43:43.630+0200 INFO [input.aws-s3] compat/compat.go:111 Input aws-s3 starting {"id": "6E0AAAB966620B45"} ... 2021-08-26T19:43:43.631+0200 INFO [input.aws-s3] compat/compat.go:124 Input 'aws-s3' stopped {"id": "6E0AAAB966620B45"}

OK I tested it with only aws-s3 input and here is what's in the log:

2021-08-26T14:33:03.661-0600 WARN [aws-s3] awss3/config.go:54 neither queue_url nor bucket_arn were provided, input aws-s3 will stop 2021-08-26T14:33:06.635-0600 INFO [add_cloud_metadata] add_cloud_metadata/add_cloud_metadata.go:101 add_cloud_metadata: hosting provider type not detected. 2021-08-26T14:33:10.668-0600 INFO [crawler] beater/crawler.go:141 Starting input (ID: 3022854742447124592) 2021-08-26T14:33:10.668-0600 INFO [input.aws-s3] compat/compat.go:111 Input aws-s3 starting {"id": "29F3565F5B2A7070"} 2021-08-26T14:33:10.668-0600 INFO [input.aws-s3] compat/compat.go:124 Input 'aws-s3' stopped {"id": "29F3565F5B2A7070"}

But with the aws-s3 input stopped, Filebeat is still running. Is that expected? For the other validations done in this Validate function, if validation didn't pass, it returns the error and Filebeat stops.

That's because Validate will pass if neither queue_url nor bucket_arn is set.
I change this behaviour after report on #23226 (comment)

Since we cannot control all users' config we would risk a lot of setup updated to 7.15 starting to fail to start Filebeat because they have aws module defined with no all the filesets not actually used set to enable: false
This shouldn't affect filesets in aws module that are already setup properly with queue_url

@kaiyan-sheng the original behaviour is to no stop Filebeat if an aws fileset is not defined, and we already have "strange" log entries when this happens:

2021-08-25T12:03:34.312+0200 ERROR [input.aws-s3] awss3/input.go:93 getRegionFromQueueURL failed: QueueURL is not in format: https://sqs.{REGION_ENDPOINT}.{ENDPOINT}/{ACCOUNT_NUMBER}/{QUEUE_NAME} {"queue_url": "<no value>"}

the behaviour will be the same and the log entry will be replace with:

2021-08-26T16:29:12.960+0200 WARN [aws-s3] awss3/config.go:55 neither queue_url or bucket_arn were provided, input aws-s3 will stop

The problem, as far as I can see it, is that we cannot control what aws module's config users have defined, with or without every filesets defined with enabled: false
The source of the problem resides on https://github.com/elastic/beats/blob/master/filebeat/fileset/modules.go#L75 and https://github.com/elastic/beats/blob/master/filebeat/fileset/modules.go#L83, where we set an empty FilesetConfig having FilesetConfig.Enabled = nil and then we check for FilesetConfig.Enabled != nil && !(*fcfg.Enabled).
Changing to fcfg = &FilesetConfig{Enabled: new(bool) }, would be probably the right solution, but I can see a similar change in behaviour (https://github.com/elastic/beats/pull/27526/files) was decided to be breaking and will happen only on 8.x [1]

Excluding the change there, and assuming we don't want to have updates to 7.15 for filebeat stop starting (given custom users' config with no fileset defined), I cannot see any other way to prevent than the "fix" in this PR (I've tried changing the config rendering so to not fail to start, and even if possible in the end the result wouldn't be different than logging a strange message that something is not properly configured)

Before queue_url was always rendered in the fileset config, as <no value> if the fileset was not defined at all.
Now either queue_url or bucket_arn will be rendered according to which one of the related var is defined.
Since both of them can not being defined, and the fileset won't be skipped for validation, the less disruptive way I found would be to not fail config validation in this case.

The case was reported in #23226

@exekias hope you have more context

[1] This would be my preferred fix if possible to backport

Sorry for the late reply,

Ideally a wrong config would end up in an error, which would make Filebeat stop.

I understand the concern around the change to current behavior, and it seems we cannot implement this error in a backwards compatible way, at least for 7.15. What about doing this change as it is now and enabling the error once #27526 goes in?

Ideally a wrong config would end up in an error, which would make Filebeat stop.

I agree, the problem here is that it could be both a true positive error (fileset defined with no queue_url or bucket_arn) or a false positive one (fileset not defined), we have to decide what's most destructive

What about doing this change as it is now and enabling the error once #27526 goes in?

for me it's fine, but this means targeting 8.x
@kaiyan-sheng ?

@exekias Thanks for your input. @aspacca Targeting 8.x sounds good to me. Maybe we can add a warning in the documentation for now?

#27612) * warn log entry and no validation failure when both queue_url and bucket_arn are not provided * improve documentation (cherry picked from commit 76974f8)

#27612) (#27801) * warn log entry and no validation failure when both queue_url and bucket_arn are not provided * improve documentation (cherry picked from commit 76974f8) Co-authored-by: Andrea Spacca <[email protected]>

#27612) (#27800) * warn log entry and no validation failure when both queue_url and bucket_arn are not provided * improve documentation (cherry picked from commit 76974f8) Co-authored-by: Andrea Spacca <[email protected]>

* master: (39 commits) [Heartbeat] Move JSON tests from python->go (elastic#27816) docs: simplify permissions for Dockerfile COPY (elastic#27754) Osquerybeat: Fix osquery logger plugin severy levels mapping (elastic#27789) [Filebeat] Update compatibility function to remove processor description on ES < 7.9.0 (elastic#27774) warn log entry and no validation failure when both queue_url and buck… (elastic#27612) libbeat/cmd/instance: ensure test config file has appropriate permissions (elastic#27178) [Heartbeat] Add httpcommon options to ZipURL (elastic#27699) Add a header round tripper option to httpcommon (elastic#27509) [Elastic Agent] Add validation to ensure certificate paths are absolute. (elastic#27779) Rename dashboards according to module.yml files for master (elastic#27749) Refactor vagrantfile, add scripts for provisioning with docker/kind (elastic#27726) Accept syslog dates with leading 0 (elastic#27775) [Filebeat] Add timezone config option to decode_cef and syslog input (elastic#27727) [Filebeat] Threatintel compatibility updates (elastic#27323) Add support for ephemeral containers in elastic agent dynamic provider (elastic#27707) [Filebeat] Integration tests in CI for AWS-S3 input (elastic#27491) Fix flakyness of TestFilestreamEmptyLine (elastic#27705) [Filebeat] kafka v2 using parsers (elastic#27335) Update Kafka version parsing / supported range (elastic#27720) Update Sarama to 1.29.1 (elastic#27717) ...

elastic#27612) * warn log entry and no validation failure when both queue_url and bucket_arn are not provided * improve documentation

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

09cd1c3

…et_arn are not provided

aspacca requested a review from kaiyan-sheng August 26, 2021 14:42

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 26, 2021

aspacca added backport-v7.15.0 Automated backport with mergify backport-v7.16.0 Automated backport with mergify Team:Integrations Label for the Integrations team labels Aug 26, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 26, 2021

kaiyan-sheng reviewed Aug 26, 2021

View reviewed changes

aspacca mentioned this pull request Sep 2, 2021

Filebeat: Have filesets disabled unless explicitly configured #27526

Merged

5 tasks

improve documentation

bb40813

aspacca mentioned this pull request Sep 6, 2021

Breaking changes for 8.0 #13911

Closed

19 tasks

kaiyan-sheng approved these changes Sep 7, 2021

View reviewed changes

aspacca merged commit 76974f8 into elastic:master Sep 8, 2021

mergify bot mentioned this pull request Sep 8, 2021

[7.x](backport #27612) warn log entry and no validation failure when both queue_url and buck… #27800

Merged

mergify bot mentioned this pull request Sep 8, 2021

[7.15](backport #27612) warn log entry and no validation failure when both queue_url and buck… #27801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aspacca commented Aug 26, 2021 •

edited

Loading

elasticmachine commented Aug 26, 2021

elasticmachine commented Aug 26, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

kaiyan-sheng Aug 26, 2021

aspacca Aug 26, 2021

aspacca Aug 26, 2021

kaiyan-sheng Aug 26, 2021

aspacca Sep 1, 2021

aspacca Sep 2, 2021

aspacca Sep 2, 2021 •

edited

Loading

exekias Sep 2, 2021 •

edited

Loading

aspacca Sep 3, 2021 •

edited

Loading

kaiyan-sheng Sep 3, 2021

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

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

Conversation

aspacca commented Aug 26, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

elasticmachine commented Aug 26, 2021

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

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

kaiyan-sheng Aug 26, 2021

Choose a reason for hiding this comment

aspacca Aug 26, 2021

Choose a reason for hiding this comment

aspacca Aug 26, 2021

Choose a reason for hiding this comment

kaiyan-sheng Aug 26, 2021

Choose a reason for hiding this comment

aspacca Sep 1, 2021

Choose a reason for hiding this comment

aspacca Sep 2, 2021

Choose a reason for hiding this comment

aspacca Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

exekias Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

aspacca Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

kaiyan-sheng Sep 3, 2021

Choose a reason for hiding this comment

aspacca commented Aug 26, 2021 •

edited

Loading

elasticmachine commented Aug 26, 2021 •

edited by jenkins-beats-ci bot

Loading

aspacca Sep 2, 2021 •

edited

Loading

exekias Sep 2, 2021 •

edited

Loading

aspacca Sep 3, 2021 •

edited

Loading