-
Notifications
You must be signed in to change notification settings - Fork 306
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
Conversation
Overall package sizeSelf size: 7.15 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 |
BenchmarksBenchmark execution time: 2024-09-24 16:59:35 Comparing candidate commit 90fef58 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics. |
Just want to point out that removing Object literal may only specify known properties, and 'enablePropagationWithAmazonHeaders' does not exist in type 'http'. |
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 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? 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. |
@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. |
thanks @tlhunter , sounds like a plan 👍 |
Has anyone seen this affect v3 of the AWS SDK? And has anyone seen this affect any service other than S3? |
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. |
…" (#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
…" (#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
…" (#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
What does this PR do?
Motivation
Plugin Checklist
Additional Notes