x-pack/filebeat/input/awss3: relax queue_url constraints for non-standard endpoints #35520

efd6 · 2023-05-22T03:40:42Z

What does this PR do?

Currently, the queue_url option is used to obtain the region name for the SQS receiver. This prevents pointing the input at other sources for testing, so add a region_name option to provide an alternative way to provide the region name for non-AWS endpoints.

To avoid confusion, log a warning if the user configures the region option in conjunction with amazonaws.com endpoints such that they disagree.

Why is it important?

This is needed for supporting tests of inputs using s3.

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

Related issues

Closes [Filebeat] aws-s3 input should support any queue_url #35496

Use cases

Screenshots

Logs

elasticmachine · 2023-05-22T05:51:46Z

💚 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: 2023-06-08T08:33:58.468+0000
Duration: 129 min 20 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5968
Skipped	360
Total	6328

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-05-22T07:01:45Z

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

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

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

legoguy1000 · 2023-05-23T05:57:03Z

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

+		if err != nil {
+			return fmt.Errorf("invalid queue_url: %w", err)
+		}
+		if strings.HasSuffix(u.Host, ".amazonaws.com") {


There are other AWS domains besides just amazonaws.com that you should include. See https://github.com/elastic/beats/blob/main/x-pack/filebeat/input/awss3/input.go#L350

Thanks. I was looking for a list.

bhapas

LGTM

andrewkroh

This is complicated because of all the ways in which a user can set the AWS region. I think those are:

default_region in input config
AWS_DEFAULT_REGION env var
AWS_REGION env var
extracted from the queue_url
region_name in input config (proposed)

My personal preference in behavior is to require an explicit region name to be configured either through env (which is part of the AWS SDK) or via the existing default_region config option. This would have the most clear semantics IMO -- you must set it or else it won't work and there's no guessing about the region being used. But our own SDK config wrapper injects a "default default" region of us-east-1 so we can't detect if the user did not set a region.

But we can't change that behavior without a breaking change, so if we make region_name take precedence over queue_url parsing I still that will be clear.

One thing I want to be sure of here is that we have tested this before merging to ensure that it works to pull data from Localstack. @bhapas can help with that step.

andrewkroh · 2023-05-24T22:28:36Z

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

@@ -78,6 +79,13 @@ func (c *config) Validate() error {
 		return fmt.Errorf("number_of_workers <%v> must be greater than 0", c.NumberOfWorkers)
 	}

+	if c.QueueURL != "" {
+		region, _ := getRegionFromQueueURL(c.QueueURL, c.AWSConfig.Endpoint)


Do we need to have this validation or could we relax it? As a user, I think the semantics of explicitly setting a region name would be my preference in my config. (It's just a shame that we cannot rely on the AWS SDK config for this because it injects a default region so we can't tell if the user explicitly configured it).

This will steer users that are having trouble with their combination of queue_url and endpoint toward the region_name which is documented as for testing purposes. Not sure this is bad but I wanted to call it out. A common problem is that users get an error from getRegionFromQueueURL until they learn that they need to set endpoint.

I'd be happy to relax this if we emit a log warning, though I don't think that is possible at this point, but is in input.go with some additional cost per start. WDYT?

andrewkroh · 2023-05-24T22:45:33Z

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

@@ -308,9 +311,11 @@ func getRegionFromQueueURL(queueURL string, endpoint string) (string, error) {
 		return "", fmt.Errorf(queueURL + " is not a valid URL")


We need a unit test for this function. It has zero test coverage. When it was added it had some, but I must have lost them in a refactoring. Can you please add back the test case from 7b729da.

andrewkroh · 2023-05-24T22:50:07Z

x-pack/filebeat/docs/inputs/input-aws-s3.asciidoc

@@ -257,6 +257,12 @@ configuring multiline options.

 URL of the AWS SQS queue that messages will be received from. (Required when `bucket_arn` and `non_aws_bucket_name` are not set).

+[float]
+==== `region_name`


I think using region is more consistent with everything in the AWS realm.

andrewkroh

LGTM.

Before merging, @bhapas can you please test that this works with Localstack based SQS and S3.

zmoog

LGTM.

mergify · 2023-06-06T21:46:40Z

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 35496-awss3 upstream/35496-awss3
git merge upstream/main
git push upstream 35496-awss3

efd6 · 2023-06-07T11:52:37Z

/test

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

Currently, the queue_url option is used to obtain the region name for the SQS receiver. This prevents pointing the input at other sources for testing, so add a region_name option to provide an alternative way to provide the region name for non-AWS endpoints. To avoid confusion, prevent using the region_name option in conjunction with amazonaws.com endpoints and document this.

Also improve comprehensability of test cases.

Make getRegionFromQueueURL aware of configured region name, this means that it will return either a non-zero region name or a non-nil error.

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

bhapas

LGTM

efd6 added enhancement Filebeat Filebeat Team:Security-External Integrations backport-skip Skip notification from the automated backport with mergify 8.9-candidate labels May 22, 2023

efd6 self-assigned this May 22, 2023

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 May 22, 2023

efd6 requested a review from a team May 22, 2023 03:40

efd6 force-pushed the 35496-awss3 branch from 56a51bc to ea9a2c6 Compare May 22, 2023 03:41

efd6 marked this pull request as ready for review May 22, 2023 07:01

efd6 requested a review from a team as a code owner May 22, 2023 07:01

bhapas reviewed May 22, 2023

View reviewed changes

x-pack/filebeat/input/awss3/config.go Outdated Show resolved Hide resolved

x-pack/filebeat/input/awss3/input.go Outdated Show resolved Hide resolved

legoguy1000 reviewed May 23, 2023

View reviewed changes

efd6 force-pushed the 35496-awss3 branch from da18d63 to 2572777 Compare May 24, 2023 00:58

bhapas approved these changes May 24, 2023

View reviewed changes

andrewkroh reviewed May 24, 2023

View reviewed changes

efd6 changed the title ~~x-pack/filebeat/input/awss3: relax queue_url constraints for testing~~ x-pack/filebeat/input/awss3: relax queue_url constraints for non-standard endpoints May 25, 2023

efd6 force-pushed the 35496-awss3 branch from dda94fd to 06e8b19 Compare May 25, 2023 00:31

This comment was marked as outdated.

Sign in to view

efd6 force-pushed the 35496-awss3 branch from 06e8b19 to d639d7e Compare May 25, 2023 01:04

efd6 requested a review from andrewkroh May 28, 2023 23:11

efd6 force-pushed the 35496-awss3 branch from d639d7e to 979c68e Compare June 1, 2023 03:39

andrewkroh approved these changes Jun 1, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

efd6 force-pushed the 35496-awss3 branch from 979c68e to b36fee9 Compare June 5, 2023 23:15

zmoog approved these changes Jun 5, 2023

View reviewed changes

efd6 force-pushed the 35496-awss3 branch from b36fee9 to 38700cb Compare June 6, 2023 21:55

bhapas reviewed Jun 7, 2023

View reviewed changes

x-pack/filebeat/input/awss3/input.go Outdated Show resolved Hide resolved

x-pack/filebeat/input/awss3/input.go Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

efd6 added 4 commits June 8, 2023 11:39

address pr comment

b30fc06

Also improve comprehensability of test cases.

address pr comment

895f935

address pr comments

fb955e5

efd6 force-pushed the 35496-awss3 branch from 38700cb to fb955e5 Compare June 8, 2023 02:09

efd6 added 2 commits June 8, 2023 13:29

ignore empty region name

8752c3e

address pr comment

0fc0ab5

Make getRegionFromQueueURL aware of configured region name, this means that it will return either a non-zero region name or a non-nil error.

bhapas reviewed Jun 8, 2023

View reviewed changes

x-pack/filebeat/input/awss3/input.go Show resolved Hide resolved

bhapas approved these changes Jun 8, 2023

View reviewed changes

efd6 merged commit 7b1a839 into elastic:main Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x-pack/filebeat/input/awss3: relax queue_url constraints for non-standard endpoints #35520

x-pack/filebeat/input/awss3: relax queue_url constraints for non-standard endpoints #35520

efd6 commented May 22, 2023 •

edited

Loading

elasticmachine commented May 22, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

elasticmachine commented May 22, 2023

legoguy1000 May 23, 2023

efd6 May 23, 2023

bhapas left a comment

andrewkroh left a comment

andrewkroh May 24, 2023

efd6 May 24, 2023

andrewkroh May 24, 2023

andrewkroh May 24, 2023

This comment was marked as outdated.

andrewkroh left a comment

This comment was marked as outdated.

zmoog left a comment

mergify bot commented Jun 6, 2023

efd6 commented Jun 7, 2023

This comment was marked as outdated.

bhapas left a comment

		@@ -308,9 +311,11 @@ func getRegionFromQueueURL(queueURL string, endpoint string) (string, error) {
		return "", fmt.Errorf(queueURL + " is not a valid URL")

x-pack/filebeat/input/awss3: relax queue_url constraints for non-standard endpoints #35520

x-pack/filebeat/input/awss3: relax queue_url constraints for non-standard endpoints #35520

Conversation

efd6 commented May 22, 2023 • 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 May 22, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented May 22, 2023

legoguy1000 May 23, 2023

Choose a reason for hiding this comment

efd6 May 23, 2023

Choose a reason for hiding this comment

bhapas left a comment

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh May 24, 2023

Choose a reason for hiding this comment

efd6 May 24, 2023

Choose a reason for hiding this comment

andrewkroh May 24, 2023

Choose a reason for hiding this comment

andrewkroh May 24, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

andrewkroh left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

zmoog left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 6, 2023

efd6 commented Jun 7, 2023

This comment was marked as outdated.

bhapas left a comment

Choose a reason for hiding this comment

efd6 commented May 22, 2023 •

edited

Loading

elasticmachine commented May 22, 2023 •

edited by jenkins-beats-ci bot

Loading