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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Sep 8, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09cd1c3
warn log entry and no validation failure when both queue_url and buck…
Aug 26, 2021
bb40813
improve documentation
Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion x-pack/filebeat/input/awss3/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/elastic/beats/v7/libbeat/common/cfgtype"
"github.com/elastic/beats/v7/libbeat/common/match"
"github.com/elastic/beats/v7/libbeat/logp"
"github.com/elastic/beats/v7/libbeat/reader/parser"
"github.com/elastic/beats/v7/libbeat/reader/readfile"
"github.com/elastic/beats/v7/libbeat/reader/readfile/encoding"
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

when return nil here, will the input stop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@aspacca aspacca Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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:/elastic/beats/blob/master/filebeat/fileset/modules.go#L75 and https:/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:/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

Copy link
Contributor

@exekias exekias Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@aspacca aspacca Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

if c.QueueURL != "" && c.BucketARN != "" {
Expand Down
8 changes: 5 additions & 3 deletions x-pack/filebeat/input/awss3/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func TestConfig(t *testing.T) {
},
},
"",
func(queueURL, s3Bucketr string) config {
func(queueURL, s3Bucket string) config {
c := makeConfig(queueURL, "")
regex := match.MustCompile("/CloudTrail/")
c.FileSelectors = []fileSelectorConfig{
Expand All @@ -112,8 +112,10 @@ func TestConfig(t *testing.T) {
"queue_url": "",
"bucket_arn": "",
},
"queue_url or bucket_arn must provided",
nil,
"",
func(queueURL, s3Bucket string) config {
return makeConfig("", "")
},
},
{
"error on both queueURL and s3Bucket",
Expand Down