[golang] Add apim weblog variant for azure/apim-callout - #7516
Open
eliottness wants to merge 9 commits into
Open
[golang] Add apim weblog variant for azure/apim-callout#7516eliottness wants to merge 9 commits into
eliottness wants to merge 9 commits into
Conversation
Add a Go weblog variant `apim` exercising
dd-trace-go/contrib/azure/apim-callout, at activation parity with `envoy`.
Azure APIM has no runnable container, so the gateway role is filled by a
system-tests-owned, stdlib-only Go shim that reimplements APIM `send-request`
behavior. It never parses policy XML.
test runner --> apim-gateway (shim, :80 -> host 127.0.0.1:7777)
|-> apim-callout (POST :8080, health :8081)
`-> http-app (:8080)
The shim defaults to the 4-call flow (deferred bodies). A control header
`X-Datadog-Apim-Body-Mode: inline` switches both sides to inline delivery,
which suppresses `allowed-body-size` and collapses the exchange to 2 calls.
The header is stripped before building the callout payload and before
forwarding upstream. The shared suite never sends it.
Unlike the APIM policy's ignore-error="true", the shim fails closed (502 +
stderr) on detectable transport/protocol failure, so system-tests can assert
on it. The callout's HTTP 200 `{}` for processor errors is indistinguishable
from a legitimate phase-2/4 success, so the one available check is requiring
a non-empty request-id in the phase-1 response.
All 89 `envoy:` manifest rows are widened to `envoy, apim:`, transcribing
enabled rows as enabled. Widening all 89 rather than only the non-redundant
ones is required, not just harmless: 29 rows have a `"*"` sibling and in every
one the `"*"` value differs from envoy's, so for 13 of them the fallback would
have given apim an outright wrong declaration.
`containers.py:998` also gains `apim`, otherwise get_image_list opens a
nonexistent apim.Dockerfile and breaks get-image-list.py while every
acceptance criterion still passes.
Cover the apim shim's two body modes with system-tests: - default (deferred) drives 4 callout calls sharing one request-id - inline drives exactly 2 - span structure and component are identical in both modes - inline mode closes cached state, so no orphaned-span warning appears in apim-callout stderr after the 30s TTL The gateway now logs the request path alongside the phase and request-id. Without it these tests cannot isolate their own traffic: the gateway healthcheck is a bodiless GET, which is deferred mode, so it drives a 4-call flow of its own and the container only reports healthy once such a request succeeds. Asserting merely that "some request-id somewhere made 4 calls" therefore could not fail. Each test now uses a unique probe path and asserts exactly one correlated request-id group for it. The path is logged to stderr rather than returned as a response header so it stays invisible to the WAF, the upstream and the client.
AC1 measured ~11.93s from container start to the first successful health probe. Replace the provisional 153s ceiling with ~21s (15s start_period + 6 * 1s), keeping a cushion because `go run .` still compiles the shim from an empty build cache on every container start.
…p, cuts Fail-closed paths now emit a diagnostic. D3 requires "502 + stderr", but failClosed discarded the error, so 11 paths returned a bare 502 with nothing in the log: request/upstream body reads, inline and deferred body encodes, the upstream call, and block writes at all four phases. Each now logs `apim-gateway fail-closed stage=<stage> error=...`. Callout-error sites use a separate non-logging variant, since callout() already logged them and a single logging helper would double-log. Adds tests for the upstream-closed-port and malformed-block paths, and tightens the existing closed-port test to assert an exact line count rather than a substring, which previously tolerated the very double-logging this splits apart. Record the span-links gap for apim. apim-callout defaults DD_TRACE_PROPAGATION_STYLE to datadog, so a conflicting W3C traceparent is never extracted and no terminated-context span link is created. Envoy's processor leaves the variable unset and gets the tracer default, which is why it passes. haproxy-spoa ships the same datadog-only default and is already missing_feature on this row, so apim is marked beside it rather than having the harness override a shipped default to force a pass. Drop three pieces of dead or gold-plated code: - calloutMessage.Gateway was never assigned. With omitempty and no writer it could not serialize, so removal is wire-neutral, and it was the footgun for the rule against sending a gateway at all. - decodeJSON's trailing-JSON rejection was untested and guarded nothing: the first Decode had already produced the correct result. It only manufactured a 502 the real callout cannot trigger, adding a third candidate cause to a shim whose main risk is undiagnosable failures. The two sibling guards stay, because each converts a handler panic into a clean 502. - responseStatus() reflected over fixture JSON and panicked to recover values already present as literals; replaced by a wantStatus field. Also drops the four manifest entries for the new tests. The @Irrelevant decorator already produces the "*" outcome, and the sibling test_apm.py has no manifest rows at all, so the entries duplicated activation across two mechanisms and the version floor was unreachable.
allowed-body-size is a pointer, and a returned 0 is a limit rather than an absence, so both body phases must still fire with a fully truncated body. Every existing fixture returned a positive size, so narrowing the check from "is set" to "is positive" left the whole suite green -- the shim's single most important semantic rule was unguarded. Adds a four-phase fixture returning 0 on both header phases, asserting the callout receives an empty body while upstream and the client still receive the untruncated payloads. Under a mutation to a positive-value check, every pre-existing test still passes and only this one fails, which is what makes it worth having. Also covers the negative-size guard. Deleting it panics with slice bounds out of range [:-1] at the body[:*limit] site, so the guard's rationale is now verified rather than asserted; the shim returns 502 with a fail-closed diagnostic instead.
Contributor
|
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: ec42fca | Docs | Datadog PR Page | Give us feedback! |
The apim end-to-end jobs hung at "Pull images" while every sibling golang
variant passed. .github/actions/pull_images redirects get-image-list.py's
stdout into compose.yaml and feeds that to `docker compose`, and container
objects are constructed during that call -- so ApimCalloutContainer's
logger.stdout() lines landed inside the compose document as a bogus top-level
key:
{'WARNING': 'binaries/golang-apim-callout-image not found...',
'services': {}}
Both branches logged, which is why both the prod job (fallback) and the dev
job (pointer found) hung. The sibling processor containers are unaffected only
because they swallow the same branch silently.
Switch to logger.info/logger.warning. The resolved tag still reaches stdout on
a real run via GoProcessorContainer.post_start, which prints
"Processor image: <name>", so the released-vs-commit-under-test distinction
stays visible where it matters.
Adds tests/test_the_test/test_get_image_list.py asserting that the script's
stdout parses as a compose document whose only top-level key is `services`,
parametrised over all three go-proxy weblogs and over both pointer states.
Reintroducing either logger.stdout call fails it. It deliberately does not
pin services == {}, since a pointer to an image absent from the local cache
legitimately adds an entry.
The deferred probes asserted four callout phases and failed in CI with three: <ResponseBody> never fired. The gateway only makes that call when the <ResponseHeaders> callout answers with allowed-body-size, and the callout only asks for a response body it can parse. The stock http-app (jasonrm/dummy-server) has a single 34-line handler that answers every request with the status code as text/plain, so there is no request that can make it return a parseable body -- pointing the probes elsewhere is not an option. Across both CI jobs, 1164 requests produced zero <ResponseBody> phases under two different callout versions, while the 20 <RequestBody> phases line up exactly with the JSON-body POSTs, so the request side works and the response side is upstream-driven. Local validation missed this because the amd64-only http-app cannot run on arm64 and was substituted with a JSON-returning stand-in, which makes the fourth phase fire. The comment on UPSTREAM_DEPENDENT_PHASE says so, to stop someone re-requiring it after validating the same way. So the deferred assertion now requires the three guaranteed phases and tolerates a single trailing <ResponseBody>. What matters is preserved: the deferred flow must contain <RequestBody> and the inline flow must not, which is the distinction these tests exist to prove. Renames the test and probe path that claimed four calls.
eliottness
marked this pull request as ready for review
August 14, 2026 14:05
![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: fe965689a2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The health budget was tightened to a ~21s ceiling off a single 11.93s measurement, which was too aggressive. `go run .` rebuilds net/http and crypto/tls from source into an empty GOCACHE on every container start -- ~12s wall but ~13s of CPU locally -- so a contended 2-vCPU runner can exceed 21s, and the failure mode is a dead scenario rather than a slow one. execute_command stops at the first success, so a high retry count costs nothing when the container is already up. It also treats start_period as an unconditional sleep rather than a Docker grace period, so dropping it makes a fast start detected sooner than the previous blind 15s wait. Every other container in the file uses retries 10-60 with no start_period; 5 was the lowest value present. Also route block headers through applyHeaders. It already canonicalizes, and writeBlock was the one place writing callout-supplied header names verbatim. Not currently reachable, since dd-trace-go builds those headers via Header().Set and they arrive canonical, but the failure mode is quiet: a lowercase content-type would be missed by Go's internal Header.get, so the server would content-sniff and emit a second Content-Type.
pr-review.mdc forbids time.sleep() inside setup_ methods in end-to-end scenarios, and the 31s sleep also added that much unconditional wall time to every DEFAULT run. The wait was unnecessary. Every setup_ runs before any test_, and the containers are stopped and their logs collected in between, so far more than the callout's 30s state TTL has already elapsed by the time the assertion reads the log. Also scope the assertion to the probe's own request-id, which _assert_inline_probe_phases already computed and discarded. Matching the bare warning text would fail on any unrelated request that legitimately orphaned state and blame inline mode for it.
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
1 participant
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.
Motivation
dd-trace-goships an Azure API Management integration (contrib/azure/apim-callout) that had no system-tests coverage. The other two Go proxy integrations, Envoy and HAProxy, each have a weblog variant; APIM did not, so the callout's behaviour was only covered by that repo's own unit tests.The obstacle is that Azure APIM has no runnable container — it is a managed Azure service. There is nothing to
docker runas the gateway.Changes
A new Go weblog variant
apim, at activation parity withenvoy.Since APIM itself cannot run locally, the gateway role is filled by a small system-tests-owned Go shim that reimplements APIM
send-requestbehaviour. It deliberately does not parse policy XML — it reproduces what the policy does:The shim is stdlib-only, has no
requireblock, and adds no configuration surface beyond a single control header.Body delivery modes. The callout supports two, and testing only one would leave half its body-handling code dark. Default is the deferred 4-call flow.
X-Datadog-Apim-Body-Mode: inlineswitches both sides to inline delivery, which suppressesallowed-body-sizeand collapses the exchange to 2 calls. The header is stripped before the callout payload and before the upstream forward, so it never reaches the WAF or the app, and the shared suite never sends it.Fail-closed, deliberately unlike APIM. The real policy uses
ignore-error="true". The shim instead returns 502 plus a stderr diagnostic on detectable failure, so system-tests can assert on it. One case is genuinely undetectable and documented as such: the callout returns HTTP 200{}both for processor errors and as a legitimate phase-2/4 success.Manifest. All 89
envoy:rows are widened toenvoy, apim:, transcribing enabled rows as enabled. Widening all 89 rather than only the non-redundant ones is necessary, not merely tidy: 29 rows have a"*"sibling and in every one the"*"value differs from envoy's, so for 13 of them the fallback would have handedapiman outright wrong declaration.One row is declared
missing_featureforapimrather than enabled —test_span_links_from_conflicting_contexts. This is a real library gap, not a shim defect:apim-calloutdefaultsDD_TRACE_PROPAGATION_STYLEtodatadog, so a conflicting W3Ctraceparentis never extracted and no terminated-context span link is created.haproxy-spoaships the identical default and is alreadymissing_featureon that exact row; envoy's processor leaves the variable unset and gets the tracer default, which is why it passes. Recording the gap was preferred over overriding the variable in the harness, which would test a configuration no default deployment runs.containers.py:998also gainsapim. Without itget_image_listtries to open a nonexistentapim.Dockerfileand breaksget-image-list.pywhile every test still passes — a silent failure.CI
apimis picked up automatically bycompute-workflow-parameters.py(verified: it appears inendtoend_defs_parallel_jobswithweblog_build_required: falseand scenarios[APPSEC_BLOCKING, DEFAULT]), so no workflow changes were needed. Likeenvoyandhaproxy, it uses the:latestcallout image in normal PR CI and:devin_system_tests_dev_mode, whereload-binary.shwrites the pointer.Validation
Both scenarios were run locally against
apim-callout:dev(golang@2.11.0-dev.1): DEFAULT128 passedand APPSEC_BLOCKING50 passed, with zero enabled-row failures. Also verified by hand: a live 403 WAF block through the shim, inline yielding exactly 2 callout calls and deferred exactly 4 (each sharing onerequest-id), and the fail-closed 502 path.Note a bodiless GET correctly produces 3 calls, not 4 — the callout only returns
allowed-body-sizewhen it actually wants a request body.One caveat:
DummyServerContainerhardcodes an amd64-only image, so local validation on arm64 required substituting a nativehttp-app. This affectsenvoyandhaproxyidentically and is pre-existing; amd64 CI exercises the stock topology.Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teamutils/_context/containers.py, theGoProxyWeblogsunion and dispatch inutils/_context/weblog_infrastructure.py, thecontainers.py:998image-list guard, plusutils/scripts/load-binary.shand the new shim underutils/build/docker/golang/apim/.build-XXX-imagelabel is presentbuild_mode: noneand runsgolang:1.25-alpine(already mirrored) with the shim bind-mounted read-only.apimis restricted to the existingDEFAULTandAPPSEC_BLOCKING.