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

AWS S3 Input custom endpoint handling broken in 8.15 #40792

Open
strawgate opened this issue Sep 12, 2024 · 3 comments
Open

AWS S3 Input custom endpoint handling broken in 8.15 #40792

strawgate opened this issue Sep 12, 2024 · 3 comments
Assignees
Labels
bug Team:Cloud-Monitoring Label for the Cloud Monitoring team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Comments

@strawgate
Copy link
Contributor

strawgate commented Sep 12, 2024

8.15 no longer has the fix for AWS S3 endpoint handling that was present in 8.14

8.15:

if config.AWSConfig.Endpoint != "" {
// Add a custom endpointResolver to the awsConfig so that all the requests are routed to this endpoint
awsConfig.EndpointResolverWithOptions = awssdk.EndpointResolverWithOptionsFunc(func(service, region string, options ...interface{}) (awssdk.Endpoint, error) {
return awssdk.Endpoint{
PartitionID: "aws",
URL: config.AWSConfig.Endpoint,
SigningRegion: awsConfig.Region,
}, nil
})
}

8.14:

if config.AWSConfig.Endpoint != "" {
// Parse a URL for the host regardless of it missing the scheme
endpointUri, err := url.Parse(config.AWSConfig.Endpoint)
if err != nil {
return nil, fmt.Errorf("failed to parse endpoint: %w", err)
}
// For backwards compat:
// If the endpoint does not start with S3, we will use the endpoint resolver to make all SDK requests use the specified endpoint
// If the endpoint does start with S3, we will use the default resolver uses the endpoint field but can replace s3 with the desired service name like sqs
if !strings.HasPrefix(endpointUri.Hostname(), "s3") {
awsConfig.EndpointResolverWithOptions = awssdk.EndpointResolverWithOptionsFunc(func(service, region string, options ...interface{}) (awssdk.Endpoint, error) {
return awssdk.Endpoint{
PartitionID: "aws",
Source: awssdk.EndpointSourceCustom,
URL: config.AWSConfig.Endpoint,
SigningRegion: awsConfig.Region,
}, nil
})
}
}

In AWS, the endpoint field is supposed to act kind of like a "base url" where service URLs are built using the value in the endpoint field. So when the SQS client makes a request, an endpoint field of s3.us-east1.amazonaws.com is transformed into sqs.us-east1.amazonaws.com, etc.

The 8.15 code forces all endpoints to use the value in the endpoint field instead of relying on the resolver to use the endpoint to "build" each service's endpoint (s3, sqs, etc). In the example above, this would cause the SQS client to directly query s3.us-east1.amazonaws.com

Even the 8.14 code has an issue that crops up with some customers. We should likely switch to only using a custom endpoint resolver when a user explicitly tells us to, for example by introducing a new setting called "static endpoint" or something similar, that when set to true, sets the endpoint resolver as it is set currently. This would be a breaking change.

An alternative would be introducing a setting called, "dynamic_endpoint" or something similar which, when set, sets the endpoint field without using a resolver.

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 12, 2024
@ycombinator ycombinator added bug Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Sep 12, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 12, 2024
@ycombinator ycombinator added the Team:Cloud-Monitoring Label for the Cloud Monitoring team label Sep 12, 2024
@cmacknz
Copy link
Member

cmacknz commented Sep 13, 2024

#39709 was only in 8.14 and #39722 to bring it to main was never merged or backported.

@strawgate
Copy link
Contributor Author

#39709 was only in 8.14 and #39722 to bring it to main was never merged or backported.

39722 was not supposed to merge, it was just an example for how to include it post s3/sqs refactor, ill close the PR but add that as a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Team:Cloud-Monitoring Label for the Cloud Monitoring team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

No branches or pull requests

5 participants