Collect client IP tags for AI Guard requests#11233
Conversation
smola
force-pushed
the
smola/ai-guard-client-ip-tags
branch
from
93c0a25 to
3b41551
Compare
smola
force-pushed
the
smola/ai-guard-client-ip-tags
branch
from
3b41551 to
041ae77
Compare
smola
changed the title
smola
added
type: enhancement
smola
requested review from
a team,
daniel-romano-DD,
manuel-alvarez-alvarez and
mtoffl01
and removed request for
a team
![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: 041ae77c3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
manuel-alvarez-alvarez
left a comment
manuel-alvarez-alvarez
left a comment
There was a problem hiding this comment.
LGTM, to review the Codex comment
vandonr
left a comment
vandonr
left a comment
There was a problem hiding this comment.
looks ok, needs a review from core too
| if (peerIp != null && localRootSpan.getTag(Tags.NETWORK_CLIENT_IP) == null) { | ||
| localRootSpan.setTag(Tags.NETWORK_CLIENT_IP, peerIp); | ||
| } | ||
| final String inferredClientIp = clientIpAddressData.getInferredClientIp(); | ||
| if (inferredClientIp != null && localRootSpan.getTag(Tags.HTTP_CLIENT_IP) == null) { | ||
| localRootSpan.setTag(Tags.HTTP_CLIENT_IP, inferredClientIp); | ||
| } |
There was a problem hiding this comment.
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.
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).
| // This should be enabled if: | ||
| // - DD_TRACE_CLIENT_IP_ENABLED=true, or | ||
| // - DD_APPSEC_ENABLED=true (or AppSec enabled at runtime) |
There was a problem hiding this comment.
this is kinda repeating the logic that's written below, not sure it's very useful + it risks being outdated if the condition changes
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok, makes sense, I saw it as mostly rewording the bool computation below, but it's indeed carrying more information.
smola
force-pushed
the
smola/ai-guard-client-ip-tags
branch
from
041ae77 to
25e6058
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25e6058ade
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| new AIGuard.Options().block(false) | true | false | ||
| } | ||
|
|
||
| void 'test evaluate applies captured client ip tags to local root span'() { |
There was a problem hiding this comment.
The repo-level AGENTS.md explicitly says, "Do not write new Groovy / Spock tests" and to migrate existing Groovy tests to JUnit 5 when touching them. This commit adds new Spock test methods in this Groovy spec, increasing the legacy test surface instead of following the maintained JUnit 5 convention.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Oh well, once we migrate the test class as a whole.
When DD_AI_GUARD_ENABLED=true, resolve the client IP eagerly during HTTP server request decoration and stash it on the request context. Apply the tags (http.client_ip and network.client.ip) on the local root span only when an ai_guard span is actually created, so non-AI requests of an AI-Guard-enabled service do not get IP tags. APPSEC-62199
…spans Replace getAndResetClientIpAddressData with a non-destructive getClientIpAddressData, and guard the stash write in HttpServerDecorator so that a child server span cannot overwrite IP data already resolved from the root span's proxy headers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
smola
force-pushed
the
smola/ai-guard-client-ip-tags
branch
from
25e6058 to
1cccdee
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cccdeed34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
The repo-level AGENTS.md test guideline says to always use JUnit 5 and not write new Groovy/Spock tests, migrating existing Groovy tests instead. This change adds new Spock test coverage here (and similarly in the HTTP decorator/smoke specs), so the new AI Guard client-IP coverage should be implemented as JUnit 5 tests rather than extending Groovy specs.
Useful? React with 👍 / 👎.
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
smola/ai-guard-client-ip-tags
branch
3 participants
What Does This Do
When DD_AI_GUARD_ENABLED=true, resolve the client IP eagerly during HTTP
server request decoration and stash it on the request context. Apply the
tags (http.client_ip and network.client.ip) on the local root span only
when an ai_guard span is actually created, so non-AI requests of an
AI-Guard-enabled service do not get IP tags.
APPSEC-62199
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]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.