Conversation
DynamoDB and S3 client spans named the table or bucket but never the
account that owns it, so two accounts each holding an "orders" table
produced indistinguishable spans. The owning account is available to
the tracer without any extra call in three situations, and this change
uses all of them:
DynamoDB, TableName given as an ARN. The ARN carries partition, Region
and account. Spans now get aws.table.arn and aws_account, and
aws.table.name / tablename / peer.service carry the bare table name
instead of the raw ARN.
DynamoDB, bare TableName. DynamoDB documents that a bare name is
resolved in the requestor's own account ("If you only provide the table
name parameter instead of a complete ARN, the API operation will be
performed on the table in the account to which the requestor belongs"),
so the account owning the signing credentials is the table owner. On
SDK v2 the account is read from the credentials object
(AwsCredentialsIdentity.accountId(), SDK 2.26+, populated by the STS,
SSO, profile, process and container credential providers) through a
reflective, per-class cached getter so the instrumentation still loads
against 2.2.0. Spans get aws_account and a synthesized aws.table.arn
built from the client Region, account and table name. SDK v1 does not
expose the signing credentials to request handlers, so v1 only handles
the ARN form.
S3, ExpectedBucketOwner. When the caller sets it, S3 rejects the
request with 403 on a mismatch, so it is authoritative for the owner.
Spans get aws_account on both SDKs.
For older v2 SDKs whose credentials carry no account, the account can
optionally be decoded from the access key ID, which encodes it in its
trailing base32 characters. That encoding is not part of the documented
AWS API surface, so it is behind
dd.trace.aws.account.from.access.key.enabled
(DD_TRACE_AWS_ACCOUNT_FROM_ACCESS_KEY_ENABLED), default false.
aws_account is the tag name dd-trace-py already uses for the SNS and
SQS account it derives from TopicArn and QueueUrl, and the dimension
tag on the aws.dynamodb.* integration metrics, so the span joins to the
metrics for the same table without translation. It is also on the
allow list of the Agent's credit card obfuscator; a 12-digit account
under any other key is redacted to "?" by default, which is why no
second account-bearing key is introduced.
ARNs are parsed once into a new AwsArn value type (the SDK's own
software.amazon.awssdk.arns.Arn is not available at the 2.2.0 floor nor
to SDK v1), which also owns the table-name extraction shared by both
decorators. Partition mapping, table ARN assembly and access key decoding
live in AwsAccountIdentity. Both sit in the aws-java-common module and
are registered as injected helper classes by both SDK instrumentations.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
vandonr
added
inst: aws s3
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e16065da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| request | ||
| .getValueForField("ExpectedBucketOwner", String.class) | ||
| .filter(AwsAccountIdentity::isAccountId) | ||
| .ifPresent(owner -> setBucketOwner(span, owner)); |
There was a problem hiding this comment.
When a caller supplies a well-formed but incorrect 12-digit ExpectedBucketOwner, S3 returns 403 because the value does not identify the actual owner, yet this request-time code still records it as aws_account. Failed ownership checks therefore emit a confidently incorrect owning-account tag; defer or remove this tag unless the S3 request succeeds. The v1 decorator repeats the same eager assignment.
Useful? React with 👍 / 👎.
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| request | ||
| .getValueForField("ExpectedBucketOwner", String.class) | ||
| .filter(AwsAccountIdentity::isAccountId) | ||
| .ifPresent(owner -> setBucketOwner(span, owner)); |
There was a problem hiding this comment.
A well-formed but incorrect ExpectedBucketOwner causes S3 to return 403, yet its asserted value remains tagged as the bucket's actual account. Failed spans therefore report ownership that S3 explicitly rejected. Both SDK paths need to apply the tag only after a successful response.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
@pedramsafaei what do you think about the AI comment about tagging only after a successful request ? |
|
@vandonr On the 403 question it is misleading, I think. aws_account reads as "the account that owns this resource" and gets joined to the AWS integration metrics on that basis, and a 403 is exactly S3 telling us the asserted owner is wrong. Nothing useful is lost by withholding it and the error span still has the bucket and the 403. So it is now tagged only from the response hooks (v2 via an ExecutionAttribute on a 2xx, v1 in afterResponse), with a test for the 403 case. The ARN and credentials-derived accounts are unaffected. Pushed to my branch, head 2b35c67: pedramsafaei@2b35c67107. Could you reset contrib/pr-12602 to it? It covers the three Codex/Bits findings (DynamoDB enrichment gated on the service, the ExpectedBucketOwner change above, legacy pre-2019 access keys rejected via the format-marker bit) and the check_inst 4/4 failure, which was a SpotBugs ICAST in the base32 decoder. Two things I can't do is validate_supported_configurations_v2_local_file needs DD_TRACE_AWS_ACCOUNT_FROM_ACCESS_KEY_ENABLED registered in the Feature Parity Dashboard, and I could not reproduce the test_inst 8/8 / test_inst_latest 2/6 failures locally (all aws-java suites pass, including against ddapm-test-agent). If they still fail on the new head, could you paste the failing test names from the GitLab log? Thank you |
This branch has not been deployed
2 participants
Duplicate of #12602 by @pedramsafaei
DynamoDB and S3 client spans named the table or bucket but never the account that owns it, so two accounts each holding an "orders" table produced indistinguishable spans. The owning account is available to the tracer without any extra call in three situations, and this change uses all of them:
DynamoDB, TableName given as an ARN. The ARN carries partition, Region and account. Spans now get aws.table.arn and aws_account, and aws.table.name / tablename / peer.service carry the bare table name instead of the raw ARN.
DynamoDB, bare TableName. DynamoDB documents that a bare name is resolved in the requestor's own account ("If you only provide the table name parameter instead of a complete ARN, the API operation will be performed on the table in the account to which the requestor belongs"), so the account owning the signing credentials is the table owner. On SDK v2 the account is read from the credentials object (AwsCredentialsIdentity.accountId(), SDK 2.26+, populated by the STS, SSO, profile, process and container credential providers) through a reflective, per-class cached getter so the instrumentation still loads against 2.2.0. Spans get aws_account and a synthesized aws.table.arn built from the client Region, account and table name. SDK v1 does not expose the signing credentials to request handlers, so v1 only handles the ARN form.
S3, ExpectedBucketOwner. When the caller sets it, S3 rejects the request with 403 on a mismatch, so it is authoritative for the owner. Spans get aws_account on both SDKs.
For older v2 SDKs whose credentials carry no account, the account can optionally be decoded from the access key ID, which encodes it in its trailing base32 characters. That encoding is not part of the documented AWS API surface, so it is behind
dd.trace.aws.account.from.access.key.enabled
(DD_TRACE_AWS_ACCOUNT_FROM_ACCESS_KEY_ENABLED), default false.
aws_account is the tag name dd-trace-py already uses for the SNS and SQS account it derives from TopicArn and QueueUrl, and the dimension tag on the aws.dynamodb.* integration metrics, so the span joins to the metrics for the same table without translation. It is also on the allow list of the Agent's credit card obfuscator; a 12-digit account under any other key is redacted to "?" by default, which is why no second account-bearing key is introduced.
ARNs are parsed once into a new AwsArn value type (the SDK's own software.amazon.awssdk.arns.Arn is not available at the 2.2.0 floor nor to SDK v1), which also owns the table-name extraction shared by both decorators. Partition mapping, table ARN assembly and access key decoding live in AwsAccountIdentity. Both sit in the aws-java-common module and are registered as injected helper classes by both SDK instrumentations.
What Does This Do
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]