Repository navigation
DO NOT MERGE: preview of #5827 with go-libddwaf#239 - #5839
Draft
RomainMuller wants to merge 9 commits into
Draft
RomainMuller wants to merge 9 commits into
RomainMuller wants to merge 9 commits into
Conversation
A WAF or RASP timeout is not a WAF error. Other tracers do not count it
as one either. `IncWafError` now skips `waf.error`, `rasp.error` and the
error-code atomics for a timeout. Timeouts stay reported by
`waf.requests{waf_timeout:true}`, `rasp.timeout` and the
`_dd.appsec.waf.timeouts` / `_dd.appsec.rasp.timeout` span tags.
Invalid input (unknown RASP rule type, unexpected scope) is still
reported as an instrumentation exception, also for a timeout.
The `waf_error` tag of `waf.error` / `rasp.error` and the span tags `_dd.appsec.waf.error` / `_dd.appsec.rasp.error` held the positive go-libddwaf `RunError` values (1 to 6). RFC-1012 requires the `ddwaf_run` return code instead. `wafRunErrorCode` maps `ErrInternal` to -3, `ErrInvalidObject` to -2 and `ErrInvalidArgument` to -1. Any other error maps to -127 (error from the bindings). It compares with the named `RunError` constants, so a renumbering in go-libddwaf cannot change the result. `updateClosestToZero` now ignores a code >= 0. Before, a stored positive value was never replaced by a negative one.
Describe the timeout rule, the ddwaf_run error code mapping, the least-negative code rule and the conditions of the error span tags.
Resolve the conflicts with the timeout rework of #5828: - keep main's IncWafError, and the A2 code of this branch - keep main's TestIncWafErrorTimeout and the shared test helpers, and drop the tests of this branch that duplicate them - update the tests of #5828 that assert the old positive error codes
Replace the section with the A2 content only: timeouts are handled by #5828, so the section no longer describes them.
Use the head of DataDog/go-libddwaf#239 through a pseudo-version, in the root module only. This commit never lands.
`waferrors.ToWafReturnCode` (go-libddwaf#239) returns the ddwaf_run return code. Remove the copy of the mapping (`wafRunErrorCode` and its 4 constants). Keep `defaultWafErrorCode` (-127): it is the RFC-1012 code of an error that has no ddwaf_run return code. No module file changes in this commit.
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 29 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 96d28a7 | Docs | View more details | Give us feedback! |
Use the head of DataDog/go-libddwaf#239 through a pseudo-version, in the root module only. This commit never lands.
`waferrors.ToWafReturnCode` (go-libddwaf#239) returns the code to report: the ddwaf_run return code, -127 for any other error, and 0 when there is nothing to report (nil, or only timeouts). Use it as the only source of the code: - `IncWafError` computes the code once and returns it. `runWAF` re-uses it for the `waf_error` milestone of `waf.requests`. - A timeout never hides a real error: an error joined with a timeout is reported with its code, and a tree that holds only timeouts is not. - Remove `defaultWafErrorCode` and the fallback in `wafError` and `raspError`: they receive the code. No module file changes in this commit.
1 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE. This draft PR is evidence for #5827 and DataDog/go-libddwaf#239. It is never merged and will be closed when #5827 uses the released go-libddwaf
v5.1.0.What is this?
It is PR #5827 (head
0c151adb1) plus 4 commits. Only the last 4 commits are new; the other commits are the ones of #5827.chore: preview go-libddwaf 5f1d8775d— superseded by commit 3.refactor(appsec): use waferrors.ToWafReturnCode for the waf_error code— the first version of the code change (plan v4). Superseded by commit 4.chore: preview go-libddwaf 4b08dee9e— pseudo-version of the head of feat(waferrors): add ToWafReturnCode go-libddwaf#239 (v5.0.2-0.20261109122137-4b08dee9e9f7), in the rootgo.mod/go.sumonly. This commit never lands (noreplace; only a pseudo-version, so CI can fetch it).refactor(appsec): take the waf_error code from ToWafReturnCode— the code change that the final update of fix(appsec): report ddwaf_run error codes in waf_error #5827 re-uses. It has no module file.Why: Eliott asked in the review of #5827 whether the mapping should be in go-libddwaf. This PR shows the result before go-libddwaf releases
v5.1.0. The AppSec CI jobs run on this PR with the go-libddwaf change. Module-consistency checks can fail here, because only the root module has the pseudo-version.What
ToWafReturnCodedoes now (go-libddwaf#239)It returns the code to report:
-3/-2/-1forErrInternal/ErrInvalidObject/ErrInvalidArgument,-127for any other error, and0when there is nothing to report (nil, or only timeouts). A timeout gives no code and never hides another error. It looks at the whole error tree and returns the highest code. So dd-trace-go:IncWafErrorand returns it.runWAFre-uses it for thewaf_errortag ofwaf.requests;Join(ErrTimeout, ErrInternal)gives-3), and does not report a tree that holds only timeouts (also an error that only has anAsmethod givingErrTimeout);-127fallback and no copy of the mapping any more.Before and after (PR #5827 against
maincc0eefe02, measured on the files)0c151adb1)229e2f75f)metrics.go, code lines (no comments, no blank lines)metrics.go, all linesrun.go, code linesmainhas 1)RunError→ code mapping in dd-trace-gowafError+raspError(main: 2 × 4)IncWafError)TestToWafReturnCode(12 cases)TestToWafReturnCode(38 cases), order test, round-trip testThe whole-PR count grows because of the new regression tests (
as-timeout, joined errors, a test that the code is computed once). The production code shrinks.The
waf_errorvalues of every error that go-libddwaf returns today do not change from #5827:ErrInternal→-3,ErrInvalidObject→-2,ErrInvalidArgument→-1, any other error →-127.