-
Notifications
You must be signed in to change notification settings - Fork 333
Collect client IP tags for AI Guard requests #11233
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentTracer; | ||
| import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData; | ||
| import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; | ||
| import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; | ||
| import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities; | ||
|
|
@@ -248,10 +249,31 @@ public AgentSpan onRequest( | |
| } | ||
|
|
||
| AgentSpanContext.Extracted extracted = getExtractedSpanContext(parentContext); | ||
| boolean clientIpResolverEnabled = | ||
| // Whether to attach IP tags to all requests or not. | ||
| // This should be enabled if: | ||
| // - DD_TRACE_CLIENT_IP_ENABLED=true, or | ||
| // - DD_APPSEC_ENABLED=true (or AppSec enabled at runtime) | ||
|
Comment on lines
+253
to
+255
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is kinda repeating the logic that's written below, not sure it's very useful + it risks being outdated if the condition changes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic below is scattered across ~3 separate code blocks intermingled with other stuff. At least for me it was quite hard to follow every subtle detail about what gets collected when, and why. The whole logic for IP tagging should probably be splitted to a separate function so that it's grouped and fits once screen (material for another PR I guess). But until then, I would rather keep some consolidated clarification about what's going on.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, makes sense, I saw it as mostly rewording the bool computation below, but it's indeed carrying more information. |
||
| // This applies to tags: | ||
| // - http.client_ip (IP resolved from proxy tags) | ||
| // - network.client.ip (peer IP) | ||
| // - tags with proxy header values | ||
| // For backwards compatibility, it does not apply to: | ||
| // - peer.ipv4 | ||
| // - peer.ipv6 | ||
| final boolean shouldTagIps = | ||
| config.isClientIpEnabled() || traceClientIpResolverEnabled && APPSEC_ACTIVE; | ||
| // Whether to stash IP data for later tagging or not. | ||
| // AI Guard requires client IP tags on the local root span when an ai_guard span is created. | ||
| // Resolve the IPs eagerly but do not tag the span yet; stash them on the request context so | ||
| // AIGuardInternal can apply them lazily, only on requests that actually create an ai_guard | ||
| // span. | ||
| final boolean shouldStashIps = | ||
| !shouldTagIps && traceClientIpResolverEnabled && config.isAiGuardEnabled(); | ||
|
Comment on lines
+270
to
+271
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the AI Guard-only configuration ( Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed this is a bug. I'll work on a fix.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed this was a bug, subtly hidden in the smoke test. Fixed the smoke test, and fixed this in |
||
| // Whether to resolve client IP based on proxy headers or no. | ||
| final boolean shouldResolveIp = shouldTagIps || shouldStashIps; | ||
|
|
||
| if (extracted != null) { | ||
| if (clientIpResolverEnabled) { | ||
| if (shouldTagIps) { | ||
| String forwarded = extracted.getForwarded(); | ||
| if (forwarded != null) { | ||
| span.setTag(Tags.HTTP_FORWARDED, forwarded); | ||
|
|
@@ -332,7 +354,7 @@ public AgentSpan onRequest( | |
| } | ||
|
|
||
| String inferredAddressStr = null; | ||
| if (clientIpResolverEnabled && extracted != null) { | ||
| if (shouldResolveIp && extracted != null) { | ||
| InetAddress inferredAddress = ClientIpAddressResolver.resolve(extracted, span); | ||
| // the peer address should be used if: | ||
| // 1. the headers yield nothing, regardless of whether it is public or not | ||
|
|
@@ -349,9 +371,11 @@ public AgentSpan onRequest( | |
| } | ||
| if (inferredAddress != null) { | ||
| inferredAddressStr = inferredAddress.getHostAddress(); | ||
| span.setTag(Tags.HTTP_CLIENT_IP, inferredAddressStr); | ||
| if (shouldTagIps) { | ||
| span.setTag(Tags.HTTP_CLIENT_IP, inferredAddressStr); | ||
| } | ||
| } | ||
| } else if (clientIpResolverEnabled && span.getLocalRootSpan() != span) { | ||
| } else if (shouldTagIps && span.getLocalRootSpan() != span) { | ||
| // in this case extracted == null | ||
| // If there is no extracted we can't do anything but use the peer addr. | ||
| // Additionally, extracted == null arises on subspans for which the resolution | ||
|
|
@@ -370,10 +394,16 @@ public AgentSpan onRequest( | |
| } else { | ||
| span.setTag(Tags.PEER_HOST_IPV4, peerIp); | ||
| } | ||
| if (clientIpResolverEnabled) { | ||
| if (shouldTagIps) { | ||
| span.setTag(Tags.NETWORK_CLIENT_IP, peerIp); | ||
| } | ||
| } | ||
| if (shouldStashIps && (peerIp != null || inferredAddressStr != null)) { | ||
| RequestContext requestContext = span.getRequestContext(); | ||
| if (requestContext != null) { | ||
| requestContext.setClientIpAddressData(new ClientIpAddressData(peerIp, inferredAddressStr)); | ||
| } | ||
| } | ||
| setPeerPort(span, peerPort); | ||
| Flow<Void> flow = callIGCallbackAddressAndPort(span, peerIp, peerPort, inferredAddressStr); | ||
| if (flow.getAction() instanceof RequestBlockingAction) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's frequent enough that both tags are already set on the root span, it could make sense to check that before unfolding the request context ? maybe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tags would generally not be present in the root span. In the normal path, if the http decorator is tagging these for all requests, then the IPs would not be stashed for later, and this function would see
requestContext.getAndResetClientIpAddressData() == null).