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.

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

x-pack/filebeat/input/awss3: allow cross-region bucket configuration #40309

Merged
merged 3 commits into from
Jul 28, 2024

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Jul 23, 2024

Proposed commit message

See title.

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.

Disruptive User Impact

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@efd6 efd6 added Filebeat Filebeat backport-skip Skip notification from the automated backport with mergify Team:Security-Service Integrations Security Service Integrations Team labels Jul 23, 2024
@efd6 efd6 self-assigned this Jul 23, 2024
@botelastic 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 Jul 23, 2024
@efd6 efd6 marked this pull request as ready for review July 23, 2024 07:47
@efd6 efd6 requested a review from a team as a code owner July 23, 2024 07:47
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@narph narph requested a review from andrewkroh July 23, 2024 08:33
Comment on lines 293 to 298
opts := a.client.Options()
if opts.Region == region {
return a.client
}
opts.Region = region
return s3.New(opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on the frequency of the new-client path, this could generate a reasonable amount of work. Something that I considered but did not add is having awsS3API hold a map[string]*s3.Client that can cache (non-retiring) the clients for each region. Given that there are not large numbers of regions, it should be reasonable to retain clients that are constructed dynamically for the life of the awsS3API. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good idea. My intuition is that in the use-cases where this is needed that it can be in very high volumes (big orgs, lots of aws projects, lots of regions, all notifications centralized to one queue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added caching and limited the size to 100. We should never hit this limit, but I wanted to make sure that we don't put ourselves in the situation that this results in an OoM crash.

Copy link
Contributor

mergify bot commented Jul 23, 2024

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

@andrewkroh andrewkroh added bug bugfix backport-8.15 Automated backport to the 8.15 branch with mergify and removed bug labels Jul 24, 2024
@andrewkroh
Copy link
Member

I added the backport label thinking that this can try to target v8.15.1.

@efd6 efd6 removed the backport-skip Skip notification from the automated backport with mergify label Jul 25, 2024
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM.

@efd6 efd6 merged commit e177fc5 into elastic:main Jul 28, 2024
19 checks passed
mergify bot pushed a commit that referenced this pull request Jul 28, 2024
@kaiyan-sheng
Copy link
Contributor

@efd6 Should we create a separate PR to update the documentation too? Thanks!

@efd6
Copy link
Contributor Author

efd6 commented Jul 29, 2024

@kaiyan-sheng I had not intended to include documentation since this feature needs no user understanding.

efd6 added a commit that referenced this pull request Jul 29, 2024
…on bucket configuration (#40371)

* x-pack/filebeat/input/awss3: allow cross-region bucket configuration (#40309)

(cherry picked from commit e177fc5)

* remove irrelevant changelog entries

---------

Co-authored-by: Dan Kortschak <dan.kortschak@elastic.co>
andrewkroh added a commit to andrewkroh/beats that referenced this pull request Aug 27, 2024
When a region is not specified in the SQS notification then reuse the existing
S3 client instead of creating a new one based on an empty (unspecified) AWS
region name. This problem affected custom SQS notification formats that did
not specify a region name (like Crowdstrike FDR notifications).

Fixes: elastic/integrations/elastic#10647
Relates: elastic#40309
andrewkroh added a commit to andrewkroh/beats that referenced this pull request Aug 27, 2024
When a region is not specified in the SQS notification then reuse the existing
S3 client instead of creating a new one based on an empty (unspecified) AWS
region name. This problem affected custom SQS notification formats that did
not specify a region name (like Crowdstrike FDR notifications).

This addresses errors like:

    S3 download failure: s3 GetObject failed: operation error S3: GetObject, resolve auth scheme: resolve endpoint: endpoint rule error, Invalid region: region was not a valid DNS name.

Fixes: elastic/integrations/elastic#10647
Relates: elastic#40309
andrewkroh added a commit that referenced this pull request Aug 27, 2024
…#40628)

When a region is not specified in the SQS notification then reuse the existing
S3 client instead of creating a new one based on an empty (unspecified) AWS
region name. This problem affected custom SQS notification formats that did
not specify a region name (like Crowdstrike FDR notifications).

This addresses errors like:

    S3 download failure: s3 GetObject failed: operation error S3: GetObject, resolve auth scheme: resolve endpoint: endpoint rule error, Invalid region: region was not a valid DNS name.

Fixes: elastic/integrations/#10647
Relates: #40309
mergify bot pushed a commit that referenced this pull request Aug 27, 2024
…#40628)

When a region is not specified in the SQS notification then reuse the existing
S3 client instead of creating a new one based on an empty (unspecified) AWS
region name. This problem affected custom SQS notification formats that did
not specify a region name (like Crowdstrike FDR notifications).

This addresses errors like:

    S3 download failure: s3 GetObject failed: operation error S3: GetObject, resolve auth scheme: resolve endpoint: endpoint rule error, Invalid region: region was not a valid DNS name.

Fixes: elastic/integrations/#10647
Relates: #40309
(cherry picked from commit 6d25d46)
andrewkroh added a commit that referenced this pull request Aug 27, 2024
…#40628) (#40632)

When a region is not specified in the SQS notification then reuse the existing
S3 client instead of creating a new one based on an empty (unspecified) AWS
region name. This problem affected custom SQS notification formats that did
not specify a region name (like Crowdstrike FDR notifications).

This addresses errors like:

    S3 download failure: s3 GetObject failed: operation error S3: GetObject, resolve auth scheme: resolve endpoint: endpoint rule error, Invalid region: region was not a valid DNS name.

Fixes: elastic/integrations/#10647
Relates: #40309

(cherry picked from commit 6d25d46)

Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.15 Automated backport to the 8.15 branch with mergify bugfix Filebeat Filebeat Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants