diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 11a647e2..7b862118 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -24,11 +24,34 @@ jobs: # Only run on merged PRs, not just closed ones if: github.event_name != 'pull_request' || github.event.pull_request.merged == true # Add timeout to prevent long-running jobs (increased for integration benchmarks). - # Raised 45 -> 75 for the k6 aspect matrix: the lane went from three wired 60-second runs to - # eight (the six matrix aspects plus the two retained health benchmarks). The dominant - # contributor beyond the old budget is the upload-50MB aspect, which is transfer-bound and - # runs at reduced concurrency, so it does not finish in the same wall time as a request-rate - # run. The five additional runs land inside this cap. + # The -Pbenchmark profile DECLARES TWELVE k6 goals: the eight cross-gateway matrix aspects, the + # two retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two + # API-Sheriff-only passthrough-relay executions (mapped relay throughput and the empty-mode + # no-regression run). TEN of them execute: uploadLarge and websocketEcho each carry a + # bound to a property that defaults to true in benchmarks/pom.xml, because each is blocked by an + # open gateway defect (see the "Deliberately skipped" table in the coverage step below). Maven + # therefore never runs them, so they consume no wall time and cannot abort the suite. Both are + # still ordered last in the profile as defense-in-depth, so that flipping a skip property to + # false cannot mask a goal that would otherwise produce a result. + # + # Budget, from per-goal timings measured locally 2026-07-28 rather than estimated: + # * goal execution ~12 min -- sized on the TEN goals that actually execute, which is now the + # real basis: a skipped goal is not run at all, so it costs nothing but the + # Maven bookkeeping for a skipped execution. + # 10 goals x (60s k6 window + ~10s compose/k6 start and summary write). + # Re-enabling a skipped goal costs another ~70s, and upload-50MB more than + # that: it is transfer-bound at reduced concurrency, so it does not finish + # in the same wall time as a request-rate run. Re-derive this term when a + # skip property is flipped to false. + # * stack startup ~90 s -- since #118 the lane boots FOUR native gateway instances + # (api-sheriff, api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2) + # alongside Keycloak, go-httpbin, nginx-static, passthrough-backend, + # grpc-echo, toxiproxy, asset-origin and prometheus, because + # start-integration-container.sh runs a bare `up -d`. + # * native compile the dominant and most variable term on a cold cache. + # + # 75 minutes holds with headroom for the cold native compile; it is kept unchanged because the + # measured run does not approach the cap. timeout-minutes: 75 permissions: # Needed to upload artifacts @@ -84,11 +107,18 @@ jobs: --output-dir "${GITHUB_WORKSPACE}/benchmark-history" - name: Run Integration Benchmarks with k6 + id: run-benchmarks run: | - # Run k6-based integration benchmarks with native image. The -Pbenchmark profile wires - # eight executions: the six matrix aspects (unauth, bearer, http2, graphql, upload-1MB, - # upload-50MB) plus the two retained non-matrix health benchmarks (healthLiveCheck, - # gatewayHealth). The on-demand APISIX comparison lane is deliberately NOT run here. + # Run k6-based integration benchmarks with native image. The -Pbenchmark profile declares + # twelve goals: the eight cross-gateway matrix aspects (unauth, bearer, http2, + # graphql, upload-1MB, upload-50MB, ws, grpc), the two retained non-matrix health + # benchmarks (healthLiveCheck, gatewayHealth), and the two API-Sheriff-only + # passthrough-relay executions (mapped and empty). Ten of the twelve execute and are + # expected to produce a CI result; upload-50MB (uploadLarge) and ws (websocketEcho) are + # skipped by property against open gateway defects, so Maven does not run them and they + # cannot abort the suite. The on-demand APISIX comparison lane is deliberately NOT run + # here. Maven still fails fast on the goals that do run, so the coverage step below + # remains what proves which goals actually produced a result. echo "Running k6 integration benchmarks with native Quarkus..." ./mvnw --no-transfer-progress clean verify -pl benchmarks -Pbenchmark \ -Dbenchmark.history.dir="${GITHUB_WORKSPACE}/benchmark-history/integration" @@ -97,6 +127,81 @@ jobs: echo "Integration benchmark artifacts generated:" ls -la benchmarks/target/benchmark-results/ + # Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never + # reached leaves no trace in the job output — a suite that ran 3 of the 10 executing goals + # renders exactly like one that ran all 10. This step diffs the goals that MUST produce a + # summary document against the documents k6 actually wrote, so a partial suite is visibly + # partial and a goal that silently vanished fails the job. When Maven failed, it also states + # in words whether the suite was truncated by that failure. Skipped goals are named with their + # reason and their skip property rather than omitted, so "not run" is never + # indistinguishable from "forgotten". + - name: Summarise benchmark coverage + if: always() + env: + BENCHMARK_OUTCOME: ${{ steps.run-benchmarks.outcome }} + run: | + set -uo pipefail + results_dir=benchmarks/target/benchmark-results/k6 + + # Goals expected to write a summary. A goal settled as deliberately skipped is NOT listed + # here — it belongs in the skip table below, which is what keeps the two states distinct. + expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall grpcUnary" + + missing=0 + missing_names="" + { + echo "## Benchmark coverage" + echo + echo "| Benchmark | Result |" + echo "| --- | --- |" + } >> "$GITHUB_STEP_SUMMARY" + + for name in ${expected}; do + if [ -f "${results_dir}/${name}-summary.json" ]; then + echo "| \`${name}\` | ran |" >> "$GITHUB_STEP_SUMMARY" + else + echo "| \`${name}\` | **DID NOT RUN** |" >> "$GITHUB_STEP_SUMMARY" + missing=$((missing + 1)) + missing_names="${missing_names:+${missing_names}, }${name}" + fi + done + + { + echo + echo "### Deliberately skipped" + echo + echo "| Benchmark | Skipped by | Reason |" + echo "| --- | --- | --- |" + echo "| \`uploadLarge\` | \`skip.benchmark.upload.large\` (defaults to \`true\`) | The gateway rejects the 50MB body with 413: the Quarkus HTTP body limit is not derived from the upload anchor's 64 MiB \`max_body_bytes\`, so the declared cap is unreachable. Needs a gateway-side fix; re-enable with \`-Dskip.benchmark.upload.large=false\`. |" + echo "| \`websocketEcho\` | \`skip.benchmark.websocket.echo\` (defaults to \`true\`) | Each WebSocket upgrade leaks an edge admission permit; once exhausted the gateway rejects all traffic with 503 until restarted, which would also poison every goal after it. Needs a gateway-side fix; re-enable with \`-Dskip.benchmark.websocket.echo=false\`. |" + echo "| \`sessionMediated\` | not wired | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" + } >> "$GITHUB_STEP_SUMMARY" + + # Only fail on a missing summary when Maven itself reported success: when the benchmark + # step already failed, its own error is the actionable signal and this step must not + # mask it with a derived one. + if [ "${missing}" -gt 0 ] && [ "${BENCHMARK_OUTCOME}" = "success" ]; then + echo "::error::${missing} expected benchmark summary document(s) absent although the benchmark step succeeded." + exit 1 + fi + + # When Maven failed, the DID NOT RUN rows above and the Maven error are almost always the + # same event, but nothing on the page says so — a reader is left to infer the causal link. + # State it. This path never exits non-zero: the benchmark step's own failure already fails + # the job, and duplicating that here would only bury the real error. + if [ "${BENCHMARK_OUTCOME}" != "success" ]; then + { + echo + if [ "${missing}" -gt 0 ]; then + echo "> **Maven failed and the suite was TRUNCATED**: goals ${missing_names} produced no summary because the run aborted before reaching them. The DID NOT RUN rows above are a consequence of that failure, not ${missing} independent problems — fix the reported Maven error first, then re-read this table." + else + echo "> **Maven failed AFTER every expected goal produced a summary.** No goal was lost to truncation, so the failure lies outside goal execution (post-processing, the baseline comparison, or container teardown). The results above are complete and usable." + fi + } >> "$GITHUB_STEP_SUMMARY" + fi + + echo "Benchmark coverage: ${missing} expected summary document(s) absent (benchmark step outcome: ${BENCHMARK_OUTCOME})." + - name: Assemble benchmark artifacts for deployment run: | python3 benchmarks/scripts/benchmark-pages.py assemble \ diff --git a/.plan/project-architecture/api-sheriff-parent/enriched.json b/.plan/project-architecture/api-sheriff-parent/enriched.json index 80161942..4d045c44 100644 --- a/.plan/project-architecture/api-sheriff-parent/enriched.json +++ b/.plan/project-architecture/api-sheriff-parent/enriched.json @@ -4,7 +4,8 @@ ], "insights": [ "CI aggregation gates (build/conclusion, integration-tests/conclusion) fail only as derivatives of their upstream jobs, and a red build/sonar-build with successful scanner auth means a genuine server-side SonarCloud new-code quality-gate verdict (fetchable without credentials via the public sonarcloud.io API) \u2014 triage the upstream cause, never the aggregation gate.", - "CI rollup 'conclusion' gate jobs (build / conclusion, integration-tests / conclusion) fail solely as mirrors of their failing child job; the project treats them as carrying no independent root cause and dispositions them taken_into_account, triaging only the child job's failure." + "CI rollup 'conclusion' gate jobs (build / conclusion, integration-tests / conclusion) fail solely as mirrors of their failing child job; the project treats them as carrying no independent root cause and dispositions them taken_into_account, triaging only the child job's failure.", + "Verify a review bot's reported diff range before trusting a re-review verdict. CodeRabbit re-reviews on this repo have re-reported the ORIGINAL commit range (e.g. base..first-head) on a later sweep, so the bot graded code it never saw and raised a Major-severity finding against an already-fixed defect. Ground-truth every bot claim against the current HEAD before opening a fix task. Related: a plan's own batched triage-reply comment is re-fetched as a new pr-comment finding on the next sweep, so a review sweep never reaches a clean zero \u2014 expect one no-op self-ingested finding per pass." ], "internal_dependencies": [ "api-sheriff", diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 4f530026..5026f22a 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -145,6 +145,80 @@ management interface has exactly one port — its config declares no `ssl-port` `insecure-requests` — so activating management TLS converted port 9000 itself to HTTPS. No plain-HTTP surface remains anywhere in the stack. +=== Which goals actually run + +The CI baseline lane declares twelve goals. Until PLAN-25 it had never executed all of them: the +job died at goal 4 (`passthroughRelay`, whose target hostname resolved nowhere) and Maven's +fail-fast discarded goals 5-12, which had therefore never run in CI even once. Being declared in +`pom.xml` is not evidence that a goal produces a number. + +Current disposition — the CI end-of-job coverage step (see `.github/workflows/benchmark.yml`) +enforces this table, so a goal that silently stops running fails the job: + +[cols="2,1,4", options="header"] +|=== +|Benchmark |State |Notes + +|`healthLiveCheck`, `gatewayHealth`, `proxiedStatic` +|runs +|Already executing before PLAN-25. + +|`passthroughRelay`, `passthroughRelayEmpty` +|runs +|Repaired by PLAN-25: the relay target now resolves, and the `mapped`/`empty` selector actually + reaches the container, so the two modes are genuinely distinct runs. + +|`bearerProxied`, `http2`, `graphql`, `uploadSmall`, `grpcUnary` +|runs +|First execution in CI under PLAN-25. Their trend series start empty here. + +|`uploadLarge` +|*skipped* +|Skipped by `skip.benchmark.upload.large`, which defaults to `true`. The gateway rejects the 50MB + body with `413`: the `upload` anchor declares `max_body_bytes: 67108864`, but the Quarkus HTTP body + limit is not derived from it, so the declared 64 MiB cap is unreachable and the body is refused + before the security filter runs. Blocked on a gateway-side fix. + +|`websocketEcho` +|*skipped* +|Skipped by `skip.benchmark.websocket.echo`, which defaults to `true`. Each WebSocket upgrade leaks + an edge admission permit; once they are exhausted the gateway rejects *all* traffic with `503` + until restarted, which would also poison every goal sequenced after it. Blocked on a gateway-side + fix. + +|`sessionMediated` +|*unwired* +|Backs no Maven goal on purpose — see below. +|=== + +*Skipped means not executed.* Each skipped goal carries a `` element in `benchmarks/pom.xml` +bound to its own property, declared there with the id of the finding that blocks it and defaulting +to `true` — so Maven does not run the goal, it consumes no wall time, and it cannot abort the suite. +The two states are kept distinct rather than collapsed: the CI coverage step gates on the ten goals +that execute and names each skipped goal, its skip property and its reason in the job summary. When +Maven fails, that step also states in words whether the failure truncated the suite. Silence is +never the signal. + +Re-enable a goal once its gateway defect is fixed by flipping its property, which is also how you +reproduce the defect through the harness: + +[source,bash] +---- +./mvnw clean verify -pl benchmarks -Pbenchmark -Dskip.benchmark.upload.large=false +---- + +=== Deliberately unwired scripts + +`session_mediated.js` (benchmark name `sessionMediated`) is present in `k6-scripts/` but is wired to +*no* Maven execution, **on purpose**: BFF session mediation belongs to PLAN-07A, which shipped the +server-session variant and explicitly deferred its per-variant benchmark additions because +`benchmarks/**` is PLAN-25's surface. The script is therefore ready but unclaimed, not forgotten. + +It is listed in the CI end-of-job coverage summary under its own skip section, so its absence from a +run is stated in every job rather than reading as a silent omission. Do not treat a missing +`sessionMediated-summary.json` as a regression, and do not wire the script to a Maven goal without +the owning plan. + == Running the CI baseline lane [source,bash] @@ -339,7 +413,7 @@ scripts/run-comparison.sh --target apisix |`api-sheriff` (default), `apisix`, or `both`. |`--aspects` -|Comma-separated subset of the six matrix aspects. Default: all six. An unknown aspect fails +|Comma-separated subset of the eight matrix aspects. Default: all eight. An unknown aspect fails before any run starts. |`--output` @@ -427,8 +501,13 @@ know: . *Post-processing*: `WrkBenchmarkConverter` / `WrkResultPostProcessor` are replaced by `K6BenchmarkConverter` / `K6ResultPostProcessor`. The downstream report pipeline (badges, 10-run history, trends, GitHub Pages) consumes an identically-shaped model and is unchanged. -. *Coverage*: the lane grew from three wired executions to eight. `bearer` is genuinely new CI - coverage — the wrk-era bearer runner existed on disk but was registered in no Maven execution. +. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, ten of which + execute and are expected to produce a CI result — `uploadLarge` and `websocketEcho` are declared + but skipped by property against open gateway defects, so Maven does not run them at all. `bearer` + is genuinely new CI coverage — the wrk-era bearer runner existed on disk but was registered in no + Maven execution. Being *wired* is not the same as *executing*, and *executing* is not the same as + *producing a result*: until PLAN-25 the job died at the fourth goal and goals 5-12 had never run + in CI even once. See _Which goals actually run_ above. === One-time gh-pages baseline restart @@ -441,8 +520,24 @@ incomparable, the k6 series starts fresh rather than continuing the wrk series: discontinuity* at the swap commit that is a toolchain artifact, not a regression. * The `error` / variability / confidence-band columns go permanently zero-width from that point (see the `latency_ms.stdev` limit above). -* The four new matrix aspects begin with no history and need ten runs before their trend series is - meaningful. +* Every benchmark that writes a summary document for the first time begins with *no history* and + needs ten runs before its trend series is meaningful. Under PLAN-25 that is most of the lane: + eight of the twelve declared goals had never executed in CI, and six of those eight are expected + to produce a result and so start their series empty at this commit. `uploadLarge` and + `websocketEcho` start *no* series at all — only a goal that actually writes a summary document + opens one, and both are skipped by property rather than executed. The workflow's end-of-job coverage step + (`.github/workflows/benchmark.yml`) is the record of which goals produced a result in a given run. + A flat or short trend line on a name that *did* run is an absence of history, not a regression. + +*Two further discontinuities are coming, and they are distinct causes — do not conflate them:* + +. *First execution under PLAN-25* (this commit): goals that had never run and now produce a result + begin their series here. The cause is coverage — data that never existed now exists. +. *PLAN-31 D6/D7 (`%it` elimination)*: that work will change the measured configuration itself, so + the numbers before and after are measurements of different things on the *same* names. The cause + is configuration, not coverage, and it will break continuity again at a later commit. + +A reader comparing across either boundary is comparing incomparable eras. Compare only within one. ==== A second, target-change discontinuity diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index ba971a88..003bfee3 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -28,6 +28,26 @@ true + + + + true + + + true + false @@ -506,10 +526,11 @@ - + - run-k6-upload-large-benchmark + run-k6-grpc-unary-benchmark integration-test exec @@ -522,28 +543,40 @@ --rm k6 run - /scripts/upload_large.js + /scripts/grpc_unary.js ${integration.compose.dir} 240000 - ${k6.vus.upload.large} + ${k6.vus} ${k6.duration} ${k6.output.dir} - + + + - run-k6-websocket-echo-benchmark + run-k6-upload-large-benchmark integration-test exec + + ${skip.benchmark.upload.large} docker compose @@ -551,28 +584,30 @@ --rm k6 run - /scripts/websocket_echo.js + /scripts/upload_large.js ${integration.compose.dir} 240000 - ${k6.vus} + ${k6.vus.upload.large} ${k6.duration} ${k6.output.dir} - + - run-k6-grpc-unary-benchmark + run-k6-websocket-echo-benchmark integration-test exec + + ${skip.benchmark.websocket.echo} docker compose @@ -580,7 +615,7 @@ --rm k6 run - /scripts/grpc_unary.js + /scripts/websocket_echo.js ${integration.compose.dir} 240000 diff --git a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js index 2fb2e405..5c9b3954 100644 --- a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js +++ b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js @@ -30,11 +30,19 @@ import { targetUrl } from './lib/target.js'; const MODE = (__ENV.PASSTHROUGH_SNI || 'mapped').toLowerCase(); /** - * The mapped SNI host the passthrough run's ClientHello carries. It must both resolve to the gateway - * container and be listed in `tls.passthrough_sni`; the benchmark compose overlay (D4) provides that - * mapping and the TLS-enabled backend behind it. Overridable so a run can target a different edge. + * The mapped SNI host the passthrough run's ClientHello carries. Three separate pieces must line up, + * and all three live in the BASE integration-test configuration -- not in the benchmark overlay: + * + * * `integration-tests/docker-compose.yml` puts `passthrough.test.example` in the `api-sheriff` + * service's network `aliases`, which is what makes the name resolve to the gateway container; + * * `integration-tests/src/main/docker/sheriff-config/gateway.yaml` lists it under + * `tls.passthrough_sni`, mapping it to the `PASSTHROUGH_BACKEND` topology alias; + * * the `passthrough-backend` service in that same base compose file is the TLS-enabled backend + * the relay dials, and its certificate carries the name as a SAN. + * + * Overridable so a run can target a different edge. */ -const PASSTHROUGH_TARGET_URL = __ENV.PASSTHROUGH_TARGET_URL || 'https://passthrough.api-sheriff:8443/get'; +const PASSTHROUGH_TARGET_URL = __ENV.PASSTHROUGH_TARGET_URL || 'https://passthrough.test.example:8443/get'; /** * Resolves the (benchmarkName, url) pair for the selected mode. An unknown mode is fatal at module diff --git a/integration-tests/docker-compose.yml b/integration-tests/docker-compose.yml index 54f09433..e8cb6ac6 100644 --- a/integration-tests/docker-compose.yml +++ b/integration-tests/docker-compose.yml @@ -269,9 +269,16 @@ services: # the application port 8443 via /dev/tcp and never touches 9000, so it is unaffected by the # management-TLS switch. - # Network isolation (production pattern) + # Network isolation (production pattern). The alias is what lets a passthrough client reach + # this gateway under a tls.passthrough_sni name: the benchmark's ClientHello carries + # passthrough.test.example, so that name must resolve to THIS container for the accept-time + # SNI front listener to see a match and L4-relay to PASSTHROUGH_BACKEND. Map form is required + # here — the short-form list carries no aliases. The backend certificate already covers the + # name (SAN DNS:passthrough.test.example on the passthrough-backend service). networks: - - api-sheriff + api-sheriff: + aliases: + - passthrough.test.example # Production restart policy restart: unless-stopped @@ -562,11 +569,17 @@ services: # user is not an option because the uid is not known to compose, so it runs as root and # the result files stay removable by `mvn clean` (the parent directory is host-owned). user: "0:0" + # Value-less entries pass the host variable through only when it is set, so an unset + # variable stays unset in the container rather than becoming an empty string. PASSTHROUGH_SNI + # selects the passthrough_relay.js mode (mapped | empty) and is set per benchmark execution in + # benchmarks/pom.xml; without it here the selector never reaches k6 and both executions would + # run the same mode. environment: - BENCHMARK_VUS - BENCHMARK_DURATION - BENCHMARK_MAX_ERROR_RATE - GATEWAY_TARGET + - PASSTHROUGH_SNI networks: api-sheriff: