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

always enable tracing header injection for AWS requests #4717

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

tlhunter
Copy link
Member

@tlhunter tlhunter commented Sep 23, 2024

What does this PR do?

Motivation

  • I don't believe this has caused issues for users in years
  • users are complaining that this isn't the default behavior
  • users are unable to programmatically change config in certain environments
    • technically this could be solved by adding another env var

Plugin Checklist

Additional Notes

Copy link

github-actions bot commented Sep 23, 2024

Overall package size

Self size: 7.15 MB
Deduped: 62.51 MB
No deduping: 62.79 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 9.0.0 | 580.4 kB | 1.03 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@pr-commenter
Copy link

pr-commenter bot commented Sep 23, 2024

Benchmarks

Benchmark execution time: 2024-09-24 16:59:35

Comparing candidate commit 90fef58 in PR branch tlhunter/aws-always-trace with baseline commit b079c6f in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics.

@tlhunter tlhunter marked this pull request as ready for review September 24, 2024 16:58
@tlhunter tlhunter requested review from a team as code owners September 24, 2024 16:58
@tlhunter tlhunter merged commit 1d2543c into master Sep 25, 2024
192 checks passed
@tlhunter tlhunter deleted the tlhunter/aws-always-trace branch September 25, 2024 18:58
@Romack
Copy link

Romack commented Oct 15, 2024

Just want to point out that removing enablePropagationWithAmazonHeaders from the types file is a breaking change and should've been a major version update. Any typescript projects that that have implemented this "workaround" (e.g. us) will receive the following error if/when they upgrade dd-trace from 5.22.0 to version 5.23.1

Object literal may only specify known properties, and 'enablePropagationWithAmazonHeaders' does not exist in type 'http'.

@alxgrk
Copy link

alxgrk commented Oct 18, 2024

Unfortunately, this change broke our production environment, because it made AWS requests fail. Specifically, it it the following request that fails:

const {S3} = require('@aws-sdk/client-s3');
const awsS3Client = new S3();
const response = await awsS3Client
      .getObject({
        Bucket: 'my-bucket',
        Key: 'my-file',
        IfNoneMatch: 'my-etag',
      });

We tested both, with aws-sdk@2.884.0 as well as with @aws-sdk/client-s3@3.673.0 - same result.

From reading the different PRs/issues, it looks like since 2018 (here) the exception for signed AWS requests existed. In November 2023, it got removed (here), but reverted on the same day (here), because some users were facing issues. For some reason that I can not yet understand, it was assumed in this PR that the errors won't show up again. Is there any reason that led to this assumption?
As a workaround, we reverted to 5.22.0, but of course this error prevents us from upgrading.

PS: I have also submitted a helpdesk request, but wanted to make this problem visible to others as well.

EDIT: I have to correct myself, for aws-sdk@v3 the error was different (region missing in my example). So I assume, this issue only affects aws-sdk@v2.

@tlhunter
Copy link
Member Author

@alxgrk from the research I had done it appeared that Amazon had fixed the bug which caused any arbitrary header to affect the signature. I didn't realize this was only fixed with SDK v3 and not with v2.

We'll work on reverting this change, perhaps just for SDK v2 and not for v3.

@alxgrk
Copy link

alxgrk commented Oct 29, 2024

thanks @tlhunter , sounds like a plan 👍

@tlhunter
Copy link
Member Author

Has anyone seen this affect v3 of the AWS SDK? And has anyone seen this affect any service other than S3?

@tmercswims
Copy link

tmercswims commented Nov 6, 2024

This change broke our production as well. We still use aws-sdk v2, and get a signature mismatch error on many (but not all, interestingly) requests to DynamoDB. Downgrading to 5.22.0 fixes the errors.

tlhunter added a commit that referenced this pull request Nov 7, 2024
tlhunter added a commit that referenced this pull request Nov 7, 2024
…" (#4867)

- this reverts commit 1d2543c.
- reverts a change that would automatically inject tracing headers into AWS requests
- this appears to break S3 requests (and DynamoDB?) when using AWS SDK v2
  - we don't have any reports of other services or of AWS SDK v3 breaking
- for follow up work we need to make this a configurable environment variable instead of just an init setting
  - this is because folks using the lambda layer need to configure the tracer via env vars
  - alternatively we only block s3 and dynamo? however there could be other services that fail...
  - alternatively we only block aws sdk v2? however it seems that a bunch of the services are fine...
- internal stuff: APMS-13694, APMS-13713
- more discussion in #4717
tlhunter added a commit that referenced this pull request Nov 7, 2024
…" (#4867)

- this reverts commit 1d2543c.
- reverts a change that would automatically inject tracing headers into AWS requests
- this appears to break S3 requests (and DynamoDB?) when using AWS SDK v2
  - we don't have any reports of other services or of AWS SDK v3 breaking
- for follow up work we need to make this a configurable environment variable instead of just an init setting
  - this is because folks using the lambda layer need to configure the tracer via env vars
  - alternatively we only block s3 and dynamo? however there could be other services that fail...
  - alternatively we only block aws sdk v2? however it seems that a bunch of the services are fine...
- internal stuff: APMS-13694, APMS-13713
- more discussion in #4717
tlhunter added a commit that referenced this pull request Nov 7, 2024
…" (#4867)

- this reverts commit 1d2543c.
- reverts a change that would automatically inject tracing headers into AWS requests
- this appears to break S3 requests (and DynamoDB?) when using AWS SDK v2
  - we don't have any reports of other services or of AWS SDK v3 breaking
- for follow up work we need to make this a configurable environment variable instead of just an init setting
  - this is because folks using the lambda layer need to configure the tracer via env vars
  - alternatively we only block s3 and dynamo? however there could be other services that fail...
  - alternatively we only block aws sdk v2? however it seems that a bunch of the services are fine...
- internal stuff: APMS-13694, APMS-13713
- more discussion in #4717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants