fix(appsec): always cleanup any created asm request context managers [backport #5813 to 1.13] (#5815)

Backport of #5813 to 1.13

There is a possible code path that could cause us to not clean up the
asm request context added to the span context.

If this occurs, then we will get an error encoding because
`span.context._meta` is only meant to hold string values.

This is a short term mitigation to ensure we always cleanup the context
manager added to `span.context._meta`, but we should consider an
alternative approach which doesn't rely on `span.context._meta`.

## 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] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
are followed.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] OPTIONAL: PR description includes explicit acknowledgement of the
performance implications of the change as reported in the benchmarks PR
comment.

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.

Co-authored-by: Brett Langdon <[email protected]>

Author: emmettbutler

ddtrace/appsec/processor.py

@@ -376,23 +376,22 @@ def _is_needed(self, address):
 
     def on_span_finish(self, span):
         # type: (Span) -> None
-        if span.span_type != SpanTypes.WEB:
-            return
-
-        # Force to set respond headers at the end
-        headers_req = _context.get_item(SPAN_DATA_NAMES.RESPONSE_HEADERS_NO_COOKIES, span=span)
-        if headers_req:
-            _set_headers(span, headers_req, kind="response")
-
-        # this call is only necessary for tests or frameworks that are not using blocking
-        if span.get_tag(APPSEC.JSON) is None and _asm_request_context.in_context():
-            log.debug("metrics waf call")
-            _asm_request_context.call_waf_callback()
-
-        self._ddwaf._at_request_end()
-
-        # release asm context if it was created by the span
         asm_context = span.context._meta.get("ASM_CONTEXT_%d" % id(span), None)
-        if asm_context is not None:
-            asm_context.__exit__(None, None, None)  # type: ignore
-            del span.context._meta["ASM_CONTEXT_%d" % id(span)]
+        try:
+            if span.span_type == SpanTypes.WEB:
+                # Force to set respond headers at the end
+                headers_req = _context.get_item(SPAN_DATA_NAMES.RESPONSE_HEADERS_NO_COOKIES, span=span)
+                if headers_req:
+                    _set_headers(span, headers_req, kind="response")
+
+                # this call is only necessary for tests or frameworks that are not using blocking
+                if span.get_tag(APPSEC.JSON) is None and _asm_request_context.in_context():
+                    log.debug("metrics waf call")
+                    _asm_request_context.call_waf_callback()
+
+                self._ddwaf._at_request_end()
+        finally:
+            # release asm context if it was created by the span
+            if asm_context is not None:
+                asm_context.__exit__(None, None, None)  # type: ignore
+                del span.context._meta["ASM_CONTEXT_%d" % id(span)]

releasenotes/notes/fix-appsec-request-context-cleanup-afd26509cfbf2cc5.yaml

@@ -0,0 +1,3 @@
+fixes:
+  - |
+    appsec: Fixes an encoding error when we are unable to cleanup the AppSec request context associated with a span.

tests/appsec/test_processor.py

@@ -652,3 +652,18 @@ def test_ddwaf_run_contained_oserror(tracer_appsec, caplog):
 
     assert span.get_tag(APPSEC.JSON) is None
     assert "OSError: ddwaf run failed" in caplog.text
+
+
+def test_asm_context_meta(tracer_appsec):
+    tracer = tracer_appsec
+
+    # For a web type span, a context manager is added, but then removed
+    with tracer.trace("test", span_type=SpanTypes.WEB) as span:
+        assert span.context._meta["ASM_CONTEXT_%d" % (id(span),)]
+    assert span.context._meta.get("ASM_CONTEXT_%d" % (id(span),)) is None
+
+    # Regression test, if the span type changes after being created, we always removed
+    with tracer.trace("test", span_type=SpanTypes.WEB) as span:
+        span.span_type = SpanTypes.HTTP
+        assert span.context._meta["ASM_CONTEXT_%d" % (id(span),)]
+    assert span.context._meta.get("ASM_CONTEXT_%d" % (id(span),)) is None