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

Prevent S3 URL presigning from generating a remote request span #1494

Merged
merged 4 commits into from
May 21, 2021

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented May 4, 2021

Fixes #922

This PR addresses ddtrace capturing a "fake" S3 request that the AWS S3 SDK reports when presigning a request URL.

Presigned URLs give users a pre-authenticated URL to be used in the future to retrieve data from S3, but they do not preform any data retrieval to get created.

The Ruby S3 SDK performs a "fake" request internally to generated a presigned URL: it creates an S3 get_object request, sends it through the regular request handler (one of which being Datadog::Contrib::Aws::Handler), but hijacks the "data send" step at the end by not actually sending the request.

Unfortunately, there's no data we can use to differentiate between a real request and a fake request in our ddtrace handler. I've opened a PR with AWS to provide us with that information, so we can simplify the logic from this PR in the future: aws/aws-sdk-ruby#2513

This PR effectively removes our request handler from these fake presign requests, the same approach taken internally by the S3 SDK for some of its own handlers that do not play nice with the fake presign request: https://github.com/aws/aws-sdk-ruby/blob/656691b470bb970627286cf1df2bd84049b6787d/gems/aws-sdk-s3/lib/aws-sdk-s3/presigner.rb#L194-L196

@marcotc marcotc requested a review from a team May 4, 2021 17:52
@marcotc marcotc self-assigned this May 4, 2021
@marcotc marcotc added the bug Involves a bug label May 4, 2021
@marcotc marcotc added this to In review in Active work May 4, 2021
@allcentury
Copy link

@marcotc congrats on getting the AWS SDK updated as well, this looks great. We're fine with this solution, or one where a AWS SDK minimum version is required.

ericmustin
ericmustin previously approved these changes May 21, 2021
Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

I think this is good to go, one small nit in code comments, not blocking though.

It looks like your upstream PR got merged but aws doesn't cut a new release that frequently, however in the future we may want to revist this and rely on the upstream approach of checking context[:presigned_url] for relevant versions.

lib/ddtrace/contrib/aws/instrumentation.rb Outdated Show resolved Hide resolved
Co-authored-by: Eric Mustin <eric.mustin@datadoghq.com>
@marcotc
Copy link
Member Author

marcotc commented May 21, 2021

It looks like your upstream PR got merged but aws doesn't cut a new release that frequently, however in the future we may want to revist this and rely on the upstream approach of checking context[:presigned_url] for relevant versions.

It would be nice.
We still have to support clients with older versions though...

I'll add a comment referencing the easier way to do it in the code.

@marcotc marcotc merged commit 9249240 into master May 21, 2021
@marcotc marcotc deleted the s3-presign branch May 21, 2021 23:00
Active work automation moved this from In review to Merged & awaiting release May 21, 2021
@github-actions github-actions bot added this to the 0.50.0 milestone May 21, 2021
@ivoanjo ivoanjo moved this from Merged & awaiting release to Released in Active work Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug
Projects
Active work
  
Released
Development

Successfully merging this pull request may close these issues.

Redis, S3 with Unexpected Results
3 participants