-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[8.14] Fix handling of custom Endpoint when using S3 + SQS #39709
Conversation
…ser-provided custom endpoints
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
…se the default_region if a custom endpoint is parsed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly some general Go commentary. I didn't look at the getRegionFromQueueURL changes very closely.
regionName, err := getRegionFromQueueURL(in.config.QueueURL, in.config.AWSConfig.Endpoint, in.config.RegionName) | ||
if err != nil && in.config.RegionName == "" { | ||
return fmt.Errorf("failed to get AWS region from queue_url: %w", err) | ||
regionName, err := getRegionFromQueueURL(in.config.QueueURL, in.config.AWSConfig.Endpoint, in.config.AWSConfig.DefaultRegion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's uncommon to return an error and a value that the caller should use. Typically these are mutually exclusive. You either get an error OR you get values that you should use. I suggest trying to a do a small bit of refactoring to keep with those conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@faec has refactored basically all of this plugin on main including undoing this but it's too different to backport.
I made a series of integration tests which cover all the various combinations of settings but I'm worried refactoring this might not be worth it given it's all going away soon
@cmacknz added additional tests |
To have CI actually trigger the tests you need to add the |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me. Thanks for the additional tests.
Fix custom endpoint selection in the S3/SQS input (#39718) by porting @strawgate's 8.14 fix (#39709) to main. In addition to the previous fixes, this simplifies the logic for detecting queue region, since the 8.14 version still had some broken cases caused by requiring over-strict endpoint matching, and it was concluded (talking to @strawgate) that there's no advantage to rejecting standard region format from queue URLs just because the endpoint URL is different (if there is a genuine mismatch in the queue and endpoint we'll learn it from the connection attempt, not from `getRegionFromQueueURL`).
Fix custom endpoint selection in the S3/SQS input (#39718) by porting @strawgate's 8.14 fix (#39709) to main. In addition to the previous fixes, this simplifies the logic for detecting queue region, since the 8.14 version still had some broken cases caused by requiring over-strict endpoint matching, and it was concluded (talking to @strawgate) that there's no advantage to rejecting standard region format from queue URLs just because the endpoint URL is different (if there is a genuine mismatch in the queue and endpoint we'll learn it from the connection attempt, not from `getRegionFromQueueURL`). (cherry picked from commit cf13781)
Fix custom endpoint selection in the S3/SQS input (#39718) by porting @strawgate's 8.14 fix (#39709) to main. In addition to the previous fixes, this simplifies the logic for detecting queue region, since the 8.14 version still had some broken cases caused by requiring over-strict endpoint matching, and it was concluded (talking to @strawgate) that there's no advantage to rejecting standard region format from queue URLs just because the endpoint URL is different (if there is a genuine mismatch in the queue and endpoint we'll learn it from the connection attempt, not from `getRegionFromQueueURL`). (cherry picked from commit cf13781)
…#41537) * Fix handling of custom endpoints in AWS input (#41504) Fix custom endpoint selection in the S3/SQS input (#39718) by porting @strawgate's 8.14 fix (#39709) to main. In addition to the previous fixes, this simplifies the logic for detecting queue region, since the 8.14 version still had some broken cases caused by requiring over-strict endpoint matching, and it was concluded (talking to @strawgate) that there's no advantage to rejecting standard region format from queue URLs just because the endpoint URL is different (if there is a genuine mismatch in the queue and endpoint we'll learn it from the connection attempt, not from `getRegionFromQueueURL`). (cherry picked from commit cf13781) * Update CHANGELOG.next.asciidoc auto-merge picked up an unrelated change --------- Co-authored-by: Fae Charlton <fae.charlton@elastic.co>
Fix custom endpoint selection in the S3/SQS input (#39718) by porting @strawgate's 8.14 fix (#39709) to main. In addition to the previous fixes, this simplifies the logic for detecting queue region, since the 8.14 version still had some broken cases caused by requiring over-strict endpoint matching, and it was concluded (talking to @strawgate) that there's no advantage to rejecting standard region format from queue URLs just because the endpoint URL is different (if there is a genuine mismatch in the queue and endpoint we'll learn it from the connection attempt, not from `getRegionFromQueueURL`). (cherry picked from commit cf13781) Co-authored-by: Fae Charlton <fae.charlton@elastic.co>
Proposed commit message
Fix issues described in #39706 that prevent using a custom endpoint with S3 + SQS.
Users can workaround this issue via S3 bucket polling. The S3 bucket polling still works just fine with a custom endpoint, it's just adding in SQS where it breaks. We need to publish a new version of the AWS integration with the endpoint field exposed on the relevant AWS integrations which is tracked here
Proposed Fixes for Main: #39722
Fixes for 8.14:
s3
Optional for 8.14:
s3
Limit the scope of the endpoint resolver:
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
Hopefully none.
The entire addition of getRegionFromQueueURL to handle custom endpoints can be removed and the user would just have to manually specify a region. Which would make this a bit smaller.
How to test this PR locally
Login to AWS CLI, provide the following in a filebeat config
See that the SQS ReceiveMessage works and you can publish an item to the bucket and get a result
See that the SQS ReceiveMessage works as the region is inferred from the queue_url matching the endpoint, and you can publish an item to the bucket and get a result
See that the SQS ReceiveMessage works as the region is inferred from the queue_url matching the endpoint, and you can publish an item to the bucket and get a result
See that the following fails:
See that the following works but fails to connect (no such host)
See that endpoint is
s3......
but the failure message sayssqs.localtest.amazonaws.com
Use cases
Allow users who use custom-but-AWS domains to enjoy the benefits of S3 and SQS together.