Repository navigation
Tag DynamoDB and S3 client spans with the owning AWS account - #12602
pedramsafaei wants to merge 5 commits into
Conversation
mhlidd
left a comment
mhlidd
left a comment
There was a problem hiding this comment.
LGTM from SDK Capabilities POV
574076d to
e1b9ef2
Compare
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Looking good from platform POV. Thanks for the follow up changes!
There was a problem hiding this comment.
Looks like we could use AWS SDK provided ARN parser for a good chunk of the work here, any reason to reimplement it ourselves here ?
https://docs.aws.amazon.com/java/api/latest/software/amazon/awssdk/arns/Arn.html
Also, if we want to keep our own implem, I think we should parse it once in an object. Here we end up checking several times if an ARN is valid for instance when we retrieve multiple values.
There was a problem hiding this comment.
The arns module is not a dependency of aws-core at the 2.2.0 floor this instrumentation compiles against (checked the 2.2.0 POM), and SDK v1 has no equivalent, so depending on it would break loading on older v2 and be unusable from the v1 module. Agreed on parsing once though pushed a new change that replaces the per-field helpers with an AwsArn value type parsed in a single pass, and both decorators call parse once.
| tableName = resource.substring("table/".length()); | ||
| int slash = tableName.indexOf('/'); | ||
| if (slash > 0) { | ||
| tableName = tableName.substring(0, slash); |
There was a problem hiding this comment.
doing the substring once with beginning and end would be more efficient (it'd save an intermediary string)
There was a problem hiding this comment.
Done. The extraction moved into AwsArn.dynamoDbTableName() and does one substring(begin, end).
| String account = AwsAccountIdentity.accountFromArn(tableName); | ||
| String resource = AwsAccountIdentity.resourceFromArn(tableName); | ||
| if (resource != null && resource.startsWith("table/")) { | ||
| span.setTag(InstrumentationTags.AWS_TABLE_ARN, tableName); |
There was a problem hiding this comment.
isn't it weird that if we found that the tableName is an ARN but the resource doesn't start with "table/" then we don't save it in the "AWS_TABLE_ARN" tag ?
There was a problem hiding this comment.
no it is intentional, but it deserved to be explicit. An ARN whose resource is not a table (a backup or export ARN, or a mistaken value) is not a table ARN, so tagging it as aws.table.arn would be wrong so in that case the raw value stays as the table name exactly as before this PR. In the new commit I pushed that decision lives in AwsArn.dynamoDbTableName() returning null, with a comment at both call sites.
| } | ||
| String expectedBucketOwner = access.getExpectedBucketOwner(originalRequest); | ||
| if (AwsAccountIdentity.isAccountId(expectedBucketOwner)) { | ||
| // Asserted by the caller and enforced by S3 (403 on mismatch), so authoritative when set. |
There was a problem hiding this comment.
if it's authoritative when set, why do we need to check if the returned value is an account ID ?
There was a problem hiding this comment.
Fair, the comment was misleading. It is authoritative for requests S3 accepts, but the span is tagged before the response is known, and S3 rejects a malformed owner (400) or a wrong one (403). Without the check a request the application got wrong would stamp aws_account=not-an-account on its error span. Kept the check and reworded the comment in the new commit the "malformed owner" test should cover it.
| if (resource != null && resource.startsWith("table/")) { | ||
| name = resource.substring("table/".length()); | ||
| int slash = name.indexOf('/'); | ||
| if (slash > 0) { | ||
| // arn:...:table/<name>/index/<index> and similar sub-resources | ||
| name = name.substring(0, slash); | ||
| } | ||
| tableArn = tableName; |
There was a problem hiding this comment.
looks like this duplicated part could go in the common helper
There was a problem hiding this comment.
new tests should be written in java using jUnit, not in groovy (except if strongly interconnected with existing tests, but I don't think it's the case here ?)
There was a problem hiding this comment.
Done. Now the only Groovy changes are new rows in the existing Aws2ClientTest and AWS1ClientTest.
|
|
||
| // Owning account of the addressed resource. aws_account matches the tag dd-trace-py sets and the | ||
| // dimension tag on the AWS integration metrics. | ||
| public static final String AWS_ACCOUNT = "aws_account"; |
There was a problem hiding this comment.
why not with a dot ?
| public static final String AWS_ACCOUNT = "aws_account"; | |
| public static final String AWS_ACCOUNT = "aws.account"; |
There was a problem hiding this comment.
Three reasons, all about the join. aws_account is the exact dimension tag on the aws.dynamodb.* and aws.s3.* integration metrics, so span and metric join without a rename and it is the key dd-trace-py already emits for the SNS and SQS account and also it is on the Agent credit-card obfuscator's allow list, whereas any other key carrying a 12-digit value is rewritten to ? at intake today (that happened live with aws.bucket.owner, see the obfuscator note in the description). aws.account would arrive as ? until DataDog/datadog-agent#56242 lands. Happy to also emit aws.account once that merges if you want the dotted form.
e1b9ef2 to
383c14b
Compare
vandonr
left a comment
vandonr
left a comment
There was a problem hiding this comment.
ok looks good (2 minor comments)
I'll need to duplicate this PR under my own account to be able to run the CI, I'll take care of this next week :)
| * | ||
| * @return the 12-digit account, or {@code null} when the key does not have the expected shape. | ||
| */ | ||
| public static String accountFromAccessKeyId(final String accessKeyId) { |
There was a problem hiding this comment.
I think it'd be nicer to untangle the base 32 parsing from actually getting the access key ID
| } | ||
|
|
||
| /** {@code true} when the value has the shape of an ARN, without fully parsing it. */ | ||
| public static boolean isArn(final String value) { |
There was a problem hiding this comment.
I think we can remove this as it's only used in tests, and checking AwsArn.parse(...) != null is actually a stricter check
383c14b to
a9e1606
Compare
a9e1606 to
2b35c67
Compare
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. The enrichment is gated on the DynamoDB service so
other request models with a TableName member (Timestream, Keyspaces)
keep the plain table name tags.
S3, ExpectedBucketOwner. When the caller sets it, S3 rejects the
request with 403 on a mismatch, so it names the owner whenever the
request succeeds. Spans get aws_account on both SDKs, applied from the
successful response so a rejected owner is never tagged.
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 for keys issued since March 2019 (older keys
carry no account and are rejected by a format marker check). 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.
2b35c67 to
9e8bb14
Compare
Use GenericClassValue for the optional accountId method handle so credentials classes and their application class loaders can be unloaded. Retain compatibility with the SDK 2.2.0 floor and cover present, absent, malformed, and failing providers. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Keep the credentials-derived aws_account for bare table names, but only emit aws.table.arn from request ARNs. Remove the region-to-partition table and ARN builder, and preserve service-gating regression coverage. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Remove the undocumented access-key decoder and its opt-in configuration, including configuration metadata and decoder tests. Older SDKs without accountId now leave the credentials account absent. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Rethrow VirtualMachineError and ThreadDeath while treating other provider and lookup failures, including LinkageError, as an unavailable account. Include method-handle lookup inside the error boundary and cover fatal and recoverable provider errors. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Keep aws.table.name, tablename, and peer.service unchanged for ARN TableName values on both SDK versions. Retain validation before adding aws.table.arn, reject other services and empty table names, and cover the behavior with unit and integration tests. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Resolve conflicts with the instrumentation-helper and tag-registry changes on master: - AwsSdkModule (SDK v1 and v2): drop the manually listed helperClassNames(); helper classes are now discovered automatically, so AwsAccountIdentity and AwsArn no longer need to be listed. - GetterAccess and AwsSdkClientDecorator (SDK v1): keep both the existing ExpectedBucketOwner getter / aws_account handling and master's new StateMachineArn, ExecutionArn and FunctionName getters and Lambda identity tagging. - InstrumentationTags: keep both the aws_account / aws.table.arn constants and master's new ARN/function constants. - Register aws.table.arn and aws_account in tag-conventions.yaml so the new tag constants are covered by the generated tag registry.
Declaring aws_account under both aws.s3 and aws.dynamodb (both client-direction span types sharing the aws.client parent) makes the tag-registry generator fail: a tag may be declared only once per direction. Declare it once on the shared aws.client parent so every AWS service span type inherits the same tag identity.
A tag declared on two concrete span types that share a parent direction has no single identity, so the generator must reject it; declaring it once on the shared parent lets both siblings inherit one tag. Cover both the rejection and the shared-parent resolution.
What Does This Do
Tags DynamoDB and S3 client spans (AWS SDK v1 and v2) with the AWS account that owns the addressed resource, using only information the tracer already holds. No additional network calls.
TableNameis an ARNaws_account,aws.table.arn(existingaws.table.name/tablename/peer.serviceare unchanged)TableName(SDK v2)aws_accountExpectedBucketOwnerset and the request succeedsaws_accountWhy the bare-name case is sound: DynamoDB documents that "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" (Cross-account access with resource-based policies). Cross-account access requires either the full ARN as
TableName(account is then in the request) or credentials from a role in the owning account (account is then in the credentials). Either way the account owning the signing credentials is the table owner whenever the name is bare.Why
ExpectedBucketOwneris sound: S3 rejects the request with 403 when it does not match the bucket owner, so on a successful response it is authoritative. The tag is applied from the response hook (v2: carried in anExecutionAttributefromonSdkRequesttoonSdkResponse, set only on a 2xx; v1: set inafterResponse, which does not run for errors), so a rejected owner is never tagged on the error span.Where the caller account comes from on SDK v2:
AwsSignerExecutionAttribute.AWS_CREDENTIALSis read inonSdkRequest, andAwsCredentialsIdentity.accountId()(added in SDK 2.26, populated by the STS, SSO, profile, process and container credential providers) is called through aMethodHandlelooked up by name per credentials class, because theAwsCredentialsIdentityinterface itself does not exist at the 2.2.0 floor the instrumentation compiles against. The handle is held in aClassValue(the sameGenericClassValuethe v1GetterAccessuses) rather than a static map, so a credentials class from an application class loader is never pinned by instrumentation state. Fatal errors (VirtualMachineError,ThreadDeath) thrown by the lookup are rethrown; anything else means the account is absent for that request. On SDKs older than 2.26 the credentials carry no account and no account is tagged for a bare name. Onlyaws_accountis added in that case; no table ARN is synthesized from it, soaws.table.arnonly ever carries a value that was in the request.SDK v1 does not expose the signing credentials to request handlers (and
HandlerContextKeydoes not exist at the 1.11.0 floor), so v1 covers the ARN andExpectedBucketOwnercases only.The tag name
aws_accountis whatdd-trace-pyalready emits for the SNS and SQS account it derives fromTopicArnandQueueUrl, and it is the dimension tag on theaws.dynamodb.*integration metrics, so a span and the metrics for the same table join without translation. It is also on the allow list of the Agent's credit card obfuscator (see below).Implementation:
aws-java-common: newAwsArnvalue type that parses an ARN once (partition, service, Region, account, resource), plusAwsAccountIdentity(twelve-digit account validation). Both are purejava.langwith no agent dependencies. The SDK's ownsoftware.amazon.awssdk.arns.Arnis not used because thearnsmodule is not a dependency ofaws-coreat the 2.2.0 floor these instrumentations compile against, and SDK v1 has no equivalent. Both SDK modules takeaws-java-commonas animplementation project(...)dependency and register the classes inhelperClassNames()so they are injected alongside the decorators, the same pattern aselasticsearch-commonandkafka-common. (Initially placed inagent-bootstrap; moved here per review, since bootstrap is loaded for every JVM and this is AWS-specific.)aws-java-sdk-2.2:onDynamoDbTablereplaces the plainsetTableNamecall;callerAccount/credentialsAccountIdread the credentials;ExpectedBucketOwnerhandled in the S3 branch.aws-java-sdk-1.11:GetterAccessgainsgetExpectedBucketOwner; the decorator handles the ARN form ofTableNameandExpectedBucketOwner.internal-api: the two tag constants. No new configuration.Motivation
aws.table.namesays which table, not whose. Two accounts can each own anorderstable in the same Region, and the span alone cannot tell them apart, which breaks any account-aware dependency map and any join to the AWS integration metrics (which are keyed by account). The ARN and the signing credentials already carry the answer; the tracer was discarding it.Compatibility: no existing tag changes value. A
TableNamegiven as an ARN is still emitted verbatim asaws.table.name,tablenameandpeer.service, as today; the ARN and account are additions. (An earlier revision replaced those with the bare table name; that was reverted since it would have changedpeer.service, and with it service maps and monitors, for callers that already pass ARNs.)Additional Notes
Testing
AwsArnTestandAwsAccountIdentityTest(aws-java-common, JUnit 5, 23 + 10 cases): ARN parsing across partitions and resource shapes, malformed ARNs, twelve-digit account validation.AwsSdkClientDecoratorCredentialsTest(aws-java-sdk-2.2, JUnit 5, 5 cases): theaccountId()lookup against credentials classes that declare the method (the suite's SDK predates it): account read; class without the method (AwsBasicCredentials2.2.0) ignored; empty and malformed values ignored; a provider that throws is treated as absent; anOutOfMemoryErrorpropagates.Aws2ClientTest(forkedTest, 37 + 37 across the V0/V1 naming forks + 23 legacy): new feature methods for an ARNTableName(asserts the exact tag set:aws.table.arnandaws_accountadded, name tags andpeer.serviceunchanged) and for a bare name with credentials that carry no account (asserts no account tags are invented).S3BucketOwnerForkedTest(JUnit 5, 4 cases, in thepayloadTaggingTestsource set because its S3 model, 2.18.40, hasExpectedBucketOwner; the base suite pins S3 2.2.0 which predates the field): owner set and accepted, owner absent, owner malformed, owner rejected with 403 (noaws_accounton the error span). Uses path-style addressing so the bucket is not resolved assomebucket.localhost, which works on hosts whose resolver maps*.localhostto loopback and not on CI runners.AWS1ClientTest(24 + 24 forked + 17 legacy): new row with an ARNTableName.spotlessApplyclean on all touched modules.How the end-to-end test was run
Publish, KinesisPutRecord, Step FunctionsStartExecution+DescribeExecution, LambdaInvoke, DynamoDBGetItem(by name and by ARN), S3GetObjectand a JDBCSELECT 1, all inside one traced method so the client spans share a trace.dd-java-agent.jarcopied into it: the released 1.66.0 tracer (before) and the jar built from this branch with./gradlew :dd-java-agent:shadowJar(after). Same node pool, sameDD_ENV, distinctDD_VERSIONso the two runs are separable in Span Search./api/v2/spans/events/search) was queried for each run's client spans and the per-span attributes compared key by key; the raw responses are retained. Where the change enables a join to the AWS integration metrics, the corresponding/api/v1/querywas run against the same org and the returned scope recorded.Live before/after on a real EKS workload
A Java 21 workload using AWS SDK v2 2.29.52 under IRSA on an EKS cluster ran
GetItemagainst a real table twice per cycle (once by bare name, once by ARN) andGetObjectwithExpectedBucketOwneragainst a real bucket, once with the stockdd-java-agent1.66.0 and once with the jar from this branch, same image, same node pool. Spans as returned by the Span Search API (account masked). Captured on an earlier revision of this branch: the current revision leavesaws.table.name/tablenameat the value given and does not synthesizeaws.table.arnfor a bare name;aws_account, which is what the join below uses, is unchanged.The join this enables, against the same org's AWS integration metrics:
Obfuscator note for reviewers
The first live run also emitted the S3 owner under a second key,
aws.bucket.owner, and it arrived at Datadog as?. The Agent's credit card obfuscator redacts any 12-digit value whose key is not on its allow list;aws_accountis on that list,aws.bucket.ownerwas not. This PR therefore emits the account underaws_accountonly. Anyone adding a new account-bearing key in the tracer should expect the same behaviour (see also DataDog/datadog-agent#56242, which addscloud.account.idandaws.account.idto that allow list).Design notes
TableNamemember (Timestream, Keyspaces, Athena) keep the pre-existing plainaws.table.name/tablenametags and never receive an account.aws.table.arnfor a bare name from the credentials account, the client Region and a Region-to-partition table; that table and the synthesized ARN are gone.aws_accountalone is what the join with the AWS integration metrics needs.aws_accountis only set where the account is provable: an ARN in the request, credentials whose account is asserted by the SDK, or an owner the caller asserted and S3 enforces. A bare S3 bucket name is deliberately not attributed to the caller, since bucket policies allow cross-account access by name.StreamNameand LambdaFunctionNamefollow the same "bare name resolves in the requestor's account" rule and could get the same treatment; left for a follow-up to keep this change reviewable.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labels — suggestedinst: aws sdk,inst: aws dynamodb,inst: aws s3,type: feature(external contributor, cannot set labels)close,fix, or any linking keywords when referencing an issuedd-java-agent/instrumentation/aws-java/(aws-java-common/andaws-java-sdk-2.2/src/payloadTaggingTest/java/), covered by the existing@DataDog/apm-serverlessdirectory entrySuggested labels (cannot set as an external contributor):
inst: aws sdk,inst: aws dynamodb,inst: aws s3,type: feature.