Skip to content

Commit 372b28d

Browse files
fix(asm): clear ddwaf contexts on span finish [backport 2.6] (#8728)
Backport 68ec70f from #8724 to 2.6. ## Description Since the ddwaf ctx is passed to a stored callback and passed to a dynamic library (libddwaf), it's not cleared when the span finishes, instead is later collected by the Python GC, which sometimes can cause some semi-random latency increases when many objects have to be cleared at the end of a request. This change will make the ddwaf context to always be cleared on the `on_span_finish` method of the `AppSecSpanProcessor`. ## Checklist - [X] Change(s) are motivated and described in the PR description - [X] Testing strategy is described if automated tests are not included in the PR - [X] Risks are described (performance impact, potential for breakage, maintainability) - [X] Change is maintainable (easy to change, telemetry, documentation) - [X] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [X] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [X] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [X] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [X] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Juanjo Alvarez Martinez <[email protected]>
1 parent e6ab681 commit 372b28d

File tree

3 files changed

+37
-1
lines changed

3 files changed

+37
-1
lines changed

ddtrace/appsec/_processor.py

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from typing import Set
1313
from typing import Tuple
1414
from typing import Union
15+
import weakref
1516

1617
from ddtrace.appsec import _asm_request_context
1718
from ddtrace.appsec._capabilities import _appsec_rc_file_is_not_static
@@ -126,6 +127,7 @@ class AppSecSpanProcessor(SpanProcessor):
126127
)
127128
_addresses_to_keep: Set[str] = dataclasses.field(default_factory=set)
128129
_rate_limiter: RateLimiter = dataclasses.field(default_factory=_get_rate_limiter)
130+
_span_to_waf_ctx: weakref.WeakKeyDictionary = dataclasses.field(default_factory=weakref.WeakKeyDictionary)
129131

130132
@property
131133
def enabled(self):
@@ -220,7 +222,7 @@ def on_span_start(self, span: Span) -> None:
220222
_asm_request_context.register(span, new_asm_context)
221223

222224
ctx = self._ddwaf._at_request_start()
223-
225+
self._span_to_waf_ctx[span] = ctx
224226
peer_ip = _asm_request_context.get_ip()
225227
headers = _asm_request_context.get_headers()
226228
headers_case_sensitive = _asm_request_context.get_headers_case_sensitive()
@@ -407,3 +409,19 @@ def on_span_finish(self, span: Span) -> None:
407409
finally:
408410
# release asm context if it was created by the span
409411
_asm_request_context.unregister(span)
412+
413+
if span.span_type != SpanTypes.WEB:
414+
return
415+
416+
to_delete = []
417+
for iterspan, ctx in self._span_to_waf_ctx.items():
418+
# delete all the ddwaf ctxs associated with this span or finished or deleted ones
419+
if iterspan == span or iterspan.finished:
420+
# so we don't change the dictionary size on iteration
421+
to_delete.append(iterspan)
422+
423+
for s in to_delete:
424+
try:
425+
del self._span_to_waf_ctx[s]
426+
except Exception: # nosec B110
427+
pass
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
fixes:
3+
- |
4+
ASM: always clear the DDWaf context at the end of the span to avoid gc-induced latency spikes at the end of some requests.
5+

tests/appsec/appsec/test_processor.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,19 @@ def test_enable_custom_rules():
7777
assert processor.rules == rules.RULES_GOOD_PATH
7878

7979

80+
def test_ddwaf_ctx(tracer_appsec):
81+
tracer = tracer_appsec
82+
83+
with override_env(dict(DD_APPSEC_RULES=rules.RULES_GOOD_PATH)):
84+
with _asm_request_context.asm_request_context_manager(), tracer.trace("test", span_type=SpanTypes.WEB) as span:
85+
processor = AppSecSpanProcessor()
86+
processor.on_span_start(span)
87+
ctx = processor._span_to_waf_ctx.get(span)
88+
assert ctx
89+
processor.on_span_finish(span)
90+
assert span not in processor._span_to_waf_ctx
91+
92+
8093
@pytest.mark.parametrize("rule,exc", [(rules.RULES_MISSING_PATH, IOError), (rules.RULES_BAD_PATH, ValueError)])
8194
def test_enable_bad_rules(rule, exc, tracer):
8295
# with override_env(dict(DD_APPSEC_RULES=rule)):

0 commit comments

Comments
 (0)