diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index da2741da..8ca64ff5 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -40,16 +40,20 @@ jobs: # (k6.vus.upload.large = 5), so it does not finish in the same wall time as # a request-rate run — it is budgeted at ~3 min on its own. Re-derive this # term when a skip property is flipped, or when a goal is added or removed. - # * stack startup ~100 s -- the lane boots FIVE native gateway instances (api-sheriff, - # api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2 and - # api-sheriff-ws-admission, the last added for the WebSocket relay-permit - # exhaustion regression) 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`. Each added + # * stack startup ~120 s -- the lane boots SEVEN native gateway instances (api-sheriff, + # api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2, + # api-sheriff-ws-admission for the WebSocket relay-permit exhaustion + # regression, api-sheriff-plain-mgmt for the plain-HTTP management opt-out, + # and api-sheriff-passthrough-empty for the benchmark's empty-passthrough_sni + # arm) 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`. Each added # gateway instance costs roughly another 10 s of the readiness wait — they # share one native image, so the term grows with instance count, not with - # image builds. Re-derive this term whenever an api-sheriff* service is - # added to or removed from integration-tests/docker-compose.yml. + # image builds; the two instances added since this term was last derived are + # what take it from ~100 s to ~120 s. Re-derive this term whenever an + # api-sheriff* service is added to or removed from + # integration-tests/docker-compose.yml. # * 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 diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 02ec729d..c7f35c21 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -100,8 +100,10 @@ comparison; see the passthrough methodology note under _Methodology_. |`passthrough_relay.js` |The opaque L4 TCP relay path (`mapped` mode): `k6 -> gateway public TLS port with a mapped SNI -> L4 relay -> TLS-enabled backend`, measuring relay throughput/latency through the active passthrough - path. Its `empty` mode re-measures the same proxied static route with `passthrough_sni` empty (D1's - zero-overhead default) for a no-regression check against the `unauth` baseline. *API-Sheriff-only*; + path. Its `empty` mode runs the *same* `/proxy/static` route against the *same* `nginx-static` + upstream as the `proxiedStatic` baseline, but on a *second gateway instance* + (`api-sheriff-passthrough-empty`) whose `gateway.yaml` declares no `passthrough_sni` — so the + no-regression check has a genuinely listener-free side to measure. *API-Sheriff-only*; like `ws`/`grpc` it rides a real backend — see the passthrough methodology note. |=== @@ -273,7 +275,11 @@ proxy upstream at it (the integration tests instead point it at the `go-httpbin` |`k6.duration` |60s -|Measured window per aspect. +|Load phase per aspect (`-Pquick` sets 30s). The *measured* window reported in each summary is this + value plus however much of the scenario's 5s bounded `gracefulStop` the run actually consumes, + because k6's `requests_per_second` is a rate over the whole run duration. The passthrough gate's + window-comparability band is *derived from this value* and is handed to the comparator as + `passthrough.baseline.load.duration` — see the window-comparability note under _Methodology_. |=== == Methodology @@ -317,14 +323,87 @@ gateway relays the still-encrypted stream at L4. Read it as relay overhead versu baseline within one run, never as an absolute throughput claim. It runs in two modes selected by `PASSTHROUGH_SNI`. In `mapped` mode the ClientHello names a `tls.passthrough_sni` host, so the gateway relays the still-encrypted byte stream at L4 without -terminating and the run measures throughput/latency through the active relay path. In `empty` mode -`passthrough_sni` is empty — D1's zero-overhead default, where the accept-time front listener is -never created and the single terminated Quarkus HTTPS listener owns the public port directly — so -the run re-measures exactly the same proxied static route as the PLAN-04 `unauth` (`proxiedStatic`) -baseline. `PassthroughBaselineComparator` reads the empty-mode summary against that stored baseline -and fails the run if throughput dropped, or either latency percentile rose, beyond a fixed -percentile-band noise tolerance (k6 omits `latency_ms.stdev`, so a standard-deviation gate is not -available — see _Known fidelity limit_). A metric a run did not measure renders `n/a`, never `0`. +terminating and the run measures throughput/latency through the active relay path. ++ +`empty` mode is *not a second mode of the same gateway*, and cannot be: `passthrough_sni` is a +property of the whole gateway process — declaring it non-empty is what starts the accept-time SNI +front listener at boot — so no per-request switch can make the primary instance, which declares two +entries for its whole lifetime, behave as though the listener were absent. The `empty` arm therefore +runs against a *dedicated second gateway instance*, `api-sheriff-passthrough-empty`, whose overlaid +`gateway.yaml` is the shared descriptor with the `tls.passthrough_sni` block — and only that block — +removed. On that instance the front listener is never created and the single terminated Quarkus +HTTPS listener owns the public port directly, which is D1's zero-overhead default. ++ +Everything else is held equal, because the comparison is only meaningful as a single-variable one +(ADR-0012): the *same* native image, the *same* `/proxy/static` route, the *same* `nginx-static` +upstream the benchmark overlay repoints both instances at, the *same* mounted TLS material, and the +*same* CPU and memory limits (512M / 4.0 CPU). A difference on any of those folds its own cost into +one side of the gate and is then read as passthrough overhead. ++ +Those parity conditions are enforced structurally rather than by review convention, by four +assertions in `TlsEdgeActivationWiringTest`: `emptyPassthroughOverlayDeclaresNoPassthroughSni` (the +arm's side really declares none), `emptyPassthroughInstanceSetsNoInternalSslPort` (no internal-port +split, since there is no front listener to free the public port for), +`emptyPassthroughOverlayDiffersFromTheBaseOnlyByPassthroughSni` (the single-variable property, as a +parsed-structure equality against the base descriptor once `passthrough_sni` is removed) and +`emptyPassthroughInstanceSharesThePrimaryBenchmarkUpstream` (the same-upstream property, asserted as +an equality between the two declared `TOPOLOGY_UPSTREAM` values rather than against a hard-coded +literal, so repointing the benchmark moves both arms together). ++ +`PassthroughBaselineComparator` reads the empty-mode summary against the primary instance's stored +`proxiedStatic` baseline and fails the run if throughput dropped, or either latency percentile rose, +beyond a fixed percentile-band noise tolerance (k6 omits `latency_ms.stdev`, so a standard-deviation +gate is not available — see _Known fidelity limit_). A metric a run did not measure renders `n/a`, +never `0`. ++ +*Window comparability is checked first, and an incomparable pair is refused rather than compared.* +`requests_per_second` is a counter rate over the run's whole measured window — the load phase *plus* +the graceful-stop tail — so two arms whose windows differ in length do not produce comparable rates +at all. An arm whose tail was stretched by a single stalled virtual user reports a proportionally +deflated rate that reads as a throughput collapse which never happened. The comparator therefore +reads `start_time` / `end_time` from both summaries and compares the two windows *before* the +throughput and latency rows. ++ +*The band is derived from the configured load phase, not fixed.* Every aspect script bounds its tail +with an explicit `constant-vus` scenario and a 5s `gracefulStop`, so the worst-case inflation a +healthy run can produce is an *absolute* 5 seconds — which is a different *fraction* of every +configured duration. The comparator therefore allows a fixed absolute drift of 6s (the 5s tail plus a +second of ordinary start/stop skew) expressed as a fraction of the effective `k6.duration`, which the +POM hands it as `passthrough.baseline.load.duration`: ++ +[cols="1,1,1,3", options="header"] +|=== +|`k6.duration` |Worst-case tail |Derived band |Where it comes from + +|60s (default) +|5/60 = 8.3% +|6/60 = 10% +|The default lane. Unchanged from the previously fixed band. + +|30s (`-Pquick`) +|5/30 = 16.7% +|6/30 = 20% +|A fixed 10% band would have rejected a perfectly healthy quick run as `WINDOW_MISMATCH`, because + 16.7% of window inflation is legitimate at this duration. +|=== ++ +A reverted or unbounded tail still fails at every duration — k6's own 30s `gracefulStop` default is +at least 50% of any run this lane configures, far outside the derived band. Setting +`passthrough.baseline.window.tolerance` explicitly overrides the derivation outright. ++ +*A window that is not positive is refused before the band is applied at all.* Agreement is necessary +for comparability but never sufficient: two zero-length windows agree perfectly, and so do two whose +`end_time` precedes their `start_time`, yet neither pair describes an interval a rate could have been +measured over. Such a pair is `WINDOW_MISMATCH`, never `PASS`. This is distinct from an *absent* +window pair, which stays `NOT_MEASURED` (see below). ++ +The refusal *fails the run*: a mismatched window pair is deliberately **not** rescaled to a common +window and **not** downgraded to a warning. Rescaling would republish a number the run never +measured, and a warning would let a lane with a broken measurement window keep producing baseline +history that looks authoritative. The failure names both windows so the fix is applied to the +measurement, not to the threshold. An *absent* window pair is the single exception — it renders +`n/a`, is classified `NOT_MEASURED`, and never fails, so a summary predating these fields is not +retroactively broken. Readiness, not warm-up discard:: `pre-benchmark-health-check.sh` gates every run on the stack actually serving, so no run starts diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index b7986217..f8a938ad 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -428,10 +428,14 @@ - + run-k6-passthrough-relay-empty-benchmark integration-test @@ -749,6 +753,20 @@ ${k6.output.dir} + + + + passthrough.baseline.load.duration + ${k6.duration} + + diff --git a/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/K6BenchmarkLogMessages.java b/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/K6BenchmarkLogMessages.java index a129d3e3..6a73a41a 100644 --- a/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/K6BenchmarkLogMessages.java +++ b/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/K6BenchmarkLogMessages.java @@ -181,5 +181,17 @@ private ERROR() { .identifier(210) .template("Passthrough empty-mode regressed beyond the %s noise band vs the PLAN-04 baseline: %s") .build(); + + /** + * Logged when the two compared arms were measured over windows too far apart for their rates + * to be comparable. The comparison is refused, not rescaled — see + * {@link de.cuioss.sheriff.gateway.k6.benchmark.PassthroughBaselineComparator}. + */ + public static final LogRecord PASSTHROUGH_BASELINE_WINDOW_MISMATCH = LogRecordModel.builder() + .prefix(PREFIX) + .identifier(211) + .template("Passthrough empty-mode and baseline windows disagree beyond the %s " + + "window-comparability band, so their rates are not comparable: %s") + .build(); } } diff --git a/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparator.java b/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparator.java index 5a9f3fab..04bf608a 100644 --- a/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparator.java +++ b/benchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparator.java @@ -22,20 +22,38 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.time.Instant; +import java.time.format.DateTimeParseException; import java.util.List; import java.util.Locale; import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** - * Verifies that the empty-{@code passthrough_sni} proxied route does not regress beyond the run's - * noise band against the stored PLAN-04 plain-proxy baseline. + * Verifies that a gateway configured with no {@code tls.passthrough_sni} does not serve the plain + * proxied route measurably worse than the primary gateway, which declares one. *

- * When {@code tls.passthrough_sni} is empty the accept-time front listener is never created (D1's - * zero-overhead default) and the terminated Quarkus HTTPS listener owns the public port directly, so - * the empty-mode run measures exactly the same proxied static route as the PLAN-04 {@code unauth} - * ({@code proxiedStatic}) aspect. This comparator reads the {@code passthroughRelayEmpty} summary - * against the {@code proxiedStatic} summary produced in the same lane and asserts no regression - * beyond a percentile-band tolerance. + * The two summaries come from two different gateway instances running concurrently in the + * same lane, not from two modes of one process: {@code passthrough_sni} is a property of the whole + * gateway — declaring it non-empty is what starts the accept-time SNI front listener at boot — so + * emptiness cannot be selected per request. The {@code passthroughRelayEmpty} candidate is measured + * against the dedicated {@code api-sheriff-passthrough-empty} instance, whose overlaid + * {@code gateway.yaml} declares no {@code passthrough_sni} and where the front listener is therefore + * never created (D1's zero-overhead default, terminated Quarkus HTTPS listener owning the public + * port directly). The {@code proxiedStatic} baseline is measured against the primary instance, which + * declares two {@code passthrough_sni} entries for its whole lifetime. Both arms drive the same + * {@code /proxy/static} route to the same {@code nginx-static} upstream on the same image under the + * same resource limits, so the two runs differ in exactly one thing: the presence of the accept-time + * front listener. + *

+ * The bound is one-sided, and deliberately so. On the baseline instance the front + * listener owns the public port, so every connection — including the plain proxied ones this route + * carries — pays an accept-time ClientHello peek and an L4 relay hop to the internal terminated + * listener. The candidate instance pays neither. The expected result is therefore that the candidate + * measures at or above the baseline, and the gate asserts only + * {@code candidate >= baseline * (1 - tolerance)}: it exists to catch the front listener's absence + * somehow costing throughput, never to cap the headroom its absence yields. *

* Noise band, not a point comparison. These are single-node, containerized, * local-network measurements, so a small run-to-run delta is noise rather than signal. The band is a @@ -48,6 +66,26 @@ * Absent metric is {@code n/a}, never {@code 0}. A metric a run did not measure is * rendered {@code n/a} and classified {@link Verdict#NOT_MEASURED} — never a {@code 0} that would * read as a measured collapse and never a false regression — mirroring {@link ComparisonSummaryWriter}. + *

+ * Window comparability is a precondition, not a metric. {@code requests_per_second} + * is a counter rate over the run's whole measured window — load phase plus graceful-stop tail — so two + * arms measured over windows of different length do not produce comparable rates at all. A run in which + * one arm's window was stretched by a stalled VU reports a proportionally deflated rate that reads as a + * throughput collapse which never happened; this is exactly the false 35.8% verdict PLAN-46 diagnosed. + * The comparator therefore reads {@code start_time} / {@code end_time} from both summaries and compares + * the two windows first, against a band derived from the configured load-phase duration + * (overridable via {@value #WINDOW_TOLERANCE_PROPERTY}) — see + * {@link #derivedWindowTolerance(double)}. It also refuses a window that is not positive at all: a + * zero-length or end-before-start pair is not a comparable measurement, and comparing only the two + * windows' absolute difference would let two such windows agree with each other and pass. + *

+ * Operator ruling — refuse and fail; do not normalise. An incomparable window pair + * FAILS the run. It is deliberately NOT rescaled to a common window, and deliberately NOT downgraded to + * a warning. Rescaling would silently republish a number the run never measured, and a warning would let + * a lane whose measurement window is broken keep producing baseline history that looks authoritative. + * The failure names the two windows so the operator fixes the measurement rather than the threshold. An + * ABSENT window pair is the one exception: it reads {@code n/a} / {@link Verdict#NOT_MEASURED} and never + * fails, so a summary shape that predates the window fields is not retroactively broken. * * @author API Sheriff Team * @since 1.0 @@ -56,10 +94,13 @@ public final class PassthroughBaselineComparator { private static final CuiLogger LOGGER = new CuiLogger(PassthroughBaselineComparator.class); - /** The empty-mode candidate summary, produced by {@code passthrough_relay.js} with {@code PASSTHROUGH_SNI=empty}. */ + /** + * The candidate summary, produced by {@code passthrough_relay.js} with + * {@code PASSTHROUGH_SNI=empty} against the no-{@code passthrough_sni} gateway instance. + */ static final String CANDIDATE_SUMMARY = "passthroughRelayEmpty-summary.json"; - /** The stored PLAN-04 plain-proxy baseline summary the candidate is read against. */ + /** The plain-proxy baseline summary from the primary instance, taken in the same lane. */ static final String BASELINE_SUMMARY = "proxiedStatic-summary.json"; /** Name of the rendered artifact, written directly under the results directory. */ @@ -71,10 +112,62 @@ public final class PassthroughBaselineComparator { /** System property overriding {@link #DEFAULT_TOLERANCE}. */ static final String TOLERANCE_PROPERTY = "passthrough.baseline.tolerance"; + /** + * The absolute drift the window-comparability band allows, in seconds — the quantity the band is + * really made of, before it is expressed as a fraction of a particular run. + *

+ * It is the sum of the only two things that may legitimately separate the two arms' measured + * windows: the bounded 5s {@code gracefulStop} every aspect scenario declares (see + * {@code k6-scripts/lib/summary.js}), plus one second of ordinary start/stop skew. Both are + * absolute — a stalled VU adds at most the tail itself, never a fraction of the run — + * and that is precisely why the band cannot be a fixed fraction: the same 6s is 10% of the default + * 60s load phase but 20% of the 30s one the {@code quick} profile configures. + */ + static final double WINDOW_DRIFT_ALLOWANCE_SECONDS = 6.0; + + /** + * The load-phase duration assumed when {@value #LOAD_DURATION_PROPERTY} is unset, in seconds — + * the benchmarks POM's own {@code k6.duration} default. + */ + static final double DEFAULT_LOAD_DURATION_SECONDS = 60.0; + + /** + * System property carrying the effective k6 load-phase duration as a k6 duration string (e.g. + * {@code 60s}, {@code 30s}, {@code 1m30s}). + *

+ * The benchmarks POM hands the live {@code k6.duration} value to the comparator through it, so the + * derived band tracks the duration the run was actually configured with instead of a compiled-in + * assumption about it. That is the whole point: a {@code -Pquick} run shortens the load phase to + * 30s without shortening the graceful-stop tail, so a band blind to the duration under-covers + * exactly the scenario it exists to tolerate. + */ + static final String LOAD_DURATION_PROPERTY = "passthrough.baseline.load.duration"; + + /** + * The window band for the default 60s load phase — {@link #WINDOW_DRIFT_ALLOWANCE_SECONDS} + * expressed as a fraction of {@link #DEFAULT_LOAD_DURATION_SECONDS}, i.e. 10%. + *

+ * It is the value {@link #derivedWindowTolerance(double)} yields when the lane runs its default + * duration, and the fallback used when {@value #LOAD_DURATION_PROPERTY} is unset. Tighter than + * {@link #DEFAULT_TOLERANCE} on purpose: the window is a precondition of the comparison rather + * than one of the compared metrics, so it must catch a drift small enough to still move the + * throughput verdict, while staying above the worst case a healthy run can produce. + */ + static final double DEFAULT_WINDOW_TOLERANCE = + WINDOW_DRIFT_ALLOWANCE_SECONDS / DEFAULT_LOAD_DURATION_SECONDS; + + /** System property overriding the derived window band outright. */ + static final String WINDOW_TOLERANCE_PROPERTY = "passthrough.baseline.window.tolerance"; + + /** One component of a k6 duration string: a magnitude and one of the four units k6 accepts. */ + private static final Pattern DURATION_COMPONENT = Pattern.compile("(\\d+(?:\\.\\d+)?)(ms|s|m|h)"); + private static final String FIELD_REQUESTS_PER_SECOND = "requests_per_second"; private static final String FIELD_LATENCY_MS = "latency_ms"; private static final String FIELD_P50 = "p50"; private static final String FIELD_P99 = "p99"; + private static final String FIELD_START_TIME = "start_time"; + private static final String FIELD_END_TIME = "end_time"; private static final String NOT_AVAILABLE = "n/a"; @@ -82,9 +175,12 @@ private PassthroughBaselineComparator() { // utility class } - /** Whether a single metric stayed within the band, regressed, or was not measured on either side. */ + /** + * Whether a single metric stayed within the band, regressed, was not measured on either side, or — + * for the measurement window alone — made the whole comparison invalid. + */ enum Verdict { - PASS, REGRESSION, NOT_MEASURED + PASS, REGRESSION, NOT_MEASURED, WINDOW_MISMATCH } /** One metric's candidate-vs-baseline comparison. */ @@ -92,12 +188,30 @@ record MetricComparison(String label, String unit, Optional candidate, Optional baseline, Verdict verdict) { } - /** The full comparison across the throughput and latency metrics. */ + /** The full comparison across the measurement window and the throughput and latency metrics. */ record ComparisonResult(List metrics) { - /** A regression on any measured metric fails the run; a not-measured metric never does. */ + /** + * A regression on any measured metric fails the run, and so does an incomparable measurement + * window; a not-measured metric never does. + * + * @return whether the run must fail + */ boolean regressed() { - return metrics.stream().anyMatch(metric -> metric.verdict() == Verdict.REGRESSION); + return metrics.stream().anyMatch(metric -> metric.verdict() == Verdict.REGRESSION + || metric.verdict() == Verdict.WINDOW_MISMATCH); + } + + /** + * Whether the two arms' measured windows were too far apart for their rates to be compared. + *

+ * Reported separately from {@link #regressed()} because it is a different failure: the run did + * not regress, it produced a comparison that cannot be believed either way. + * + * @return whether the window precondition failed + */ + boolean windowMismatched() { + return metrics.stream().anyMatch(metric -> metric.verdict() == Verdict.WINDOW_MISMATCH); } } @@ -131,9 +245,17 @@ public static void main(String[] args) throws IOException { String rendered = render(result, tolerance); Files.writeString(resultsDir.resolve(OUTPUT_FILE_NAME), rendered); + // Checked BEFORE the regression branch: when the windows are incomparable the throughput verdict + // is meaningless, so reporting it as a regression would name the wrong defect. + if (result.windowMismatched()) { + LOGGER.error(K6BenchmarkLogMessages.ERROR.PASSTHROUGH_BASELINE_WINDOW_MISMATCH, + formatTolerance(resolveWindowTolerance()), detailFor(result, Verdict.WINDOW_MISMATCH)); + throw new IllegalStateException( + "empty-passthrough_sni run and baseline were measured over incomparable windows"); + } if (result.regressed()) { LOGGER.error(K6BenchmarkLogMessages.ERROR.PASSTHROUGH_BASELINE_REGRESSION, - formatTolerance(tolerance), regressionDetail(result)); + formatTolerance(tolerance), detailFor(result, Verdict.REGRESSION)); throw new IllegalStateException("empty-passthrough_sni run regressed beyond the noise band"); } LOGGER.info(K6BenchmarkLogMessages.INFO.PASSTHROUGH_BASELINE_OK, formatTolerance(tolerance)); @@ -166,6 +288,13 @@ private static Optional readSummary(Path summaryPath, Path resultsDi * @return the per-metric comparison */ static ComparisonResult compare(JsonObject candidate, JsonObject baseline, double tolerance) { + // The window row leads the list because it is the comparison's precondition: when it fails, the + // rows beneath it describe rates taken over different denominators and mean nothing. + Optional candidateWindow = windowMillis(candidate); + Optional baselineWindow = windowMillis(baseline); + MetricComparison window = new MetricComparison("measurement window", "ms", + candidateWindow, baselineWindow, + windowVerdict(candidateWindow, baselineWindow, resolveWindowTolerance())); MetricComparison rps = new MetricComparison("throughput", "RPS", topLevelMetric(candidate, FIELD_REQUESTS_PER_SECOND), topLevelMetric(baseline, FIELD_REQUESTS_PER_SECOND), @@ -173,7 +302,78 @@ static ComparisonResult compare(JsonObject candidate, JsonObject baseline, doubl topLevelMetric(baseline, FIELD_REQUESTS_PER_SECOND), tolerance)); MetricComparison p50 = latencyComparison("latency p50", candidate, baseline, FIELD_P50, tolerance); MetricComparison p99 = latencyComparison("latency p99", candidate, baseline, FIELD_P99, tolerance); - return new ComparisonResult(List.of(rps, p50, p99)); + return new ComparisonResult(List.of(window, rps, p50, p99)); + } + + /** + * Reads a summary's measured window in milliseconds from its {@code start_time} / {@code end_time} + * ISO-8601 pair. + *

+ * An absent, non-string or unparseable pair is treated as NOT MEASURED rather than as a zero-length + * window: a {@code 0} here would read as a maximally mismatched window and would fail every run + * whose summaries predate these fields. + * + * @param summary the parsed summary + * @return the window length in milliseconds, or empty when the pair is absent or unparseable + */ + static Optional windowMillis(JsonObject summary) { + Optional start = instantField(summary, FIELD_START_TIME); + Optional end = instantField(summary, FIELD_END_TIME); + if (start.isEmpty() || end.isEmpty()) { + return Optional.empty(); + } + return Optional.of((double) (end.get().toEpochMilli() - start.get().toEpochMilli())); + } + + /** + * Parses one ISO-8601 instant field, treating an absent, non-string or malformed value as absent. + * + * @param summary the parsed summary + * @param field the field name + * @return the parsed instant, or empty + */ + private static Optional instantField(JsonObject summary, String field) { + if (!summary.has(field) || !summary.get(field).isJsonPrimitive()) { + return Optional.empty(); + } + try { + return Optional.of(Instant.parse(summary.get(field).getAsString())); + } catch (DateTimeParseException e) { + return Optional.empty(); + } + } + + /** + * The comparability verdict for the two arms' measured windows. + *

+ * A window that is not positive is refused before the bands are applied at all. + * Agreement is a necessary condition for comparability, never a sufficient one: two zero-length + * windows agree perfectly, and so do two windows whose {@code end_time} precedes their + * {@code start_time}, yet neither pair describes an interval any rate could have been measured + * over. Comparing only the absolute drift would pass both and hand the throughput row a quotient + * taken over an invalid denominator — the same class of unbelievable number this precondition + * exists to refuse. + *

+ * The band is applied to the BASELINE window, matching how {@link #throughputVerdict} and + * {@link #latencyVerdict} anchor their bands, so the same override reads the same way across all + * three. A mismatch is {@link Verdict#WINDOW_MISMATCH} and fails the run — it is never normalised + * away and never downgraded to a warning (see the class javadoc's operator ruling). + * + * @param candidate the candidate window in milliseconds, when measured + * @param baseline the baseline window in milliseconds, when measured + * @param windowTolerance the fractional band the two windows must agree within + * @return {@link Verdict#NOT_MEASURED} when either side is absent, else PASS / WINDOW_MISMATCH + */ + static Verdict windowVerdict(Optional candidate, Optional baseline, double windowTolerance) { + if (candidate.isEmpty() || baseline.isEmpty()) { + return Verdict.NOT_MEASURED; + } + if (candidate.get() <= 0.0 || baseline.get() <= 0.0) { + return Verdict.WINDOW_MISMATCH; + } + double allowedDrift = baseline.get() * windowTolerance; + return Math.abs(candidate.get() - baseline.get()) <= allowedDrift + ? Verdict.PASS : Verdict.WINDOW_MISMATCH; } private static MetricComparison latencyComparison(String label, JsonObject candidate, JsonObject baseline, @@ -268,7 +468,14 @@ static String render(ComparisonResult result, double tolerance) { .append("Empty-`passthrough_sni` proxied route (`").append(CANDIDATE_SUMMARY) .append("`) vs the PLAN-04 plain-proxy baseline (`").append(BASELINE_SUMMARY).append("`). ") .append("Noise band: ").append(formatTolerance(tolerance)) - .append(". `n/a` means the run did not measure the metric.\n\n") + .append("; window-comparability band: ").append(formatTolerance(resolveWindowTolerance())) + .append(""" + . The measurement-window row is a PRECONDITION, not a metric: a \ + `WINDOW_MISMATCH` means the two arms were measured over different-length \ + windows, so their rates are not comparable and the run fails rather than \ + being rescaled. `n/a` means the run did not measure the metric. + + """) .append("| Metric | Unit | Empty-mode | Baseline | Verdict |\n") .append("|---|---|---|---|---|\n"); for (MetricComparison metric : result.metrics()) { @@ -282,10 +489,13 @@ static String render(ComparisonResult result, double tolerance) { return out.toString(); } - /** Renders the regressed metrics for the failure diagnostic. */ - private static String regressionDetail(ComparisonResult result) { + /** + * Renders every metric carrying the given verdict as a single diagnostic fragment, naming both + * sides of each row so a failure is fixed at the measurement rather than at the threshold. + */ + private static String detailFor(ComparisonResult result, Verdict verdict) { return result.metrics().stream() - .filter(metric -> metric.verdict() == Verdict.REGRESSION) + .filter(metric -> metric.verdict() == verdict) .map(metric -> "%s (empty-mode %s vs baseline %s %s)".formatted(metric.label(), render(metric.candidate()), render(metric.baseline()), metric.unit())) .reduce((left, right) -> left + ", " + right) @@ -304,20 +514,130 @@ private static String formatTolerance(double tolerance) { * @return the fractional noise band in {@code [0, 1]} */ static double resolveTolerance() { - String raw = System.getProperty(TOLERANCE_PROPERTY); + return resolveFraction(TOLERANCE_PROPERTY, DEFAULT_TOLERANCE); + } + + /** + * Resolves the window-comparability band: an explicit + * {@code passthrough.baseline.window.tolerance} override when one is set, else the band derived + * from the effective load-phase duration. An invalid override is fatal under the same rule + * {@link #resolveTolerance()} applies: a negative value would invert the precondition and a value + * above 1 would disable it. + * + * @return the fractional window band in {@code [0, 1]} + */ + static double resolveWindowTolerance() { + return resolveFraction(WINDOW_TOLERANCE_PROPERTY, + derivedWindowTolerance(resolveLoadDurationSeconds())); + } + + /** + * The window-comparability band for a given load-phase duration: + * {@link #WINDOW_DRIFT_ALLOWANCE_SECONDS} expressed as a fraction of that duration. + *

+ * This is what makes the precondition cover the scenario it protects rather than only the default + * one. The graceful-stop tail a stalled VU can add is an absolute 5s whatever the load phase is, + * so on the default 60s run it is 8.3% of the window and on the {@code quick} profile's 30s run it + * is 16.7% — above a fixed 10% band, which would have rejected a perfectly healthy quick run as a + * window mismatch. Deriving the band keeps that worst case inside it at every duration while + * keeping a reverted or unbounded tail (k6's own 30s default, at least 50% of any run this lane + * configures) outside it. + *

+ * Clamped at 1: a load phase shorter than the allowance itself cannot yield a fraction above 1, + * which the tolerance contract forbids because it would disable the precondition outright. + * + * @param loadDurationSeconds the effective k6 load-phase duration in seconds; must be positive + * @return the fractional window band in {@code (0, 1]} + */ + static double derivedWindowTolerance(double loadDurationSeconds) { + return Math.min(1.0, WINDOW_DRIFT_ALLOWANCE_SECONDS / loadDurationSeconds); + } + + /** + * Resolves the effective k6 load-phase duration from {@value #LOAD_DURATION_PROPERTY}, falling + * back to {@link #DEFAULT_LOAD_DURATION_SECONDS}. A set-but-invalid value is fatal rather than + * silently defaulted, for the reason every other override here is: silently substituting the 60s + * default for an unreadable {@code k6.duration} would compute the band for a run that never + * happened. + * + * @return the load-phase duration in seconds, strictly positive + */ + static double resolveLoadDurationSeconds() { + String raw = System.getProperty(LOAD_DURATION_PROPERTY); + if (raw == null || raw.isBlank()) { + return DEFAULT_LOAD_DURATION_SECONDS; + } + double seconds = parseDurationSeconds(raw); + if (seconds <= 0.0) { + throw new IllegalArgumentException( + LOAD_DURATION_PROPERTY + " must be a positive k6 duration, got \"" + raw + "\""); + } + return seconds; + } + + /** + * Parses a k6 duration string — one or more magnitude/unit components, as in {@code 60s}, + * {@code 500ms} or {@code 1m30s} — into seconds. + * + * @param raw the duration string + * @return the duration in seconds + * @throws IllegalArgumentException when the string is not a well-formed k6 duration + */ + static double parseDurationSeconds(String raw) { + String value = raw.strip(); + Matcher matcher = DURATION_COMPONENT.matcher(value); + double seconds = 0.0; + int consumed = 0; + while (matcher.find(consumed) && matcher.start() == consumed) { + seconds += Double.parseDouble(matcher.group(1)) * unitSeconds(matcher.group(2)); + consumed = matcher.end(); + } + if (consumed == 0 || consumed != value.length()) { + throw new IllegalArgumentException(LOAD_DURATION_PROPERTY + + " must be a k6 duration such as \"60s\", \"500ms\" or \"1m30s\", got \"" + raw + "\""); + } + return seconds; + } + + /** + * The seconds one unit of a k6 duration component represents. + * + * @param unit one of {@code ms} / {@code s} / {@code m} / {@code h} + * @return the unit's length in seconds + */ + private static double unitSeconds(String unit) { + return switch (unit) { + case "ms" -> 0.001; + case "s" -> 1.0; + case "m" -> 60.0; + // "h" — the only remaining unit DURATION_COMPONENT admits, so no other value can arrive. + default -> 3600.0; + }; + } + + /** + * Resolves one fraction-valued system property, rejecting a set-but-invalid override rather than + * silently defaulting it. + * + * @param property the system-property name + * @param fallback the value used when the property is unset or blank + * @return the resolved fraction in {@code [0, 1]} + */ + private static double resolveFraction(String property, double fallback) { + String raw = System.getProperty(property); if (raw == null || raw.isBlank()) { - return DEFAULT_TOLERANCE; + return fallback; } double value; try { value = Double.parseDouble(raw.strip()); } catch (NumberFormatException e) { throw new IllegalArgumentException( - TOLERANCE_PROPERTY + " must be a fraction in [0, 1], got \"" + raw + "\"", e); + property + " must be a fraction in [0, 1], got \"" + raw + "\"", e); } if (!Double.isFinite(value) || value < 0.0 || value > 1.0) { throw new IllegalArgumentException( - TOLERANCE_PROPERTY + " must be a fraction in [0, 1], got \"" + raw + "\""); + property + " must be a fraction in [0, 1], got \"" + raw + "\""); } return value; } diff --git a/benchmarks/src/main/resources/k6-scripts/bearer_proxied.js b/benchmarks/src/main/resources/k6-scripts/bearer_proxied.js index 9b152ce1..8c15cd37 100644 --- a/benchmarks/src/main/resources/k6-scripts/bearer_proxied.js +++ b/benchmarks/src/main/resources/k6-scripts/bearer_proxied.js @@ -17,7 +17,7 @@ */ import http from 'k6/http'; import { check, fail } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'bearerProxied'; @@ -58,8 +58,7 @@ const KEYCLOAK_USERNAME = __ENV.KEYCLOAK_USERNAME || 'benchmark-user'; const KEYCLOAK_PASSWORD = __ENV.KEYCLOAK_PASSWORD || 'benchmark-password'; export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/gateway_health.js b/benchmarks/src/main/resources/k6-scripts/gateway_health.js index d457ce77..386794f2 100644 --- a/benchmarks/src/main/resources/k6-scripts/gateway_health.js +++ b/benchmarks/src/main/resources/k6-scripts/gateway_health.js @@ -18,7 +18,7 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; const BENCHMARK_NAME = 'gatewayHealth'; // Hard-coded rather than routed through lib/target.js: the health benchmarks are deliberately @@ -26,8 +26,7 @@ const BENCHMARK_NAME = 'gatewayHealth'; const TARGET_URL = __ENV.TARGET_URL || 'https://api-sheriff:9000/q/health'; export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, // The benchmark stack terminates TLS with the self-signed localhost bundle mounted into // every service, so certificate verification is skipped for the load generator only. diff --git a/benchmarks/src/main/resources/k6-scripts/graphql.js b/benchmarks/src/main/resources/k6-scripts/graphql.js index f0126cc5..9d3743a9 100644 --- a/benchmarks/src/main/resources/k6-scripts/graphql.js +++ b/benchmarks/src/main/resources/k6-scripts/graphql.js @@ -14,7 +14,7 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'graphql'; @@ -28,8 +28,7 @@ const QUERY = JSON.stringify({ }); export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/grpc_unary.js b/benchmarks/src/main/resources/k6-scripts/grpc_unary.js index 155f8393..4b0441a5 100644 --- a/benchmarks/src/main/resources/k6-scripts/grpc_unary.js +++ b/benchmarks/src/main/resources/k6-scripts/grpc_unary.js @@ -20,7 +20,7 @@ import grpc from 'k6/net/grpc'; import { check } from 'k6'; import { Rate, Counter } from 'k6/metrics'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { grpcAddress } from './lib/target.js'; const BENCHMARK_NAME = 'grpcUnary'; @@ -43,8 +43,7 @@ const calls = new Counter('grpc_calls'); const grpcFailed = new Rate('grpc_req_failed'); export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/health_live.js b/benchmarks/src/main/resources/k6-scripts/health_live.js index 93324988..f9458ad2 100644 --- a/benchmarks/src/main/resources/k6-scripts/health_live.js +++ b/benchmarks/src/main/resources/k6-scripts/health_live.js @@ -19,14 +19,13 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; const BENCHMARK_NAME = 'healthLiveCheck'; const TARGET_URL = __ENV.TARGET_URL || 'https://api-sheriff:9000/q/health/live'; export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, // The benchmark stack terminates TLS with the self-signed localhost bundle mounted into // every service, so certificate verification is skipped for the load generator only. diff --git a/benchmarks/src/main/resources/k6-scripts/http2.js b/benchmarks/src/main/resources/k6-scripts/http2.js index 3dfbeb66..8f89bf73 100644 --- a/benchmarks/src/main/resources/k6-scripts/http2.js +++ b/benchmarks/src/main/resources/k6-scripts/http2.js @@ -17,7 +17,7 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'http2'; @@ -27,8 +27,7 @@ const TARGET_URL = __ENV.TARGET_URL || targetUrl('/proxy/static'); const EXPECTED_PROTO = 'HTTP/2.0'; export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/lib/summary.js b/benchmarks/src/main/resources/k6-scripts/lib/summary.js index 961b2606..0a3972f4 100644 --- a/benchmarks/src/main/resources/k6-scripts/lib/summary.js +++ b/benchmarks/src/main/resources/k6-scripts/lib/summary.js @@ -91,6 +91,12 @@ function round(value, digits = 2) { * clock captured in `setup()`: `handleSummary()` receives no setup data, and * `data.state.testRunDurationMs` is the engine's authoritative measured duration. * + * "Measured duration" means the WHOLE run, load phase plus graceful-stop tail -- and + * `requests_per_second` is the request counter's rate over that same denominator. A single VU + * that stalls at cutoff therefore stretches the denominator without adding requests and deflates + * the reported rate, which is why the tail is bounded at the scenario level; see + * {@link DEFAULT_GRACEFUL_STOP}. + * * A body-transfer aspect additionally opts into `throughput_mbps` via `options.throughput`. It is * derived from k6's own `data_sent` rate rather than from the body size times the request rate, * so a partially-transferred or rejected body cannot be counted as fully delivered bytes. The @@ -188,3 +194,53 @@ export function vus(fallback) { export function duration(fallback = '60s') { return __ENV.BENCHMARK_DURATION || fallback; } + +/** + * The graceful-stop window every aspect scenario bounds its tail with. + * + * The value is neither `0` nor k6's own default, and both exclusions are load-bearing: + * + * * **Not `0`.** A zero-length graceful stop interrupts every in-flight iteration at cutoff. + * Interrupted iterations register as failed requests, so at a high enough concurrency they + * can breach the `0.01` {@link maxErrorRate} ceiling (`BENCHMARK_MAX_ERROR_RATE`) and fail a + * run that measured perfectly well. The tail must exist; it must merely be bounded. + * * **Not k6's 30s default.** `requests_per_second` is a counter rate over + * `state.testRunDurationMs`, which spans the graceful-stop tail (see the module header). A + * single stalled VU keeps the run alive to the hard cap and adds the whole tail to the + * denominator while adding no requests, so on a 60s run the 30s default inflates the measured + * window by up to 50% and deflates the reported rate by the same factor -- large enough to + * read as a ~35% throughput regression that never happened. + * + * At `5s` the worst case a stalled VU can produce is `5/60` = 8.3% of window inflation, which + * stays inside the comparator's 10% window band and well inside its 15% throughput band, so a + * stall can no longer manufacture a regression verdict. + * + * @type {string} + */ +export const DEFAULT_GRACEFUL_STOP = '5s'; + +/** + * Builds the explicit `constant-vus` scenario every aspect script declares. + * + * The scenario form is chosen over k6's `vus` / `duration` shorthand precisely because the + * shorthand offers no way to bound the graceful stop: it silently applies the 30s default, and + * that default is what lets one stalled VU inflate the measured window (see + * {@link DEFAULT_GRACEFUL_STOP}). Declaring the scenario explicitly makes the tail a stated, + * bounded part of every aspect's contract instead of an inherited default. + * + * The tail is {@link DEFAULT_GRACEFUL_STOP} for every aspect, deliberately with no per-aspect + * override: one lane-wide value is what makes the two compared arms' measured windows commensurable + * in the first place. An aspect that ever needs a longer tail changes it here, for all of them. + * + * @param {number} vuCount the constant VU count to hold for the run + * @param {string} runDuration the k6 duration string the VUs are held for + * @returns {{executor: string, vus: number, duration: string, gracefulStop: string}} the scenario + */ +export function scenario(vuCount, runDuration) { + return { + executor: 'constant-vus', + vus: vuCount, + duration: runDuration, + gracefulStop: DEFAULT_GRACEFUL_STOP, + }; +} diff --git a/benchmarks/src/main/resources/k6-scripts/lib/target.js b/benchmarks/src/main/resources/k6-scripts/lib/target.js index d8190913..8163c9b8 100644 --- a/benchmarks/src/main/resources/k6-scripts/lib/target.js +++ b/benchmarks/src/main/resources/k6-scripts/lib/target.js @@ -14,6 +14,14 @@ * An unknown target is fatal rather than defaulted. Silently falling back to API Sheriff would * label an API Sheriff run as the other gateway in `gateway_target`, and a mislabelled comparison * artifact is worse than a failed run -- it is wrong data that reads as correct. + * + * One resolver deliberately sits OUTSIDE that per-target mapping: {@link passthroughEmptyUrl}. It + * addresses the dedicated `api-sheriff-passthrough-empty` gateway instance, which exists only to + * run one API Sheriff configuration (a `gateway.yaml` declaring no `tls.passthrough_sni`) against + * another. Routing it through {@link gatewayTarget} would make `GATEWAY_TARGET=apisix` silently + * point an API-Sheriff-versus-API-Sheriff comparison at APISIX, which has no such instance and no + * such configuration -- the run would still produce a summary, and that summary would be nonsense. + * The exclusion is the point, not an oversight. */ /** @@ -29,6 +37,27 @@ const BASE_URLS = { /** The target assumed when `GATEWAY_TARGET` is unset, keeping the CI lane plumbing-free. */ const DEFAULT_TARGET = 'api-sheriff'; +/** + * Base URL of the dedicated gateway instance whose `gateway.yaml` declares no + * `tls.passthrough_sni`, reached by compose service name on the shared `api-sheriff` network like + * every other edge. It is a second API Sheriff process, not a second gateway product, so it is + * absent from {@link BASE_URLS} and unreachable through {@link gatewayTarget} -- see the module + * `@fileoverview` for why that separation is deliberate. + * + * @type {string} + */ +export const PASSTHROUGH_EMPTY_BASE_URL = 'https://api-sheriff-passthrough-empty:8443'; + +/** + * Drops a single trailing slash so a base URL concatenates with a leading-slash path exactly once. + * + * @param {string} url the base URL to normalize + * @returns {string} the URL without a trailing slash + */ +function withoutTrailingSlash(url) { + return url.endsWith('/') ? url.slice(0, -1) : url; +} + /** * Resolves the gateway a run is taken against, validated against the supported set. * @@ -60,7 +89,7 @@ export function gatewayTarget() { export function baseUrl() { const override = __ENV.TARGET_BASE_URL; const resolved = override === undefined || override === '' ? BASE_URLS[gatewayTarget()] : override; - return resolved.endsWith('/') ? resolved.slice(0, -1) : resolved; + return withoutTrailingSlash(resolved); } /** @@ -73,6 +102,23 @@ export function targetUrl(path) { return `${baseUrl()}${path}`; } +/** + * Builds an absolute URL for a route path on the no-`passthrough_sni` gateway instance. + * + * `PASSTHROUGH_EMPTY_BASE_URL` overrides {@link PASSTHROUGH_EMPTY_BASE_URL} for a run against an + * edge that is not a compose service, in the same shape as `TARGET_BASE_URL` overrides the + * per-target mapping in {@link baseUrl}. It resolves independently of `GATEWAY_TARGET`, so this + * function returns the same host whichever gateway the surrounding run measures. + * + * @param {string} path the route path, with a leading slash (e.g. `/proxy/static`) + * @returns {string} the absolute URL to request + */ +export function passthroughEmptyUrl(path) { + const override = __ENV.PASSTHROUGH_EMPTY_BASE_URL; + const resolved = override === undefined || override === '' ? PASSTHROUGH_EMPTY_BASE_URL : override; + return `${withoutTrailingSlash(resolved)}${path}`; +} + /** * Builds an absolute WebSocket URL for a route path on the targeted gateway, reusing the same * host/target resolution as {@link targetUrl} but on the `wss://` scheme the WebSocket upgrade diff --git a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js index 5c9b3954..7633a852 100644 --- a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js +++ b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js @@ -9,22 +9,27 @@ * the backend completes the handshake and presents its *own* certificate. Measures * relay throughput/latency through the active passthrough path. Emits `passthroughRelay`. * - * empty -> drive the *same proxied static route* the PLAN-04 `unauth` baseline uses, but with - * `passthrough_sni` empty — D1's zero-overhead default, where the front listener is - * never created and the terminated Quarkus HTTPS listener owns the public port directly. - * This is the no-regression comparison side: `PassthroughBaselineComparator` reads its - * summary against the stored PLAN-04 `proxiedStatic` baseline. Emits `passthroughRelayEmpty`. + * empty -> drive the *same proxied static route* against a *different gateway instance*: the + * dedicated `api-sheriff-passthrough-empty` service, whose overlaid `gateway.yaml` + * declares no `tls.passthrough_sni` at all. `passthrough_sni` is a property of the + * whole gateway process — declaring it non-empty is what starts the accept-time SNI + * front listener — so emptiness cannot be selected per request on the primary + * instance, which declares two entries for its whole lifetime. On this instance the + * front listener is never created and Quarkus terminates TLS on the public port + * directly. This is the no-regression comparison side: `PassthroughBaselineComparator` + * reads its summary against the primary instance's `proxiedStatic` baseline over the + * same route and the same nginx-static upstream. Emits `passthroughRelayEmpty`. * * One script body drives both modes so the two runs share identical VU/duration/threshold plumbing - * and only the SNI/route differs. Native k6 thresholds (`http_req_failed`, `checks`) gate the run, + * and only the target edge and route differ. Native k6 thresholds (`http_req_failed`, `checks`) gate the run, * exactly as the other aspects: a run that starts rejecting every request exits non-zero rather than * benchmarking as an improvement. An unknown `PASSTHROUGH_SNI` value is fatal rather than defaulted, * mirroring `lib/target.js` — a mislabelled run is worse than a failed one. */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; -import { targetUrl } from './lib/target.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { passthroughEmptyUrl } from './lib/target.js'; /** The passthrough mode this run measures, defaulting to the active relay path. */ const MODE = (__ENV.PASSTHROUGH_SNI || 'mapped').toLowerCase(); @@ -54,7 +59,7 @@ function resolveMode() { case 'mapped': return { benchmarkName: 'passthroughRelay', url: __ENV.TARGET_URL || PASSTHROUGH_TARGET_URL }; case 'empty': - return { benchmarkName: 'passthroughRelayEmpty', url: __ENV.TARGET_URL || targetUrl('/proxy/static') }; + return { benchmarkName: 'passthroughRelayEmpty', url: __ENV.TARGET_URL || passthroughEmptyUrl('/proxy/static') }; default: throw new Error(`PASSTHROUGH_SNI must be one of mapped, empty, got "${__ENV.PASSTHROUGH_SNI}"`); } @@ -63,8 +68,7 @@ function resolveMode() { const { benchmarkName: BENCHMARK_NAME, url: TARGET_URL } = resolveMode(); export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/proxied_static.js b/benchmarks/src/main/resources/k6-scripts/proxied_static.js index c72a059f..880bdf59 100644 --- a/benchmarks/src/main/resources/k6-scripts/proxied_static.js +++ b/benchmarks/src/main/resources/k6-scripts/proxied_static.js @@ -8,15 +8,14 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'proxiedStatic'; const TARGET_URL = __ENV.TARGET_URL || targetUrl('/proxy/static'); export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/session_mediated.js b/benchmarks/src/main/resources/k6-scripts/session_mediated.js index 148c4b78..0694cea2 100644 --- a/benchmarks/src/main/resources/k6-scripts/session_mediated.js +++ b/benchmarks/src/main/resources/k6-scripts/session_mediated.js @@ -28,7 +28,7 @@ */ import http from 'k6/http'; import { check, fail } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { baseUrl, targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'sessionMediated'; @@ -55,8 +55,7 @@ const KEYCLOAK_USERNAME = __ENV.KEYCLOAK_USERNAME || 'integration-user'; const KEYCLOAK_PASSWORD = __ENV.KEYCLOAK_PASSWORD || 'integration-password'; export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/upload_large.js b/benchmarks/src/main/resources/k6-scripts/upload_large.js index f449108e..7ceccc61 100644 --- a/benchmarks/src/main/resources/k6-scripts/upload_large.js +++ b/benchmarks/src/main/resources/k6-scripts/upload_large.js @@ -23,7 +23,7 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'uploadLarge'; @@ -40,8 +40,7 @@ const LARGE_UPLOAD_VUS = 5; const BODY = 'x'.repeat(BODY_BYTES); export const options = { - vus: vus(LARGE_UPLOAD_VUS), - duration: duration(), + scenarios: { default: scenario(vus(LARGE_UPLOAD_VUS), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/upload_small.js b/benchmarks/src/main/resources/k6-scripts/upload_small.js index 61aec208..bfc2e791 100644 --- a/benchmarks/src/main/resources/k6-scripts/upload_small.js +++ b/benchmarks/src/main/resources/k6-scripts/upload_small.js @@ -15,7 +15,7 @@ */ import http from 'k6/http'; import { check } from 'k6'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { targetUrl } from './lib/target.js'; const BENCHMARK_NAME = 'uploadSmall'; @@ -30,8 +30,7 @@ const BODY_BYTES = 1e6; const BODY = 'x'.repeat(BODY_BYTES); export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/main/resources/k6-scripts/websocket_echo.js b/benchmarks/src/main/resources/k6-scripts/websocket_echo.js index fcf733fe..c6473c61 100644 --- a/benchmarks/src/main/resources/k6-scripts/websocket_echo.js +++ b/benchmarks/src/main/resources/k6-scripts/websocket_echo.js @@ -21,7 +21,7 @@ import { WebSocket } from 'k6/websockets'; import { check } from 'k6'; import { Trend, Rate, Counter } from 'k6/metrics'; -import { buildSummary, duration, maxErrorRate, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; +import { buildSummary, duration, maxErrorRate, scenario, SUMMARY_TREND_STATS, vus } from './lib/summary.js'; import { wsUrl } from './lib/target.js'; const BENCHMARK_NAME = 'websocketEcho'; @@ -45,8 +45,7 @@ const roundtrips = new Counter('ws_roundtrips'); const wsErrors = new Rate('ws_errors'); export const options = { - vus: vus(50), - duration: duration(), + scenarios: { default: scenario(vus(50), duration()) }, summaryTrendStats: SUMMARY_TREND_STATS, insecureSkipTLSVerify: true, thresholds: { diff --git a/benchmarks/src/test/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparatorTest.java b/benchmarks/src/test/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparatorTest.java index 2fe55358..b49cc56f 100644 --- a/benchmarks/src/test/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparatorTest.java +++ b/benchmarks/src/test/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparatorTest.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; +import java.time.Instant; import java.util.Locale; import java.util.Optional; @@ -39,6 +40,21 @@ * failure. The band tests are property-style: CUI Test Generator supplies fresh baselines and * within-/beyond-band deltas each repetition, so the verdict is exercised across a range of * magnitudes rather than a single hand-picked pair. + *

+ * The measurement-window precondition is asserted on the same terms, against the field shape of CI run + * 30872335137: two arms that served the same request count over windows differing by ~1.56x, whose + * counter-derived rates therefore differed by the same factor with nothing having slowed down. That + * pair must be classified {@link Verdict#WINDOW_MISMATCH} and must fail the run, while an equal-window + * collapse must still be attributed to throughput and an absent window pair must stay + * {@link Verdict#NOT_MEASURED}. + *

+ * Two further properties of that precondition are pinned here. First, the band is derived from + * the configured load-phase duration rather than fixed: the bounded graceful-stop tail it must + * tolerate is an absolute 5s and therefore a different fraction of every duration, so the fixed 10% + * band rejected a perfectly healthy {@code -Pquick} run (5/30 = 16.7%). Second, agreement alone never + * establishes comparability — two zero-length windows, and two end-before-start ones, agree perfectly + * while describing no interval at all, and must be refused rather than allowed to license a rate + * computed over an invalid denominator. */ @EnableGeneratorController class PassthroughBaselineComparatorTest { @@ -46,6 +62,43 @@ class PassthroughBaselineComparatorTest { /** The comparator's own default band, so the tests track it if the production default changes. */ private static final double TOLERANCE = PassthroughBaselineComparator.DEFAULT_TOLERANCE; + /** The comparator's own window band, for the same reason — never a hand-copied literal. */ + private static final double WINDOW_TOLERANCE = PassthroughBaselineComparator.DEFAULT_WINDOW_TOLERANCE; + + /** The label the comparator gives the measurement-window precondition row. */ + private static final String WINDOW_LABEL = "measurement window"; + + /** The label the comparator gives the throughput row. */ + private static final String THROUGHPUT_LABEL = "throughput"; + + /** The nominal 60s k6 load phase both arms declare. */ + private static final double BASELINE_WINDOW_MILLIS = 60_000.0; + + /** + * The request count both arms of CI run 30872335137 carried — the same work on both sides, which + * is what makes that run's rate difference an artifact of the window rather than of throughput. + */ + private static final double FIELD_SHAPE_REQUESTS = 480_000.0; + + /** + * The window inflation the candidate arm of run 30872335137 carried: its measured window ran about + * 1.56x the baseline's, which deflated its counter-derived rate by the same factor and produced the + * false ~35.8% throughput verdict PLAN-46 diagnosed. + */ + private static final double FIELD_SHAPE_WINDOW_RATIO = 1.56; + + /** An arbitrary but fixed window start; only the two windows' lengths are under test. */ + private static final Instant WINDOW_START = Instant.parse("2026-08-04T10:00:00Z"); + + /** The load phase the {@code quick} benchmark profile configures, in seconds. */ + private static final double QUICK_LOAD_DURATION_SECONDS = 30.0; + + /** The bounded graceful-stop tail every aspect scenario declares, in milliseconds. */ + private static final double GRACEFUL_STOP_MILLIS = 5_000.0; + + /** The 30s load phase a {@code -Pquick} run measures over, in milliseconds. */ + private static final double QUICK_WINDOW_MILLIS = QUICK_LOAD_DURATION_SECONDS * 1_000.0; + /** Builds a k6-shaped summary carrying a top-level throughput and both latency percentiles. */ private static JsonObject summary(double rps, double p50, double p99) { return JsonParser.parseString(""" @@ -57,6 +110,41 @@ private static JsonObject summary(double rps, double p50, double p99) { """.formatted(rps, p50, p99)).getAsJsonObject(); } + /** + * The same shape as {@link #summary(double, double, double)} plus the {@code start_time} / + * {@code end_time} pair k6 emits, so the measurement window is derivable the way it is in the field. + */ + private static JsonObject summary(double rps, double p50, double p99, double windowMillis) { + return JsonParser.parseString(""" + { + "requests_per_second": %s, + "error_rate": 0.0, + "latency_ms": { "avg": 4.0, "p50": %s, "p99": %s }, + "start_time": "%s", + "end_time": "%s" + } + """.formatted(rps, p50, p99, WINDOW_START, + WINDOW_START.plusMillis((long) windowMillis))).getAsJsonObject(); + } + + /** + * The counter-derived rate k6 reports: a request count divided by the whole measured window. This + * is the arithmetic that makes a stretched window read as a throughput collapse. + */ + private static double ratePerSecond(double requests, double windowMillis) { + return requests / (windowMillis / 1000.0); + } + + /** The verdict on one labelled row of a comparison, failing loudly when the row is absent. */ + private static Verdict verdictOf(ComparisonResult result, String label) { + return result.metrics().stream() + .filter(metric -> label.equals(metric.label())) + .findFirst() + .orElseThrow(() -> new AssertionError( + "no metric labelled '" + label + "' in " + result.metrics())) + .verdict(); + } + // ---- Throughput (higher-is-better) band ------------------------------------------------------ @RepeatedTest(25) @@ -243,7 +331,7 @@ void resolveToleranceDefaultsWhenThePropertyIsUnset() { assertEquals(PassthroughBaselineComparator.DEFAULT_TOLERANCE, PassthroughBaselineComparator.resolveTolerance()); } finally { - restoreToleranceProperty(prior); + restoreProperty(PassthroughBaselineComparator.TOLERANCE_PROPERTY, prior); } } @@ -257,7 +345,7 @@ void resolveToleranceReadsAValidOverride() { // Act + Assert assertEquals(0.25, PassthroughBaselineComparator.resolveTolerance()); } finally { - restoreToleranceProperty(prior); + restoreProperty(PassthroughBaselineComparator.TOLERANCE_PROPERTY, prior); } } @@ -281,15 +369,451 @@ void resolveToleranceRejectsAnOutOfRangeOrMalformedOverride() { PassthroughBaselineComparator::resolveTolerance, "a malformed value must be rejected rather than silently defaulted"); } finally { - restoreToleranceProperty(prior); + restoreProperty(PassthroughBaselineComparator.TOLERANCE_PROPERTY, prior); } } - private static void restoreToleranceProperty(String prior) { + private static void restoreProperty(String property, String prior) { if (prior == null) { - System.clearProperty(PassthroughBaselineComparator.TOLERANCE_PROPERTY); + System.clearProperty(property); } else { - System.setProperty(PassthroughBaselineComparator.TOLERANCE_PROPERTY, prior); + System.setProperty(property, prior); + } + } + + // ---- Measurement window: the precondition, not a metric -------------------------------------- + + @Test + void sameWorkOverAnInflatedWindowIsClassifiedWindowMismatchRatherThanRegression() { + // Arrange -- the field shape from CI run 30872335137: both arms served the SAME request count, + // but the candidate's measured window ran ~1.56x the baseline's, so its counter-derived rate + // came out proportionally lower without a single request being served more slowly. + double candidateWindow = BASELINE_WINDOW_MILLIS * FIELD_SHAPE_WINDOW_RATIO; + JsonObject baseline = summary(ratePerSecond(FIELD_SHAPE_REQUESTS, BASELINE_WINDOW_MILLIS), + 2.0, 30.0, BASELINE_WINDOW_MILLIS); + JsonObject candidate = summary(ratePerSecond(FIELD_SHAPE_REQUESTS, candidateWindow), + 2.0, 30.0, candidateWindow); + + // Act + ComparisonResult result = PassthroughBaselineComparator.compare(candidate, baseline, TOLERANCE); + + // Assert -- the pair is refused as incomparable, and it is the WINDOW row that says so. + assertEquals(Verdict.WINDOW_MISMATCH, verdictOf(result, WINDOW_LABEL), + "a %.2fx window inflation must exceed the %.2f window band".formatted( + FIELD_SHAPE_WINDOW_RATIO, WINDOW_TOLERANCE)); + assertTrue(result.windowMismatched(), + "an incomparable window pair must be reported as a window mismatch"); + } + + @Test + void anIncomparableWindowPairFailsTheRun() { + // Arrange -- the same field shape as above. + double candidateWindow = BASELINE_WINDOW_MILLIS * FIELD_SHAPE_WINDOW_RATIO; + JsonObject baseline = summary(ratePerSecond(FIELD_SHAPE_REQUESTS, BASELINE_WINDOW_MILLIS), + 2.0, 30.0, BASELINE_WINDOW_MILLIS); + JsonObject candidate = summary(ratePerSecond(FIELD_SHAPE_REQUESTS, candidateWindow), + 2.0, 30.0, candidateWindow); + + // Act + ComparisonResult result = PassthroughBaselineComparator.compare(candidate, baseline, TOLERANCE); + + // Assert -- pins the operator's ruling as executable behaviour: an incomparable pair FAILS the + // run. It is deliberately not rescaled to a common window and not downgraded to a warning. + assertTrue(result.regressed(), "a window mismatch must fail the run, not merely warn"); + } + + @Test + void theInflatedWindowPairReadsAsAThroughputRegressionWithoutTheWindowPrecondition() { + // Arrange -- the same two rates, compared the way the pre-window comparator did it: throughput + // alone, with no window row to refuse the pair first. + double candidateWindow = BASELINE_WINDOW_MILLIS * FIELD_SHAPE_WINDOW_RATIO; + double baselineRate = ratePerSecond(FIELD_SHAPE_REQUESTS, BASELINE_WINDOW_MILLIS); + double candidateRate = ratePerSecond(FIELD_SHAPE_REQUESTS, candidateWindow); + + // Act + Verdict throughputOnly = PassthroughBaselineComparator.throughputVerdict( + Optional.of(candidateRate), Optional.of(baselineRate), TOLERANCE); + + // Assert -- fail-without-fix, made explicit: on exactly this pair the throughput row alone says + // REGRESSION (the ~35.8% drop that no request ever experienced), which is the false verdict the + // lane shipped. The fix does not change this arithmetic -- it adds the window row that runs + // first, so the same pair is now refused as incomparable instead of blamed on throughput. + assertEquals(Verdict.REGRESSION, throughputOnly, + () -> "candidate %.2f vs baseline %.2f RPS is a %.1f%% drop on paper" + .formatted(candidateRate, baselineRate, + (1.0 - candidateRate / baselineRate) * 100.0)); + } + + @Test + void anEqualWindowThroughputCollapseStillRegressesOnThroughput() { + // Arrange -- the matched negative control for the case above: windows identical, so the window + // precondition holds and the throughput row is believable. Half the work in the same window. + JsonObject baseline = summary(ratePerSecond(FIELD_SHAPE_REQUESTS, BASELINE_WINDOW_MILLIS), + 2.0, 30.0, BASELINE_WINDOW_MILLIS); + JsonObject candidate = summary(ratePerSecond(FIELD_SHAPE_REQUESTS * 0.5, BASELINE_WINDOW_MILLIS), + 2.0, 30.0, BASELINE_WINDOW_MILLIS); + + // Act + ComparisonResult result = PassthroughBaselineComparator.compare(candidate, baseline, TOLERANCE); + + // Assert -- the window guard must not swallow a genuine collapse: this one is still a + // REGRESSION, attributed to throughput rather than to the measurement. + assertEquals(Verdict.PASS, verdictOf(result, WINDOW_LABEL), + "identical windows must be comparable"); + assertEquals(Verdict.REGRESSION, verdictOf(result, THROUGHPUT_LABEL), + "a genuine equal-window throughput collapse must still regress"); + assertFalse(result.windowMismatched(), + "a genuine collapse must not be misreported as a window mismatch"); + assertTrue(result.regressed(), "a genuine throughput collapse must fail the run"); + } + + @Test + void anAbsentWindowPairIsNotMeasuredAndNeverFailsTheRun() { + // Arrange -- summaries in the pre-window shape, carrying no start_time / end_time at all, with + // every other metric identical so nothing else could fail the run. + JsonObject baseline = summary(8_000.0, 2.0, 30.0); + JsonObject candidate = summary(8_000.0, 2.0, 30.0); + + // Act + ComparisonResult result = PassthroughBaselineComparator.compare(candidate, baseline, TOLERANCE); + + // Assert -- an older summary shape is not retroactively broken: absent is NOT_MEASURED, never a + // zero-length window that would read as maximally mismatched. + assertEquals(Verdict.NOT_MEASURED, verdictOf(result, WINDOW_LABEL), + "an absent window pair must be NOT_MEASURED"); + assertFalse(result.windowMismatched(), "an absent window pair must not report a mismatch"); + assertFalse(result.regressed(), "an absent window pair must never fail the run"); + } + + @Test + void aWindowMissingOnEitherSideAloneIsNotMeasured() { + // Arrange + double measured = Generators.doubles(1_000.0, 120_000.0).next(); + + // Act + Assert -- one-sided absence is still absence, in both directions. + assertEquals(Verdict.NOT_MEASURED, PassthroughBaselineComparator.windowVerdict( + Optional.empty(), Optional.of(measured), WINDOW_TOLERANCE)); + assertEquals(Verdict.NOT_MEASURED, PassthroughBaselineComparator.windowVerdict( + Optional.of(measured), Optional.empty(), WINDOW_TOLERANCE)); + } + + @RepeatedTest(25) + void windowsAgreeingInsideTheBandAreComparable() { + // Arrange -- a drift strictly inside the window band, in either direction. + double baseline = Generators.doubles(10_000.0, 300_000.0).next(); + double driftFraction = Generators.doubles(-WINDOW_TOLERANCE * 0.95, WINDOW_TOLERANCE * 0.95).next(); + double candidate = baseline * (1.0 + driftFraction); + + // Act + Verdict verdict = PassthroughBaselineComparator.windowVerdict( + Optional.of(candidate), Optional.of(baseline), WINDOW_TOLERANCE); + + // Assert + assertEquals(Verdict.PASS, verdict, + () -> "candidate window %.2f vs baseline %.2f (drift %.4f) is within the %.2f band" + .formatted(candidate, baseline, driftFraction, WINDOW_TOLERANCE)); + } + + @RepeatedTest(25) + void windowsDisagreeingBeyondTheBandAreIncomparable() { + // Arrange -- a drift beyond the window band, in either direction: a window that ran short is as + // incomparable as one that ran long, because the rate is a quotient either way. + double baseline = Generators.doubles(10_000.0, 300_000.0).next(); + double magnitude = Generators.doubles(WINDOW_TOLERANCE * 1.05, 0.9).next(); + double driftFraction = Generators.booleans().next() ? magnitude : -magnitude; + double candidate = baseline * (1.0 + driftFraction); + + // Act + Verdict verdict = PassthroughBaselineComparator.windowVerdict( + Optional.of(candidate), Optional.of(baseline), WINDOW_TOLERANCE); + + // Assert + assertEquals(Verdict.WINDOW_MISMATCH, verdict, + () -> "candidate window %.2f vs baseline %.2f (drift %.4f) exceeds the %.2f band" + .formatted(candidate, baseline, driftFraction, WINDOW_TOLERANCE)); + } + + @Test + void theBoundedGracefulStopTailStaysInsideTheWindowBandWhileTheObservedInflationDoesNot() { + // Arrange -- the two magnitudes the band was chosen between: the worst case the bounded 5s + // gracefulStop can add to a 60s load phase, and the inflation run 30872335137 actually carried. + double healthyWorstCase = BASELINE_WINDOW_MILLIS + 5_000.0; + double observedInflation = BASELINE_WINDOW_MILLIS * FIELD_SHAPE_WINDOW_RATIO; + + // Act + Verdict healthy = PassthroughBaselineComparator.windowVerdict( + Optional.of(healthyWorstCase), Optional.of(BASELINE_WINDOW_MILLIS), WINDOW_TOLERANCE); + Verdict inflated = PassthroughBaselineComparator.windowVerdict( + Optional.of(observedInflation), Optional.of(BASELINE_WINDOW_MILLIS), WINDOW_TOLERANCE); + + // Assert -- the band must separate the two, or it is either useless or permanently red. + assertEquals(Verdict.PASS, healthy, + "a bounded 5s tail on a 60s run must never trip the window band"); + assertEquals(Verdict.WINDOW_MISMATCH, inflated, + "the observed window inflation must trip the window band"); + } + + @Test + void windowMillisReadsTheIsoPairAndTreatsAnAbsentOrMalformedPairAsNotMeasured() { + // Arrange + JsonObject windowed = summary(8_000.0, 2.0, 30.0, BASELINE_WINDOW_MILLIS); + JsonObject withoutWindow = summary(8_000.0, 2.0, 30.0); + JsonObject malformed = JsonParser.parseString(""" + { "start_time": "not-a-timestamp", "end_time": "2026-08-04T10:01:00Z" } + """).getAsJsonObject(); + + // Act + Assert -- a measured pair yields the window length in millis; an absent or unparseable + // pair yields empty rather than a 0 that would read as a maximally mismatched window. + assertEquals(Optional.of(BASELINE_WINDOW_MILLIS), + PassthroughBaselineComparator.windowMillis(windowed)); + assertTrue(PassthroughBaselineComparator.windowMillis(withoutWindow).isEmpty(), + "an absent start/end pair must be empty"); + assertTrue(PassthroughBaselineComparator.windowMillis(malformed).isEmpty(), + "an unparseable timestamp must be empty, never a zero-length window"); + } + + // ---- A window that is not positive is not a measurement at all ------------------------------- + + @Test + void agreeingWindowsThatAreNotPositiveAreStillIncomparable() { + // Arrange -- the two pairs that agree perfectly with each other while describing no interval + // any rate could have been measured over: two zero-length windows, and two whose end_time + // precedes their start_time by the same amount. + double negative = -Generators.doubles(1_000.0, 60_000.0).next(); + + // Act + Verdict bothZero = PassthroughBaselineComparator.windowVerdict( + Optional.of(0.0), Optional.of(0.0), WINDOW_TOLERANCE); + Verdict bothNegative = PassthroughBaselineComparator.windowVerdict( + Optional.of(negative), Optional.of(negative), WINDOW_TOLERANCE); + + // Assert -- absolute agreement is necessary for comparability but never sufficient. Comparing + // only the drift would score both pairs as a perfect match and hand the throughput row a + // quotient taken over an invalid denominator. + assertAll("non-positive windows", + () -> assertEquals(Verdict.WINDOW_MISMATCH, bothZero, + "two zero-length windows agree, but neither measured anything"), + () -> assertEquals(Verdict.WINDOW_MISMATCH, bothNegative, + () -> "two end-before-start windows (%.2f ms) agree, but neither is an interval" + .formatted(negative))); + } + + @Test + void aNonPositiveWindowOnEitherArmAloneIsIncomparable() { + // Arrange -- one healthy window, one that is not an interval, in both orders and both shapes. + Optional healthy = Optional.of(BASELINE_WINDOW_MILLIS); + + // Act + Assert -- a valid arm never rescues an invalid one, whichever side carries it. + assertAll("one-sided non-positive windows", + () -> assertEquals(Verdict.WINDOW_MISMATCH, PassthroughBaselineComparator.windowVerdict( + Optional.of(0.0), healthy, WINDOW_TOLERANCE)), + () -> assertEquals(Verdict.WINDOW_MISMATCH, PassthroughBaselineComparator.windowVerdict( + healthy, Optional.of(0.0), WINDOW_TOLERANCE)), + () -> assertEquals(Verdict.WINDOW_MISMATCH, PassthroughBaselineComparator.windowVerdict( + Optional.of(-1_000.0), healthy, WINDOW_TOLERANCE)), + () -> assertEquals(Verdict.WINDOW_MISMATCH, PassthroughBaselineComparator.windowVerdict( + healthy, Optional.of(-1_000.0), WINDOW_TOLERANCE))); + } + + @Test + void summariesCarryingNonPositiveWindowsFailTheRunRatherThanComparingRates() { + // Arrange -- two k6-shaped summaries whose start_time equals their end_time, with identical + // rates so nothing but the window could fail the run; and the end-before-start counterpart. + JsonObject zeroBaseline = summary(8_000.0, 2.0, 30.0, 0.0); + JsonObject zeroCandidate = summary(8_000.0, 2.0, 30.0, 0.0); + JsonObject invertedBaseline = summary(8_000.0, 2.0, 30.0, -5_000.0); + JsonObject invertedCandidate = summary(8_000.0, 2.0, 30.0, -5_000.0); + + // Act + ComparisonResult zero = PassthroughBaselineComparator.compare(zeroCandidate, zeroBaseline, TOLERANCE); + ComparisonResult inverted = PassthroughBaselineComparator.compare( + invertedCandidate, invertedBaseline, TOLERANCE); + + // Assert -- the pair is refused end to end, and it is the WINDOW row that says so. A present + // but invalid window is deliberately NOT treated as an absent one: absence is the pre-window + // summary shape and must never fail, while a measured-but-impossible interval is a broken + // measurement the operator ruling says to refuse. + assertAll("non-positive windows end to end", + () -> assertEquals(Verdict.WINDOW_MISMATCH, verdictOf(zero, WINDOW_LABEL)), + () -> assertTrue(zero.windowMismatched(), "a zero-length window pair must be refused"), + () -> assertTrue(zero.regressed(), "a zero-length window pair must fail the run"), + () -> assertEquals(Verdict.WINDOW_MISMATCH, verdictOf(inverted, WINDOW_LABEL)), + () -> assertTrue(inverted.windowMismatched(), + "an end-before-start window pair must be refused"), + () -> assertTrue(inverted.regressed(), + "an end-before-start window pair must fail the run")); + } + + // ---- Window-tolerance resolution from the system property ------------------------------------- + + @Test + void resolveWindowToleranceDefaultsWhenNeitherPropertyIsSet() { + // Arrange -- neither the explicit band nor the effective load duration is supplied. + String priorBand = System.getProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY); + String priorDuration = System.getProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY); + System.clearProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY); + System.clearProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY); + try { + // Act + Assert -- the fallback is the band for the lane's own 60s default duration. + assertEquals(PassthroughBaselineComparator.DEFAULT_WINDOW_TOLERANCE, + PassthroughBaselineComparator.resolveWindowTolerance()); + } finally { + restoreProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, priorBand); + restoreProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, priorDuration); + } + } + + @Test + void resolveWindowToleranceReadsAValidOverride() { + // Arrange + String prior = System.getProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY); + try { + System.setProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, "0.03"); + + // Act + Assert + assertEquals(0.03, PassthroughBaselineComparator.resolveWindowTolerance()); + } finally { + restoreProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, prior); + } + } + + // ---- The band is derived from the configured load phase, not fixed --------------------------- + + @Test + void theDerivedBandCoversTheGracefulStopTailAtEveryConfiguredDuration() { + // Arrange -- the worst case a healthy run can produce is the SAME absolute 5s tail whatever + // the load phase is, so it is a different fraction of each one. + double defaultDuration = PassthroughBaselineComparator.DEFAULT_LOAD_DURATION_SECONDS; + double tailFractionOfDefault = 5.0 / defaultDuration; + double tailFractionOfQuick = 5.0 / QUICK_LOAD_DURATION_SECONDS; + + // Act + double defaultBand = PassthroughBaselineComparator.derivedWindowTolerance(defaultDuration); + double quickBand = PassthroughBaselineComparator.derivedWindowTolerance(QUICK_LOAD_DURATION_SECONDS); + + // Assert -- each band clears its own duration's worst case, the quick band is the wider of the + // two, and the 60s band is still exactly the documented default. + assertAll("derived window bands", + () -> assertEquals(PassthroughBaselineComparator.DEFAULT_WINDOW_TOLERANCE, defaultBand, + "the 60s band must remain the documented default"), + () -> assertTrue(defaultBand > tailFractionOfDefault, + () -> "%.4f must cover the 5/60 tail (%.4f)".formatted(defaultBand, tailFractionOfDefault)), + () -> assertTrue(quickBand > tailFractionOfQuick, + () -> "%.4f must cover the 5/30 tail (%.4f)".formatted(quickBand, tailFractionOfQuick)), + () -> assertTrue(quickBand > defaultBand, + "a shorter load phase must widen the band, not leave it unchanged"), + () -> assertTrue(quickBand <= 1.0, "a band above 1 would disable the precondition")); + } + + @Test + void aHealthyQuickProfileRunIsNotRejectedAsAWindowMismatch() { + // Arrange -- the worst case a -Pquick run can legitimately produce: one arm consumed the whole + // bounded 5s gracefulStop on top of the profile's 30s load phase, the other consumed none. + double inflated = QUICK_WINDOW_MILLIS + GRACEFUL_STOP_MILLIS; + double quickBand = PassthroughBaselineComparator.derivedWindowTolerance(QUICK_LOAD_DURATION_SECONDS); + + // Act + Verdict derived = PassthroughBaselineComparator.windowVerdict( + Optional.of(inflated), Optional.of(QUICK_WINDOW_MILLIS), quickBand); + Verdict underTheFixedDefault = PassthroughBaselineComparator.windowVerdict( + Optional.of(inflated), Optional.of(QUICK_WINDOW_MILLIS), WINDOW_TOLERANCE); + + // Assert -- fail-without-fix, made explicit: the fixed 10% band rejects this perfectly healthy + // quick run (5/30 = 16.7% > 10%), which is the defect. The derived band accepts it, while a + // reverted/unbounded 30s tail on the same 30s phase (100% drift) still fails. + assertEquals(Verdict.PASS, derived, + () -> "a bounded 5s tail on a %.0fs load phase must stay inside the %.4f derived band" + .formatted(QUICK_LOAD_DURATION_SECONDS, quickBand)); + assertEquals(Verdict.WINDOW_MISMATCH, underTheFixedDefault, + "the fixed 60s band under-covers a quick run -- this is what the derivation fixes"); + assertEquals(Verdict.WINDOW_MISMATCH, PassthroughBaselineComparator.windowVerdict( + Optional.of(QUICK_WINDOW_MILLIS * 2.0), Optional.of(QUICK_WINDOW_MILLIS), quickBand), + "an unbounded tail must still trip the widened band"); + } + + @Test + void resolveWindowToleranceDerivesTheBandFromTheEffectiveLoadDuration() { + // Arrange + String priorBand = System.getProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY); + String priorDuration = System.getProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY); + System.clearProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY); + try { + System.setProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, "30s"); + + // Act + Assert -- the k6 duration string the POM forwards drives the band. + assertEquals(PassthroughBaselineComparator.derivedWindowTolerance(QUICK_LOAD_DURATION_SECONDS), + PassthroughBaselineComparator.resolveWindowTolerance()); + + // And an explicit band still wins over the derived one. + System.setProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, "0.05"); + assertEquals(0.05, PassthroughBaselineComparator.resolveWindowTolerance(), + "an explicit override must still take precedence over the derivation"); + } finally { + restoreProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, priorBand); + restoreProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, priorDuration); + } + } + + @Test + void loadDurationResolutionReadsK6DurationStringsAndRejectsMalformedOnes() { + // Arrange + String prior = System.getProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY); + try { + // Act + Assert -- the k6 duration grammar the POM's k6.duration value is written in. + assertAll("k6 duration parsing", + () -> assertEquals(60.0, PassthroughBaselineComparator.parseDurationSeconds("60s")), + () -> assertEquals(30.0, PassthroughBaselineComparator.parseDurationSeconds("30s")), + () -> assertEquals(0.5, PassthroughBaselineComparator.parseDurationSeconds("500ms")), + () -> assertEquals(90.0, PassthroughBaselineComparator.parseDurationSeconds("1m30s")), + () -> assertEquals(3600.0, PassthroughBaselineComparator.parseDurationSeconds("1h"))); + + System.clearProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY); + assertEquals(PassthroughBaselineComparator.DEFAULT_LOAD_DURATION_SECONDS, + PassthroughBaselineComparator.resolveLoadDurationSeconds(), + "an unset duration falls back to the lane's own default"); + + // A set-but-unreadable duration is fatal, never silently the 60s default: computing the + // band for a run that never happened is the failure mode the derivation exists to remove. + System.setProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, "60"); + assertThrows(IllegalArgumentException.class, + PassthroughBaselineComparator::resolveLoadDurationSeconds, + "a unit-less value must be rejected rather than guessed at"); + + System.setProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, "not-a-duration"); + assertThrows(IllegalArgumentException.class, + PassthroughBaselineComparator::resolveLoadDurationSeconds, + "a malformed value must be rejected rather than silently defaulted"); + + System.setProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, "0s"); + assertThrows(IllegalArgumentException.class, + PassthroughBaselineComparator::resolveLoadDurationSeconds, + "a zero-length load phase has no band to derive and must be rejected"); + } finally { + restoreProperty(PassthroughBaselineComparator.LOAD_DURATION_PROPERTY, prior); + } + } + + @Test + void resolveWindowToleranceRejectsAnOutOfRangeOrMalformedOverride() { + // Arrange + String prior = System.getProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY); + try { + System.setProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, "1.5"); + assertThrows(IllegalArgumentException.class, + PassthroughBaselineComparator::resolveWindowTolerance, + "a value above 1 would disable the window precondition and must be rejected"); + + System.setProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, "-0.1"); + assertThrows(IllegalArgumentException.class, + PassthroughBaselineComparator::resolveWindowTolerance, + "a negative value would invert the window precondition and must be rejected"); + + System.setProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, "not-a-number"); + assertThrows(IllegalArgumentException.class, + PassthroughBaselineComparator::resolveWindowTolerance, + "a malformed value must be rejected rather than silently defaulted"); + } finally { + restoreProperty(PassthroughBaselineComparator.WINDOW_TOLERANCE_PROPERTY, prior); } } } diff --git a/doc/LogMessages.adoc b/doc/LogMessages.adoc index c70f40f6..ad986e93 100644 --- a/doc/LogMessages.adoc +++ b/doc/LogMessages.adoc @@ -101,6 +101,8 @@ section lists only the k6-specific records, whose upstream counterparts are wrk- |K6Benchmark-2 |BENCHMARK |Parsed k6 summary for benchmark '%s' (target '%s') |Logged once per parsed summary document, recording the benchmark name and the gateway the run targeted |K6Benchmark-3 |BENCHMARK |Rendering comparison summary from %s for targets '%s' and '%s' |Logged once when the on-demand comparative side-by-side summary starts rendering |K6Benchmark-4 |BENCHMARK |Wrote comparison summary covering %s aspect(s) to %s |Logged once when the comparative summary artifact has been written, outside the gh-pages tree +|K6Benchmark-5 |BENCHMARK |Comparing empty-passthrough_sni run '%s' against PLAN-04 baseline '%s' |Logged once when the empty-`passthrough_sni` no-regression comparison starts, naming both summary documents it reads +|K6Benchmark-6 |BENCHMARK |Passthrough empty-mode no-regression check passed within the %s noise band |Logged once when the empty-mode run stayed inside the no-regression noise band and the comparison passed |=== === ERROR Level (200-299) @@ -116,4 +118,8 @@ section lists only the k6-specific records, whose upstream counterparts are wrk- |K6Benchmark-205 |BENCHMARK |Incomplete k6 summary in file %s (missing field '%s') |Logged when a summary omits a mandatory field; that file yields no benchmark rather than a partial one |K6Benchmark-206 |BENCHMARK |Usage: ComparisonSummaryWriter |Logged when the comparison writer is invoked with the wrong argument count; the process then exits non-zero |K6Benchmark-207 |BENCHMARK |No results directory for target '%s' under %s |Logged when a target produced no results directory; that target's column renders `n/a` rather than being dropped +|K6Benchmark-208 |BENCHMARK |Usage: PassthroughBaselineComparator |Logged when the passthrough baseline comparator is invoked without the results directory; the process then exits non-zero +|K6Benchmark-209 |BENCHMARK |Missing k6 summary '%s' under %s — cannot run the passthrough baseline comparison |Logged when either summary the comparison requires is absent; the comparison is refused rather than run against one side +|K6Benchmark-210 |BENCHMARK |Passthrough empty-mode regressed beyond the %s noise band vs the PLAN-04 baseline: %s |Logged when the empty-mode run's throughput dropped, or a latency percentile rose, beyond the noise band; the process then exits non-zero +|K6Benchmark-211 |BENCHMARK |Passthrough empty-mode and baseline windows disagree beyond the %s window-comparability band, so their rates are not comparable: %s |Logged when the two arms were measured over different-length windows, which makes their `requests_per_second` rates incomparable; the comparison is refused and the run fails — the pair is never rescaled to a common window and never downgraded to a warning |=== diff --git a/doc/development/declared-limit-assertion-coverage.adoc b/doc/development/declared-limit-assertion-coverage.adoc index 652bcdd1..b052ca91 100644 --- a/doc/development/declared-limit-assertion-coverage.adoc +++ b/doc/development/declared-limit-assertion-coverage.adoc @@ -19,7 +19,7 @@ neither. == The Enumerated Surface -The declared surface is *twelve files* under `integration-tests/src/main/docker/`. +The declared surface is *thirteen files* under `integration-tests/src/main/docker/`. The table below is not decoration. `DescriptorInventoryWiringTest` parses the marked block at runtime and takes every monospaced path spelled inside it -- glob patterns excluded -- as its @@ -32,9 +32,10 @@ disk without touching this table and the guard fails. | Surface | Count | Files | `sheriff-config*/gateway.yaml` -| 4 +| 5 | `sheriff-config/gateway.yaml` (base), `sheriff-config-cookie/gateway.yaml`, - `sheriff-config-mtls/gateway.yaml`, `sheriff-config-ws-admission/gateway.yaml` + `sheriff-config-mtls/gateway.yaml`, `sheriff-config-passthrough-empty/gateway.yaml`, + `sheriff-config-ws-admission/gateway.yaml` | `sheriff-config/endpoints/*.yaml` | 7 @@ -49,9 +50,9 @@ disk without touching this table and the guard fails. |=== // end::inventory[] -Only the base directory carries `endpoints/` and `topology.properties`; the three overlay +Only the base directory carries `endpoints/` and `topology.properties`; the four overlay directories replace `gateway.yaml` and nothing else, which is why the endpoints tree resolves -identically for all six booted gateway instances. +identically for all seven booted gateway instances. *No limit-shaped key lives outside this surface.* The sweep is a key-name pattern, applied to declared key names only -- never to raw file text, so the descriptors' extensive prose comments @@ -61,12 +62,13 @@ cannot manufacture a hit: `max|min|limit|cap|_bytes|_seconds|timeout|ttl|leeway|length|allow|deny|threshold|budget|quota|window|depth` // end::limit-key-regex[] -Over all twelve files it returns hits in exactly nine of them: +Over all thirteen files it returns hits in exactly ten of them: // tag::limit-bearing[] * `sheriff-config/gateway.yaml` * `sheriff-config-cookie/gateway.yaml` * `sheriff-config-mtls/gateway.yaml` +* `sheriff-config-passthrough-empty/gateway.yaml` * `sheriff-config-ws-admission/gateway.yaml` * `sheriff-config/endpoints/bff-session.yaml` * `sheriff-config/endpoints/grpc.yaml` @@ -77,31 +79,54 @@ Over all twelve files it returns hits in exactly nine of them: The remaining three -- `sheriff-config/endpoints/assets.yaml`, `sheriff-config/endpoints/assets-secure.yaml` and `sheriff-config/topology.properties` -- declare -none. That complement is the other half of the same claim, so asserting the nine exactly asserts +none. That complement is the other half of the same claim, so asserting the ten exactly asserts both halves at once. *Both claims above are machine-checked against this note itself*, not merely asserted here in prose. `DescriptorInventoryWiringTest` (in `integration-tests/src/test/java/.../gateway/integration/`) reads the three marked blocks above at -runtime -- the inventory table, the sweep pattern and the nine limit-bearing paths -- and uses them +runtime -- the inventory table, the sweep pattern and the ten limit-bearing paths -- and uses them as its expected values. It hardcodes none of them, so editing the test's expectations without editing this note is not possible: the note *is* the expectation. It then asserts the committed inventory equals the parsed inventory as an *exact set* -- so an added descriptor fails as loudly as a removed one, unlike the *at-least* floor the sibling wiring guards use -- and re-runs the parsed sweep over that inventory, asserting the set of limit-declaring files equals the parsed -nine. If this note cannot be located, or a marked block is missing or empty, the guard fails loudly +ten. If this note cannot be located, or a marked block is missing or empty, the guard fails loudly rather than deriving an empty expectation and passing. === Citation notation Paths below are relative to `integration-tests/src/main/docker/`. Because a key declared in all -four `gateway.yaml` documents sits at a different line in each, such rows cite every line -explicitly as `×4 -- base:N, cookie:N, mtls:N, ws:N`, where `ws` is `sheriff-config-ws-admission`. -A row citing a bare `sheriff-config/gateway.yaml:N` is declared in the *base document only* and is -inherited by the overlays as the omitted-key default. - -The `security_defaults` block in particular is *base-only*: the cookie, mTLS and ws-admission -overlays declare no `security_defaults` at all and resolve the shipped defaults. +four *independently authored* `gateway.yaml` documents sits at a different line in each, such rows +cite every line explicitly as `×4 -- base:N, cookie:N, mtls:N, ws:N`, where `ws` is +`sheriff-config-ws-admission`. A row citing a bare `sheriff-config/gateway.yaml:N` is declared in +the *base document only* and is inherited by the overlays as the omitted-key default. + +`sheriff-config-passthrough-empty/gateway.yaml` is deliberately *outside* that `×N` notation, and +the `×4` rows are **not** re-cited as `×5`. It is not an independently authored descriptor: it is a +verbatim copy of the base document with the `tls.passthrough_sni` block -- and only that block -- +removed, so every key the base declares it also declares, at the base's own relative order. + +*Fourteen* Coverage Matrix rows cite the base document: the *seven* `×4` rows (`anchors.upload`, +`csrf.trusted_origins`, `refresh.leeway_seconds`, G2, G8, G9, G10) and the *seven* base-only +`sheriff-config/gateway.yaml:N` rows (`security_defaults.profile`, +`allow_get_with_content_length_body`, `allowed_methods`, `tls.passthrough_sni`, +`asset_defaults.content_types`, G1, G11). Citing this overlay per row would add a *fifth* line number +to each of the seven `×4` rows and a *second* to six of the seven base-only rows -- *thirteen* rows, +restating one fact thirteen times where it is better stated once: *derive it from the base citation.* +The two remaining `gateway.yaml` rows in the matrix cite an overlay only +(`sheriff-config-mtls/gateway.yaml`, `sheriff-config-ws-admission/gateway.yaml`) and declare keys the +base does not, so they are outside this derivation entirely. + +The thirteenth-versus-fourteenth row is not an off-by-one: the base-only row that would gain *no* +citation is `tls.passthrough_sni` itself, the one key this overlay does **not** carry -- and that +absence is precisely the property the instance exists to provide (see the descriptor's own header +comment). It is the single consequence of the derivation. + +The `security_defaults` block in particular is *base-only among the independently authored +overlays*: the cookie, mTLS and ws-admission overlays declare no `security_defaults` at all and +resolve the shipped defaults. The passthrough-empty overlay is the exception that follows directly +from its derivation -- being a base copy, it carries the base's `security_defaults` verbatim. == The Two Declaration Layers @@ -431,18 +456,18 @@ enlarge exactly the hole this note has chosen to report. == Report-Only Finding: the inventory guard partitions files, not keys `DescriptorInventoryWiringTest#limitShapedKeysLiveOnlyInTheRecordedSurfaces` builds a set of *file -paths* -- those for which `declaresLimitShapedKey` returns `true` -- and asserts it equals the nine +paths* -- those for which `declaresLimitShapedKey` returns `true` -- and asserts it equals the ten paths parsed from the `limit-bearing` block. `declaresLimitShapedKey` is a per-file *boolean* predicate ("does this file declare at least one limit-shaped key"), not a key enumeration, so the assertion's granularity is the file and never the key. -The consequence: adding a limit-shaped key to one of the *nine already limit-bearing* files leaves +The consequence: adding a limit-shaped key to one of the *ten already limit-bearing* files leaves the computed set identical to the parsed one. The build stays green and no Coverage Matrix row is forced. Only a key that makes one of the remaining three -- `sheriff-config/endpoints/assets.yaml`, `sheriff-config/endpoints/assets-secure.yaml` or `sheriff-config/topology.properties` -- limit-bearing crosses the partition and fails the build. -Nine of twelve is the *majority* of the surface, so this is the ordinary case rather than an edge -one: for those nine files the "add its row here in the same change" instruction that closes this +Ten of thirteen is the *majority* of the surface, so this is the ordinary case rather than an edge +one: for those ten files the "add its row here in the same change" instruction that closes this note rests on contributor convention, not on the guard. This is *reported, not fixed*, for the same reason as the finding above. Closing it means deriving @@ -467,14 +492,14 @@ This table is prose, and prose drifts the moment a descriptor changes. Three pro asserts the value appears in *no* committed descriptor, which is precisely why such a limit is the one most likely to lose its assertion unnoticed. A row carrying neither a citation nor that marker is the drift this property exists to catch. -. *The enumerated surface is stated explicitly* (twelve files, named) *and machine-checked against +. *The enumerated surface is stated explicitly* (thirteen files, named) *and machine-checked against this note*. `DescriptorInventoryWiringTest` parses the marked blocks in "The Enumerated Surface" - -- the inventory table, the sweep pattern and the nine limit-bearing paths -- and uses them as its + -- the inventory table, the sweep pattern and the ten limit-bearing paths -- and uses them as its expected values instead of carrying its own copy. Both of its assertions are *file-level set equalities*, and the guarantee is exactly that: a new descriptor directory or a new `endpoints/*.yaml` breaks the exact-inventory equality and fails the build until this note is updated, and a limit-shaped key fails the build *only when it makes a previously - non-limit-bearing file limit-bearing* -- when it moves a file across the nine/three partition. + non-limit-bearing file limit-bearing* -- when it moves a file across the ten/three partition. Adding one to a file that already declares a limit changes neither set; see the report-only finding above. Updating the test alone cannot make the build green: the test has no expectation of its own to update. @@ -483,5 +508,5 @@ This table is prose, and prose drifts the moment a descriptor changes. Three pro vacuous or mis-rooted glob fails loudly instead of passing over an empty set. This note indexes those guards; it does not replace them. -When you add a limit-shaped key to any of the twelve files, add its row here in the same change -- +When you add a limit-shaped key to any of the thirteen files, add its row here in the same change -- and give it a status that is honest, including `GAP`. diff --git a/doc/development/integration-test-topology.adoc b/doc/development/integration-test-topology.adoc index 7f84093a..08f86ee0 100644 --- a/doc/development/integration-test-topology.adoc +++ b/doc/development/integration-test-topology.adoc @@ -11,7 +11,7 @@ reverse-engineering `integration-tests/docker-compose.yml`. [IMPORTANT] ==== This is the *integration-test* topology. It is not a production deployment and not a recommended -one. Toxiproxy, go-httpbin, grpc-echo, the passthrough backend and the five variant gateway +one. Toxiproxy, go-httpbin, grpc-echo, the passthrough backend and the six variant gateway instances exist to make specific tests possible. Production and Kubernetes topologies are defined by PLAN-27 and are deliberately not drawn here or anywhere else yet. ==== @@ -57,10 +57,11 @@ link:tls-edge.adoc[TLS Edge -- Front Listener and the Accept-Time SNI Split]. === The collapsed variant group -Five further gateway instances run the *same* `api-sheriff:distroless` image and differ only by an -overlaid `gateway.yaml` -- or, for the last one, by a single environment variable -- and their -published ports. Drawing all six in full would bury the affordances the diagram exists to show, so -they are collapsed into one annotated group that still names every instance and every port pair: +Six further gateway instances run the *same* `api-sheriff:distroless` image and differ only by an +overlaid `gateway.yaml` -- or, for `api-sheriff-plain-mgmt`, by a single environment variable -- and +their published ports. Drawing all seven in full would bury the affordances the diagram exists to +show, so they are collapsed into one annotated group that still names every instance and every port +pair: [cols="2,1,1,3"] |=== @@ -96,6 +97,16 @@ they are collapsed into one annotated group that still names every instance and | The one instance whose *management* interface serves plain HTTP, proving the supported downgrade path. It overlays no `gateway.yaml` at all -- the opt-out is a single environment variable -- so the management scheme is the only variable under test. See <>. + +| `api-sheriff-passthrough-empty` +| 10449 +| 19006 +| `tls.passthrough_sni` is process-wide, and the primary instance declares it non-empty for its whole + lifetime -- so the benchmark's `passthroughRelayEmpty` arm, whose entire claim is that it measures + the *zero-overhead default where no front listener is created*, cannot be run against the primary. + This instance overlays a `gateway.yaml` that is the base document minus that one block, and is the + only instance with no front listener at all: its terminated Quarkus HTTPS listener owns `8443` + directly, so it sets no `QUARKUS_HTTP_SSL_PORT` relocation. |=== === Mounted material @@ -116,7 +127,7 @@ bring-up script waits for, what that wait actually proves, and where its retry b `integration-tests/scripts/start-integration-container.sh` waits on `/q/health/ready` for every discovered gateway instance, in the single Compose-model-derived wait loop. It is one loop for all -six instances: the service set, each instance's published management port, and the scheme its +seven instances: the service set, each instance's published management port, and the scheme its management interface speaks are read from the resolved Compose model, so adding, removing or renumbering an instance needs no edit to the script. @@ -165,8 +176,9 @@ literals that can drift apart. Its value was measured, not chosen. A sub-second prober (100 ms polling, started *before* `docker compose up -d` so that it observes the live-to-ready transition rather than inferring it -from a loop that only begins once both are already true) ran against all six gateway instances -brought up concurrently with Keycloak, under CPU contention -- 24 busy workers on 16 cores: +from a loop that only begins once both are already true) ran against the six gateway instances that +existed when it was taken, brought up concurrently with Keycloak, under CPU contention -- 24 busy +workers on 16 cores: [cols="2,1,1,1"] |=== @@ -180,6 +192,11 @@ brought up concurrently with Keycloak, under CPU contention -- 24 busy workers o | `api-sheriff-ws-admission`| 8.95s | 8.95s | 0.00s |=== +This table is evidence from a dated run, not a claim about today's instance set, so it is +deliberately *not* restated as the set grows: `api-sheriff-passthrough-empty` -- the seventh +instance -- is absent from it by design rather than by omission, and no row may be invented for it. +The headroom argument below therefore stands on the six instances actually measured. + The live-to-ready delta is *0.00s on every instance* -- below the prober's own 100 ms resolution. That is not luck; it is what the previous section describes. Because the `jwks` datum is settled at boot and cached, readiness flips at the same moment liveness does. Switching the gate from liveness @@ -213,7 +230,7 @@ to fix.* It is handled structurally rather than as a special case: each service declares the scheme its management interface speaks in a `de.cuioss.sheriff.management-scheme` label, and the wait loop adds `-k` only when that label says `https`. The scheme difference is therefore data, not a branch on a -service name -- which is what lets one loop cover all six instances. +service name -- which is what lets one loop cover all seven instances. The corollary is a standing assertion: this instance must be probed over `http://` with *no* `-k`. If it ever needs `-k`, the plain-HTTP opt-out has silently stopped working, and that is the bug -- @@ -239,11 +256,19 @@ path the wait loop probes, hoisted the retry budget into a named constant, and d probe URL from the Compose model. None of those is a fact the diagram depicts, so the diagram is not made stale by it and is deliberately not redrawn. -The check did, however, surface a *pre-existing* gap that predates this change: the collapsed -variant group in the SVG is labelled `variant instances (4)` and names only `api-sheriff-mtls`, -`api-sheriff-cookie`, `api-sheriff-cookie-2` and `api-sheriff-ws-admission`. It omits -`api-sheriff-plain-mgmt` -- the same omission the table above previously carried and now fixes. The -diagram's `` accessibility text repeats the omission. Closing it means adding the fifth member -plus its `10448 · 19005` port pair, correcting the count label and the ``, and re-rendering -against both themes; that is diagram work rather than documentation work and is deliberately left to -a follow-up rather than folded in here. +The check did, however, surface a *pre-existing* gap that predated this change: the collapsed variant +group in the SVG was labelled `variant instances (4)` and named only `api-sheriff-mtls`, +`api-sheriff-cookie`, `api-sheriff-cookie-2` and `api-sheriff-ws-admission`, omitting +`api-sheriff-plain-mgmt`; the diagram's `` accessibility text repeated the omission. + +*That gap is now closed, not deferred.* The group is labelled `variant instances (6)` and enumerates +all six members with their port pairs -- `api-sheriff-plain-mgmt` at `10448 · 19005` and +`api-sheriff-passthrough-empty` at `10449 · 19006` joining the original four -- and the `` +accessibility narrative carries both the count and the full member enumeration rather than a subset. +The enclosing group rectangle and the own-bar line extent were grown by two row heights to fit the +added rows. The follow-up this section previously deferred therefore no longer exists. + +The read-back obligation is unchanged and is *not* discharged by this note: re-render and read the +diagram back against both themes before committing any further edit to it, per the +link:diagram-type-deployment.md[deployment diagram-type standard]. Rendered PNGs remain verification +artifacts and are never committed. diff --git a/doc/resources/diagrams/integration-test-topology.svg b/doc/resources/diagrams/integration-test-topology.svg index ea9b73db..af6b3a21 100644 --- a/doc/resources/diagrams/integration-test-topology.svg +++ b/doc/resources/diagrams/integration-test-topology.svg @@ -11,7 +11,7 @@ role="img" aria-labelledby="title desc" font-family="ui-sans-serif, -apple-system, system-ui, 'Segoe UI', sans-serif"> API Sheriff integration-test topology, the Docker Compose stack used by the integration-tests module - The integration-test deployment topology, not a production deployment. A host runs a single Docker Compose bridge network named api-sheriff. Crossing from the host into the network are the published ports: the primary api-sheriff gateway on 10443 mapped to container 8443, and Keycloak on 1443 mapped to container 8443. Inside the gateway container a Vert.x SNI front listener owns the public port 8443; a matched passthrough SNI is relayed opaquely at layer 4 to the passthrough-backend and is never terminated, while every other connection is handed to the internal terminating Quarkus HTTPS listener on 8444. The management interface serves HTTPS only, on 9000. Three read-only volumes are mounted: certificates, sheriff-config and assets. The four variant gateway instances are collapsed into one annotated group; their members are api-sheriff-mtls on 10444 and 19001, api-sheriff-cookie on 10445 and 19002, api-sheriff-cookie-2 on 10446 and 19003, and api-sheriff-ws-admission on 10447 and 19004, each the same api-sheriff:distroless image with an overlaid gateway.yaml. Gateway egress crosses a second trust boundary governed by a host-exact SSRF allowlist and a named TLS trust profile, reaching Keycloak on 8443, the go-httpbin echo upstream on 8080, the asset-origin static origin on 80, and the in-repo grpc-echo upstream on 9000. Toxiproxy fronts the passthrough backend on its published admin port 8474 for mid-stream fault injection. Prometheus is published on 9090 and scrapes the management ports; k6 runs only under the benchmark compose profile. The management ports of the primary gateway, 19000, and of each variant instance are published but are not drawn as separate arrows; they are recorded in the boxes. + The integration-test deployment topology, not a production deployment. A host runs a single Docker Compose bridge network named api-sheriff. Crossing from the host into the network are the published ports: the primary api-sheriff gateway on 10443 mapped to container 8443, and Keycloak on 1443 mapped to container 8443. Inside the gateway container a Vert.x SNI front listener owns the public port 8443; a matched passthrough SNI is relayed opaquely at layer 4 to the passthrough-backend and is never terminated, while every other connection is handed to the internal terminating Quarkus HTTPS listener on 8444. The management interface serves HTTPS only, on 9000. Three read-only volumes are mounted: certificates, sheriff-config and assets. The six variant gateway instances are collapsed into one annotated group; their members are api-sheriff-mtls on 10444 and 19001, api-sheriff-cookie on 10445 and 19002, api-sheriff-cookie-2 on 10446 and 19003, api-sheriff-ws-admission on 10447 and 19004, api-sheriff-plain-mgmt on 10448 and 19005, and api-sheriff-passthrough-empty on 10449 and 19006. All six run the same api-sheriff:distroless image. Four of them differ by an overlaid gateway.yaml; api-sheriff-plain-mgmt overlays none and differs only by a single environment variable that takes its management interface to plain HTTP, and api-sheriff-passthrough-empty overlays a gateway.yaml that is the base document with the tls.passthrough_sni block removed, so it is the one instance that creates no SNI front listener and whose terminated Quarkus HTTPS listener owns the public port 8443 directly. Gateway egress crosses a second trust boundary governed by a host-exact SSRF allowlist and a named TLS trust profile, reaching Keycloak on 8443, the go-httpbin echo upstream on 8080, the asset-origin static origin on 80, and the in-repo grpc-echo upstream on 9000. Toxiproxy fronts the passthrough backend on its published admin port 8474 for mid-stream fault injection. Prometheus is published on 9090 and scrapes the management ports; k6 runs only under the benchmark compose profile. The management ports of the primary gateway, 19000, and of each variant instance are published but are not drawn as separate arrows; they are recorded in the boxes. assets/ - - - variant instances (4) - same image, overlaid gateway.yaml, own published ports + + + variant instances (6) + same image, per-variant overlay, own published ports api-sheriff-mtls api-sheriff-cookie api-sheriff-cookie-2 api-sheriff-ws-admission - 10444 · 19001 - 10445 · 19002 - 10446 · 19003 - 10447 · 19004 + api-sheriff-plain-mgmt + api-sheriff-passthrough-empty + 10444 · 19001 + 10445 · 19002 + 10446 · 19003 + 10447 · 19004 + 10448 · 19005 + 10449 · 19006 diff --git a/integration-tests/docker-compose.benchmark.yml b/integration-tests/docker-compose.benchmark.yml index 1759ea88..ab252403 100644 --- a/integration-tests/docker-compose.benchmark.yml +++ b/integration-tests/docker-compose.benchmark.yml @@ -38,3 +38,14 @@ services: - TOPOLOGY_UPSTREAM=http://nginx-static:8080 depends_on: - nginx-static + + # The empty-passthrough_sni arm is compared against the primary instance's proxiedStatic baseline, + # so it must be repointed at the SAME static backend. This is the ADR-0012 "identical upstreams" + # parity precondition: leaving this instance on go-httpbin while the primary serves from + # nginx-static would fold the JSON-serializing backend's cost into one side of the comparison only, + # and the resulting difference would be read as passthrough overhead. + api-sheriff-passthrough-empty: + environment: + - TOPOLOGY_UPSTREAM=http://nginx-static:8080 + depends_on: + - nginx-static diff --git a/integration-tests/docker-compose.yml b/integration-tests/docker-compose.yml index 62f9dccc..1357a869 100644 --- a/integration-tests/docker-compose.yml +++ b/integration-tests/docker-compose.yml @@ -217,7 +217,7 @@ services: # only the logical profile (jwks.tls_profile: benchmark-idp); this mounted file binds it to a # concrete store, so the shipped artifact carries no benchmark-idp key and no trust password in # any profile. Loaded from the already-mounted certificates/ volume — no new volume is added. - # Every QUARKUS_PROFILE=it gateway instance sets this; the value is identical across all six. + # Every QUARKUS_PROFILE=it gateway instance sets this; the value is identical across all seven. - QUARKUS_CONFIG_LOCATIONS=/app/certificates/benchmark-idp-trust.properties # File logging to mounted target directory. The shipped artifact ships file logging OFF, so a # deployment that wants a log file switches it on here — the enable flag and the path are one @@ -249,7 +249,7 @@ services: # insecure-requests — so supplying these two keys converts port 9000 itself to HTTPS. Every # consumer of 9000 (host readiness probes, Prometheus, the k6 health benchmarks) must speak # TLS with -k / insecure_skip_verify, because the certificate is the self-signed localhost - # bundle. Five of the six gateway instances carry this pair; api-sheriff-plain-mgmt is the one + # bundle. Six of the seven gateway instances carry this pair; api-sheriff-plain-mgmt is the one # deliberate exception, documented at its own service definition. - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key @@ -380,7 +380,7 @@ services: # Management-interface certificate, kept in lockstep with the primary api-sheriff service — # see that service's comment for the single-port mechanism. Without it this instance's # management port 19001 stays plain HTTP while the primary's is HTTPS, and the host-side - # readiness wait in start-integration-container.sh (https + -k for 19000-19004) would hang. + # readiness wait in start-integration-container.sh (https + -k for 19000-19004 and 19006) would hang. # api-sheriff-plain-mgmt on 19005 is the one deliberate exception to that lockstep and is # gated by its own http:// block. - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt @@ -476,7 +476,7 @@ services: # Management-interface certificate, kept in lockstep with the primary api-sheriff service — # see that service's comment for the single-port mechanism. Without it this instance's # management port 19002 stays plain HTTP while the primary's is HTTPS, and the host-side - # readiness wait in start-integration-container.sh (https + -k for 19000-19004) would hang. + # readiness wait in start-integration-container.sh (https + -k for 19000-19004 and 19006) would hang. # api-sheriff-plain-mgmt on 19005 is the one deliberate exception to that lockstep. - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key @@ -578,7 +578,7 @@ services: # Management-interface certificate, kept in lockstep with every other instance — see the # primary api-sheriff service's comment for the single-port mechanism. Without it this # instance's management port 19003 stays plain HTTP while the others are HTTPS, and the - # host-side readiness wait in start-integration-container.sh (https + -k for 19000-19004) + # host-side readiness wait in start-integration-container.sh (https + -k for 19000-19004 and 19006) # would hang. api-sheriff-plain-mgmt on 19005 is the one deliberate exception. - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key @@ -663,7 +663,7 @@ services: # Management-interface certificate, kept in lockstep with every other instance — see the # primary api-sheriff service's comment for the single-port mechanism. Without it this # instance's management port 19004 stays plain HTTP while the others are HTTPS, and the - # host-side readiness wait in start-integration-container.sh (https + -k for 19000-19004) + # host-side readiness wait in start-integration-container.sh (https + -k for 19000-19004 and 19006) # would hang. api-sheriff-plain-mgmt on 19005 is the one deliberate exception. - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key @@ -763,7 +763,7 @@ services: # passthrough_sni — so the SNI front listener claims the public port 8443 exactly as on the # primary api-sheriff service, and the terminated HTTPS listener MUST be moved to the internal # port or the two collide and boot fails with "Port 8443 seems to be in use by another - # process". The four other gateway instances escape this only because each overlays + # process". The five other gateway instances escape this only because each overlays # gateway.yaml with a variant declaring no passthrough_sni; this one deliberately does not # overlay, so that the management opt-out is the single variable under test. Keep in lockstep # with the primary service's identical setting — the invariant is documented at the @@ -820,6 +820,106 @@ services: - api-sheriff restart: unless-stopped + # --- The genuinely empty-passthrough_sni instance backing the benchmark's empty-mode arm -------- + # `passthrough_sni` is a property of the whole gateway process: declaring it non-empty is what + # STARTS the accept-time Vert.x SNI front listener on the public TLS port, and the primary instance + # declares two entries for its whole lifetime. So the `passthroughRelayEmpty` benchmark arm — whose + # entire claim is that it measures D1's zero-overhead default, where that front listener is NEVER + # created — could not be run against the primary instance without measuring the exact opposite of + # what it names. Before this instance existed it was pointed at the primary anyway, so both arms of + # the no-regression gate issued the identical request to the identical listener stack on ONE + # gateway: the gate was comparing a configuration against itself and could neither pass nor fail + # for the reason it existed. + # + # This instance reuses the SAME native image and overlays a gateway.yaml that is byte-identical to + # the shared one except for the removed `tls.passthrough_sni` block; published on 10449. It is + # separate rather than a mode of the primary because the two configurations must run CONCURRENTLY + # for the benchmark lane to measure both arms in one run. + # + # It sets NO QUARKUS_HTTP_SSL_PORT, and that absence is load-bearing rather than an omission: with + # no passthrough_sni there is no front listener to claim 8443, so the terminated Quarkus HTTPS + # listener keeps the public port directly. Setting the loopback relocation here would move the + # terminated listener off 8443 with nothing left listening on it. + api-sheriff-passthrough-empty: + image: "api-sheriff:distroless" + # Same JVM-default-truststore trust as every other instance — see the primary api-sheriff + # service's command comment for the mechanism (token-sheriff-client#597). + command: + - -Djavax.net.ssl.trustStore=/app/certificates/localhost-truststore.p12 + - -Djavax.net.ssl.trustStorePassword=localhost-trust + - -Djavax.net.ssl.trustStoreType=PKCS12 + # Management-interface scheme for host-side readiness discovery (see the api-sheriff service). + labels: + de.cuioss.sheriff.management-scheme: "https" + + ports: + - "10449:8443" # External test port for the empty-passthrough_sni benchmark arm + - "19006:9000" # Management interface (health/metrics, HTTPS — single port, see below) + environment: + - QUARKUS_PROFILE=it + # Deployment-supplied benchmark-idp trust bucket, in lockstep with the primary api-sheriff + # service — see that service's comment for the mechanism. + - QUARKUS_CONFIG_LOCATIONS=/app/certificates/benchmark-idp-trust.properties + - QUARKUS_LOG_FILE_ENABLE=true + - LOG_FILE_PATH=/logs/quarkus-passthrough-empty.log + # NO QUARKUS_HTTP_SSL_PORT here — see the service comment. The overlaid gateway.yaml declares no + # passthrough_sni, so no front listener is created and the terminated listener owns 8443. + - QUARKUS_HTTP_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt + - QUARKUS_HTTP_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key + # Management-interface certificate, kept in lockstep with every other instance — see the + # primary api-sheriff service's comment for the single-port mechanism. Without it this + # instance's management port 19006 stays plain HTTP while the others are HTTPS, and the + # host-side readiness wait in start-integration-container.sh (https + -k for 19000-19004 and + # 19006) would hang. api-sheriff-plain-mgmt on 19005 is the one deliberate exception. + - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt + - QUARKUS_MANAGEMENT_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key + - QUARKUS_TLS_DEFAULT_TRUST__STORE_P12_PATH=/app/certificates/localhost-truststore.p12 + - QUARKUS_TLS_DEFAULT_TRUST__STORE_P12_PASSWORD=localhost-trust + - SHERIFF_CONFIG_DIR=/app/sheriff-config + # Container-side value the bare ${OIDC_CLIENT_SECRET} reference in the overlaid gateway.yaml + # (oidc.client_secret) resolves to via EnvSecretResolver — kept in lockstep with the primary + # api-sheriff service, whose gateway.yaml carries the same oidc block. + - OIDC_CLIENT_SECRET=integration-secret + depends_on: + keycloak: + condition: service_started + go-httpbin: + condition: service_started + asset-origin: + condition: service_started + grpc-echo: + condition: service_healthy + volumes: + - ./src/main/docker/certificates:/app/certificates:ro + # Shared config dir, then overlay ONLY gateway.yaml with the no-passthrough_sni variant. + - ./src/main/docker/sheriff-config:/app/sheriff-config:ro + - ./src/main/docker/sheriff-config-passthrough-empty/gateway.yaml:/app/sheriff-config/gateway.yaml:ro + - ./src/main/docker/assets:/app/assets:ro + - ${LOG_TARGET_DIR:-./target/quarkus-logs}:/logs:rw + security_opt: + - no-new-privileges:true + cap_drop: + - ALL + read_only: true + tmpfs: + - /tmp:rw,noexec,nosuid,size=100m + # Identical to the primary instance's budget, per ADR-0012: the empty-mode arm is compared + # against the primary's proxiedStatic baseline, so a different CPU or memory limit here would + # make the comparison measure the budget difference rather than the passthrough configuration. + deploy: + resources: + limits: + memory: 512M + cpus: '4.0' + reservations: + memory: 256M + cpus: '1.0' + # No in-container healthcheck: the distroless image ships neither a shell nor curl. Readiness is + # gated host-side by start-integration-container.sh probing the published management port 19006. + networks: + - api-sheriff + restart: unless-stopped + prometheus: image: prom/prometheus:v3.6.0 ports: diff --git a/integration-tests/scripts/dump-keycloak-logs.sh b/integration-tests/scripts/dump-keycloak-logs.sh index 5028be7e..bf18bc19 100755 --- a/integration-tests/scripts/dump-keycloak-logs.sh +++ b/integration-tests/scripts/dump-keycloak-logs.sh @@ -10,7 +10,18 @@ set -euo pipefail # Configuration -KEYCLOAK_CONTAINER_NAME="integration-tests-keycloak-1" +# +# Compose names every container "--", and the project name is +# COMPOSE_PROJECT_NAME when set, otherwise the basename of the Compose project directory — the same +# resolution start-integration-container.sh relies on when it brings the stack up from PROJECT_DIR. +# Deriving the prefix here rather than hardcoding "integration-tests-" is what keeps this script +# working when a run sets COMPOSE_PROJECT_NAME; the hardcoded form silently found no container and +# dumped nothing at all. +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +PROJECT_DIR="$(dirname "$SCRIPT_DIR")" +COMPOSE_PROJECT="${COMPOSE_PROJECT_NAME:-$(basename "$PROJECT_DIR")}" + +KEYCLOAK_CONTAINER_NAME="${COMPOSE_PROJECT}-keycloak-1" TIMESTAMP=$(date +"%Y-%m-%d_%H-%M-%S") KEYCLOAK_LOG_FILENAME="keycloak-logs-${TIMESTAMP}.txt" @@ -42,25 +53,38 @@ echo "📝 Output file: $KEYCLOAK_LOG_FILE_PATH" # artifact) so a TEST failure — not just a startup failure — is diagnosable from the app's stdout. # Never fail the build on a dump problem. # -# The list MUST name every gateway instance docker-compose.yml starts, not just the primary and the -# mTLS peer: the Bff*Cookie*IT suites drive the two dedicated cookie-mode instances and -# WebSocketProxyIT's relay-exhaustion regression drives the low-admission-budget instance, and a -# CI-only failure on those instances previously produced NO uploaded log at all, forcing a local -# repro to see the gateway's own rejection reason. An admission refusal in particular is a bare 503 -# on the wire whose reason exists only in the gateway's own log. Keep this list in lockstep with the -# api-sheriff* services in integration-tests/docker-compose.yml. +# EVERY gateway instance must be dumped, not just the primary and the mTLS peer: the Bff*Cookie*IT +# suites drive the two dedicated cookie-mode instances, WebSocketProxyIT's relay-exhaustion +# regression drives the low-admission-budget instance, and a CI-only failure on those instances +# previously produced NO uploaded log at all, forcing a local repro to see the gateway's own +# rejection reason. An admission refusal in particular is a bare 503 on the wire whose reason exists +# only in the gateway's own log. +# +# The set is DERIVED from what is actually running rather than restated here. It used to be a +# hardcoded list of seven container names under a comment instructing the reader to keep it in +# lockstep with the api-sheriff* services in docker-compose.yml — and it had already drifted: the +# plain-mgmt instance was missing from it until PLAN-46 despite having shipped earlier, so its +# boot-time downgrade WARN (ApiSheriff-115) was silently undumped. A hardcoded list that must mirror +# a set defined elsewhere is a defect unless it is derived from that source, exactly as +# start-integration-container.sh derives its readiness targets from the resolved Compose model, so +# adding, removing or renaming an api-sheriff* service now needs no edit here. +# +# The match is anchored to THIS project's own container prefix, so a foreign stack's api-sheriff +# containers (a compose-sample or demo-client environment running on the same host) can never be +# swept in, and no non-gateway service of this project can either — no other service name in +# docker-compose.yml begins with "api-sheriff". FAILSAFE_DIR="${TARGET_ABS_PATH}/failsafe-reports" mkdir -p "$FAILSAFE_DIR" || true -for app in integration-tests-api-sheriff-1 \ - integration-tests-api-sheriff-mtls-1 \ - integration-tests-api-sheriff-cookie-1 \ - integration-tests-api-sheriff-cookie-2-1 \ - integration-tests-api-sheriff-ws-admission-1; do - if docker ps -a --format "{{.Names}}" | grep -q "^${app}$"; then - echo "📥 Dumping app logs: ${app} -> ${FAILSAFE_DIR}/${app}.log" - docker logs "$app" > "${FAILSAFE_DIR}/${app}.log" 2>&1 || true - fi -done +APP_CONTAINERS=$(docker ps -a --format "{{.Names}}" \ + | grep -E "^${COMPOSE_PROJECT}-api-sheriff(-[a-z0-9-]+)?-[0-9]+$" || true) +if [ -z "$APP_CONTAINERS" ]; then + echo "⚠️ No ${COMPOSE_PROJECT}-api-sheriff* containers found — no app logs to dump" +fi +while read -r app; do + [ -z "$app" ] && continue + echo "📥 Dumping app logs: ${app} -> ${FAILSAFE_DIR}/${app}.log" + docker logs "$app" > "${FAILSAFE_DIR}/${app}.log" 2>&1 || true +done <<< "$APP_CONTAINERS" # Check if container exists and is running if ! docker ps --format "{{.Names}}" | grep -q "^${KEYCLOAK_CONTAINER_NAME}$"; then diff --git a/integration-tests/src/main/docker/sheriff-config-passthrough-empty/gateway.yaml b/integration-tests/src/main/docker/sheriff-config-passthrough-empty/gateway.yaml new file mode 100644 index 00000000..7ed48972 --- /dev/null +++ b/integration-tests/src/main/docker/sheriff-config-passthrough-empty/gateway.yaml @@ -0,0 +1,317 @@ +# yaml-language-server: $schema=../../../../../api-sheriff/src/main/resources/schema/gateway.schema.json +# +# =========================================================================================== +# THE ABSENCE OF `tls.passthrough_sni` IS THE FEATURE UNDER MEASUREMENT. +# =========================================================================================== +# +# This document is a copy of ../sheriff-config/gateway.yaml with the `tls.passthrough_sni` +# block — and ONLY that block — removed. Everything else is byte-identical on purpose: +# security_defaults, the tls min_version / cipher_suites / alpn policy, all ten anchors, +# the token_validation issuers and the oidc block. A second difference would make the two +# arms differ in more than the one variable the benchmark is trying to isolate. +# +# WHY THE VARIANT EXISTS. Declaring a non-empty `passthrough_sni` is what STARTS the +# accept-time Vert.x SNI front listener on the public TLS port. The `passthroughRelayEmpty` +# benchmark arm claims to measure D1's zero-overhead default — the configuration in which +# that front listener is NEVER CREATED and the terminated Quarkus HTTPS listener owns the +# public port directly. Against the base document that claim is false: the base declares two +# passthrough_sni entries for the whole run, so the "empty" arm was measuring the identical +# proxied route, through the identical listener stack, as the `proxiedStatic` baseline it was +# being compared against. The gate could not fail for the reason it existed to catch, and it +# could not pass for that reason either — it was comparing a configuration against itself. +# +# CONSEQUENCE FOR EDITS. Any change to ../sheriff-config/gateway.yaml that is NOT about +# passthrough must be mirrored here, or the two arms start differing in a second variable and +# the no-regression comparison silently stops being a controlled one. Do NOT add a +# `passthrough_sni` key here to "fix" a passthrough test: this instance exists precisely to +# have none. Because no front listener is created, this instance sets no QUARKUS_HTTP_SSL_PORT +# either — the terminated listener keeps 8443 and needs no loopback relocation. +# +# Global gateway document mounted into the api-sheriff container at +# /app/sheriff-config (SHERIFF_CONFIG_DIR). The proxy integration suite exercises +# the single httpbin endpoint's /proxy route under the 'api' anchor; the disjoint +# 'bff' anchor demonstrates the two-anchor namespace model (ADR-0007). Anchor +# materialization is behaviour-neutral for this conforming config, so the existing +# routed-request IT keeps passing. +# +# Every anchor carries the two mandatory classification axes (ADR-0013): `type` +# (proxy | bff | asset) and `access` (public | authenticated), boot-validated by the +# fail-closed access->auth matrix. A `public` anchor declares NO auth block (public is +# the absence of auth), so the require: none postures that previously lived on the +# public anchors now live on their endpoints; the `authenticated` anchors keep their +# non-none auth floor. The two `asset` anchors (ADR-0014) serve static content through +# the gateway-owned response envelope: `assets-public` (public directory + upstream +# sources) and `assets-secure` (authenticated directory source, bearer-gated). +version: 1 +metadata: + config_version: "integration-test" +allowed_methods: ["GET", "POST", "PUT", "DELETE"] +# The gateway-wide inbound-filter baseline (ADR-0024). Declared EXPLICITLY rather than left to +# the omitted-block default: the default is now `strict`, so the pinned value here documents the +# posture the whole IT suite is measured against instead of letting it drift with the default. +# Every route that declares no `security_filter.profile` resolves to this value — including the +# /proxy, /bff, /graphql and asset routes — so the strict preset's caps (1 MiB body, 20 query +# parameters, 1024-character header values and parameter values, 1024-character paths) are the +# floor the suite proves. The single `minimal`-mode route below is the only opt-out, and it is a +# PARTIAL one. +# +# `max_authorization_header_value_length` bounds the `Authorization` header VALUE only, at the +# non-skippable pre-route floor (BasicChecksStage) that runs BEFORE route selection. The strict +# preset's 1024-character header-value cap cannot serve that header: a Keycloak-minted access token +# plus the `Bearer ` prefix measures above it, so without this carve-out every bearer request is +# rejected 400 before bearer validation ever runs. The key is gateway-wide because its enforcement +# point precedes route selection — no anchor is resolved there, so it cannot be scoped per route. +# Declared EXPLICITLY here, per the same convention as `profile` above, so the IT and benchmark +# suites measure a pinned value rather than the omitted-key default. A declared value below the +# resolved baseline cap is refused at boot. +# +# `allow_get_with_content_length_body` admits a Content-Length-framed body on GET. Declared true here +# so the shipped descriptor actually ACTIVATES the opt-in rather than leaving it proven by unit tests +# alone (GetWithBodyActivationWiringTest asserts this declaration). It is a PARTIAL relaxation: only +# the declared-Content-Length leg is skipped, and only for GET — a GET whose body carries no declared +# Content-Length is not Content-Length-framed and stays rejected. Transfer-Encoding +# on GET remains REJECTED unconditionally — chunked framing on an otherwise-bodyless method is the +# request-smuggling shape the framing gate exists to constrain — and a body on HEAD stays rejected on +# every leg. The CL+TE and Connection-header framing-strip defences are untouched. +security_defaults: + profile: strict + max_authorization_header_value_length: 8192 + allow_get_with_content_length_body: true +tls: + # TLS POLICY for the terminated listener, single-sourced from this document and bound onto the + # listener by tls/TlsServerCustomizer (ADR-0025). Declared here rather than left implicit so the + # integration stack actually EXERCISES a gateway.yaml-sourced protocol floor and cipher allowlist + # — the keys these replaced were raw quarkus.tls.* properties that never reached the listener, so + # a fixture that declares nothing would prove only that the absence of policy is harmless. + # + # The floor is 1.2 rather than 1.3 deliberately: it enables TLSv1.2 AND TLSv1.3, so the suite's + # mixed clients keep negotiating and the assertion under test is that the DECLARED policy is in + # force, not that the strictest possible policy is survivable. + min_version: "1.2" + # The allowlist spans both enabled protocols on purpose. TLS_AES_* are TLS 1.3 suites; the + # ECDHE_RSA pair covers TLS 1.2 against the RSA server certificate the certificates/ scripts + # generate. Listing only 1.3 suites under a 1.2 floor would leave TLS 1.2 with nothing to + # negotiate — a listener that silently accepts no 1.2 handshake at all. + cipher_suites: + - TLS_AES_256_GCM_SHA384 + - TLS_AES_128_GCM_SHA256 + - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 + - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 + # ALPN advertises h2 alongside http/1.1 so the http2 benchmark aspect negotiates + # HTTP/2 at the edge instead of silently falling back to HTTP/1.1 — a fallback + # would report an h2 number that was never measured over h2. + alpn: ["h2", "http/1.1"] + # NO `passthrough_sni` KEY HERE — see the header. Its absence is what this instance exists to + # provide, and it is the ONLY difference from ../sheriff-config/gateway.yaml. +# Add-only extension of the gateway's built-in asset content-type map, serving the asset anchors +# declared below. Declared here so the shipped descriptor ACTIVATES the feature rather than leaving +# it proven by unit tests alone: `webmanifest` is an extension the gateway does not map, so without +# this block a served .webmanifest would fall back to application/octet-stream. +# +# The block is add-only and the gateway enforces that at BOOT: an entry naming one of the built-in +# extensions (html, css, js, svg, png, ... — 21 in total) is refused with a message naming it, not +# silently ignored. That bound is a security property, not an ergonomic one — remapping `svg` away +# from image/svg+xml, or pointing any built-in at text/html, would make a served asset a stored-XSS +# lever on a security gateway. AssetContentTypeActivationWiringTest asserts both halves against this +# committed descriptor: the block is declared, and no declared key collides with a built-in. +asset_defaults: + content_types: + webmanifest: application/manifest+json +anchors: + api: + path_prefix: /proxy + type: proxy + access: public + bff: + path_prefix: /bff + type: proxy + access: public + # Server-mode BFF session namespace exercised by the Bff*IT suite. A require:session + # route lives here so an authenticated browser session mediates a bearer to the + # go-httpbin echo upstream (BffSessionMediationIT). type: bff forces access: + # authenticated (ADR-0013), and the session floor is backed by the global oidc block + # below (ConfigValidator rejects a session floor with no oidc block). The reserved + # OIDC endpoints themselves are carved out of the route table on the oidc host and + # live under /auth (see the oidc block), disjoint from this /bff-session proxy prefix. + bff-session: + path_prefix: /bff-session + type: bff + access: authenticated + auth: + require: session + # Bearer-protected namespace exercised by BearerValidationIT. The gateway's own + # token_validation issuer loads its key set from the mounted static JWKS file + # (no Keycloak dependency), so the validator is ready offline at boot. The IT + # only drives rejection scenarios (missing / malformed token -> 401), which never + # reach the upstream — proving upstream count = 0 on every bearer rejection. + secure: + path_prefix: /secure + type: proxy + access: authenticated + auth: + require: bearer + # --- Benchmark aspect namespaces (plan 04b) ------------------------------- + # These give the k6 aspect matrix an API Sheriff side to measure, so the + # on-demand APISIX comparison has a route-for-route counterpart. + graphql: + path_prefix: /graphql + type: proxy + access: public + allowed_methods: ["POST"] + upload: + path_prefix: /upload + type: proxy + access: public + allowed_methods: ["POST"] + security_filter: + # 64 MiB — above the 50 MB upload aspect so the large-body run is MEASURED + # rather than rejected at the edge. A rejection path is faster than a + # transfer path, so an under-sized cap would report flatteringly good + # numbers instead of failing. APISIX mirrors this same limit. + max_body_bytes: 67108864 + # Pre-provisioned declarations only — no behaviour is implemented here. The + # request pipeline still boot-rejects the ws/grpc protocols; roadmap plan 05 + # owns that behaviour and slots it in without re-touching this topology. + ws: + path_prefix: /ws + type: proxy + access: public + grpc: + path_prefix: /grpc + type: proxy + access: public + # --- Asset terminal-action namespaces (ADR-0014) -------------------------- + # A public asset surface serving static content two ways: a directory source + # backed by the read-only /app/assets volume mount, and an upstream source + # backed by the secondary 'asset-origin' static server (topology alias + # ASSET_ORIGIN). Only GET/HEAD are served; the gateway-owned response envelope + # sets the content type from the file extension, adds X-Content-Type-Options: + # nosniff, and strips any upstream Set-Cookie. + assets-public: + path_prefix: /assets + type: asset + access: public + allowed_methods: ["GET", "HEAD"] + # A bearer-gated asset surface (directory source). access: authenticated forces + # the envelope to Cache-Control: no-store and, crucially, enforces auth BEFORE the + # source is resolved — an unauthenticated request is rejected 401 and never reads + # a file (auth-before-source, ADR-0014). The bearer floor reuses the same offline + # token_validation issuers the 'secure' anchor relies on. + assets-secure: + path_prefix: /secure-assets + type: asset + access: authenticated + allowed_methods: ["GET", "HEAD"] + auth: + require: bearer +token_validation: + issuers: + # Retained: BearerValidationIT's rejection scenarios resolve against this + # offline static-JWKS issuer, which needs no Keycloak at boot. + - name: it-static + issuer: https://api-sheriff.test/it + jwks: + source: file + file: /app/certificates/test-jwks.json + # The bearer benchmark aspect mints its token from the Keycloak 'benchmark' + # realm. That realm's import pins frontendUrl https://keycloak:8443, so a + # minted token carries iss https://keycloak:8443/realms/benchmark regardless + # of the host-published 1443 mint port — the issuer value below MUST be that + # container-internal URL, not the host-published one, or every bearer request + # is rejected 401 and the aspect measures the rejection path while reporting + # it as bearer-validation throughput. JWKS is fetched over the shared + # api-sheriff network, where 'keycloak' resolves by service name. + - name: benchmark-keycloak + issuer: https://keycloak:8443/realms/benchmark + jwks: + source: http + url: https://keycloak:8443/realms/benchmark/protocol/openid-connect/certs + # token-sheriff's SSRF egress guard refuses a JWKS URL that resolves to a + # private address. 'keycloak' is a compose service name resolving to the + # bridge network's site-local 172.x address, so without this entry the key + # set never loads and every bearer request fails — the aspect would then + # measure a rejection path while reporting it as bearer throughput. The + # allowlist is host-exact (no wildcard) and names only this one trusted, + # benchmark-local IdP: exactly the narrow widening GW-05 / BFF-07 permit. + # The 'it-static' issuer above is deliberately NOT widened — it loads from + # a mounted file and needs no egress at all. + allowed_egress_hosts: ["keycloak"] + # Keycloak serves a self-signed certificate here, so the JWKS client must verify it + # against the stack's own trust anchors rather than the JVM default store — otherwise + # the fetch fails PKIX path building and every bearer request is rejected 401, which + # would again measure a rejection path while reporting it as bearer throughput. This + # names a trust profile; it deliberately carries no store path and no password, so this + # document stays portable and secret-free. The deployment binds the name. + tls_profile: benchmark-idp + # The server-mode BFF (oidc block below) validates the id/access tokens the + # 'integration' realm mints for the browser session. The gateway's shared + # @GatewayValidator TokenValidator is built from THIS token_validation block — not + # from the token-sheriff extension's own sheriff.token.issuers.* property surface, + # which this gateway leaves unconfigured and whose health probes it excludes — so the + # integration issuer MUST be declared here or every mediated session token is rejected. + # Same container-internal iss the integration realm pins via frontendUrl + # https://keycloak:8443, reached over the shared api-sheriff network where 'keycloak' + # resolves by service name; the same narrow SSRF egress widening and self-signed trust + # profile the benchmark issuer uses. + - name: integration-keycloak + issuer: https://keycloak:8443/realms/integration + jwks: + source: http + url: https://keycloak:8443/realms/integration/protocol/openid-connect/certs + allowed_egress_hosts: ["keycloak"] + tls_profile: benchmark-idp +# --- Server-mode BFF confidential-client block (BFF-* integration suite) ----------- +# Activates the server-mode BffRuntime (BffRuntimeProducer builds the active runtime +# only when a global oidc block declares session.mode=server AND a redirect_uri). The +# reserved OIDC endpoints are carved out of the route table exactly on the oidc host — +# the host of redirect_uri, i.e. 'localhost' — and matched by exact path, so they live +# under /auth and never collide with the /bff-session proxy anchor above. Confidential +# client: integration-client / integration-secret against the compose 'integration' +# realm (standardFlowEnabled, redirectUris '*'). OIDC discovery is lazy (first engine +# use), so boot needs no live IdP; the browser reaches the gateway on the published +# host port 10443 and Keycloak on its published host port, while the gateway reaches +# Keycloak container-internally at keycloak:8443. +oidc: + issuer: https://keycloak:8443/realms/integration + client_id: integration-client + client_secret: ${OIDC_CLIENT_SECRET} + scopes: ["openid", "profile", "email"] + # Full browser-facing callback URL. Its host ('localhost') is the oidc host every + # reserved path binds to; its origin ('https://localhost:10443') is the gateway + # origin used for same-origin return-URL validation and the default CSRF trusted origin. + redirect_uri: https://localhost:10443/auth/callback + logout: + path: /auth/logout + post_logout_redirect_uri: https://localhost:10443/auth/logout/return + # Lands the browser on a real public page instead of a deny-by-default 404. Served by the + # /assets/demo asset route (endpoints/assets.yaml), which also serves the demo SPA itself. + # + # It CANNOT simply be '/', for two independent and individually fatal reasons: + # 1. Serving '/' would need an anchor with path_prefix: /, which is structurally illegal — + # ConfigValidator.prefixContains treats a container prefix normalizing to '/' as containing + # everything, so validateAnchorPrefixDisjointness fails the boot fail-closed against every + # one of the ten anchors this document already declares. + # 2. Even were it routable, DirectoryAssetSource performs no directory-index resolution: a bare + # '/' yields an empty remainder that resolves to the directory root, fails isRegularFile and + # returns 404. + # So final_redirect names a concrete file under an anchor that already exists. This is a + # per-instance value of the same class as redirect_uri and post_logout_redirect_uri, which already + # differ across the four overlays; the mtls and ws-admission overlays keep final_redirect: / and + # are deliberately untouched. + final_redirect: /assets/demo/landing.html + backchannel_path: /auth/backchannel + session: + mode: server + store: memory + ttl_seconds: 3600 + refresh: + enabled: true + leeway_seconds: 30 + csrf: + trusted_origins: ["https://localhost:10443"] + user_info: + path: /auth/userinfo + allowed_claims: ["sub", "preferred_username", "email", "groups"] + default_view: ["sub", "preferred_username"] + login: + path: /auth/login diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/AssetContentTypeActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/AssetContentTypeActivationWiringTest.java index 1eaa09d6..c7bc645e 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/AssetContentTypeActivationWiringTest.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/AssetContentTypeActivationWiringTest.java @@ -84,7 +84,7 @@ class AssetContentTypeActivationWiringTest { * The descriptor count committed today. The glob must match at least this many, so an empty or * mis-rooted glob fails loudly instead of satisfying the per-descriptor loop vacuously. */ - private static final int COMMITTED_DESCRIPTOR_COUNT = 4; + private static final int COMMITTED_DESCRIPTOR_COUNT = 5; @Test @DisplayName("the base descriptor activates asset_defaults.content_types with at least one entry") diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java index e998af48..7a67e4b9 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java @@ -50,13 +50,13 @@ * unit test — only to a descriptor assertion like this one, or to the expensive container suite. *

* The coverage is deliberately all committed descriptors, not just the base one: - * the compose stack boots six native gateway instances over four {@code sheriff-config*} gateway + * the compose stack boots seven native gateway instances over five {@code sheriff-config*} gateway * descriptors ({@code api-sheriff}, {@code api-sheriff-mtls}, {@code api-sheriff-cookie}, - * {@code api-sheriff-cookie-2}, {@code api-sheriff-ws-admission} and - * {@code api-sheriff-plain-mgmt}), so a cap raised in any sibling descriptor pushes that instance - * into a boot abort. The descriptors are discovered by glob rather + * {@code api-sheriff-cookie-2}, {@code api-sheriff-ws-admission}, {@code api-sheriff-plain-mgmt} + * and {@code api-sheriff-passthrough-empty}), so a cap raised in any sibling descriptor pushes that + * instance into a boot abort. The descriptors are discovered by glob rather * than hard-coded, so a new {@code sheriff-config-*} directory comes under the assertion - * automatically — and a glob that matches fewer than the four present today fails rather than + * automatically — and a glob that matches fewer than the five present today fails rather than * passing vacuously. *

* It parses the committed descriptors only (YAML / properties text) and asserts the activation is @@ -82,7 +82,7 @@ class BodyLimitActivationWiringTest { * The descriptor count committed today. The glob must match at least this many, so an empty or * mis-rooted glob fails loudly instead of satisfying the per-descriptor loop vacuously. */ - private static final int COMMITTED_DESCRIPTOR_COUNT = 4; + private static final int COMMITTED_DESCRIPTOR_COUNT = 5; /** {@code LargeBodyIT}'s negative-case body size — the container override must exceed it. */ private static final long NEGATIVE_CASE_BODY_BYTES = 71303168L; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/EgressAllowlistActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/EgressAllowlistActivationWiringTest.java index 0331510d..b428e558 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/EgressAllowlistActivationWiringTest.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/EgressAllowlistActivationWiringTest.java @@ -90,7 +90,7 @@ class EgressAllowlistActivationWiringTest { * The descriptor count committed today. The glob must match at least this many, so an empty or * mis-rooted glob fails loudly instead of satisfying the per-descriptor loop vacuously. */ - private static final int COMMITTED_DESCRIPTOR_COUNT = 4; + private static final int COMMITTED_DESCRIPTOR_COUNT = 5; private static final String HTTP_SOURCE = "http"; private static final String FILE_SOURCE = "file"; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/GetWithBodyActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/GetWithBodyActivationWiringTest.java index 4a1d4619..66068fad 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/GetWithBodyActivationWiringTest.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/GetWithBodyActivationWiringTest.java @@ -16,6 +16,7 @@ package de.cuioss.sheriff.gateway.integration; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -28,6 +29,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; import org.yaml.snakeyaml.Yaml; import org.junit.jupiter.api.DisplayName; @@ -51,7 +53,25 @@ * descriptors present today fails rather than passing vacuously. Only the base descriptor is * required to declare the opt-in: the sibling instances (mtls, cookie, ws-admission) deliberately * keep the strict default, which is itself worth pinning — an accidental gateway-wide relaxation - * across every instance would go unnoticed otherwise. + * across every instance would go unnoticed otherwise. That enumeration remains exactly accurate: + * those three are still the whole set of independently-authored siblings, and every one of them is + * still held to the strict default by {@link #siblingDescriptorsKeepTheStrictDefault()}. + *

+ * The one exception is a base copy: {@code sheriff-config-passthrough-empty} is the base + * document with the {@code tls.passthrough_sni} block — and only that block — removed, because the + * absence of passthrough is the single variable the passthrough-baseline benchmark isolates. Its + * {@code security_defaults} must therefore stay identical to the base's, opt-in included. Such + * descriptors are exempted from the strict-default assertion by LITERAL PATH in + * {@link #BASE_DERIVED_DESCRIPTORS} — never by prefix, glob or substring — so the carve-out stays + * fail-closed: a sibling nobody enumerated still fails the guard, which + * {@link #theCarveOutStaysFailClosedForANonEnumeratedSibling()} pins as a negative control. + *

+ * Membership of that set is earned, not asserted. A descriptor only belongs there + * while it really is a base copy, and that claim is itself under test elsewhere: + * {@code TlsEdgeActivationWiringTest}'s single-variable equality guard asserts the + * passthrough-empty overlay equals the base document once {@code tls.passthrough_sni} is removed + * from it. Should that overlay ever drift into an independently-authored sibling, the equality guard + * goes red — so the exemption here cannot outlive the property that justifies it. *

* It parses the committed descriptors only (YAML text) and starts no container and reaches no * network. @@ -68,6 +88,24 @@ class GetWithBodyActivationWiringTest { /** The base descriptor — the one instance the suite expects the opt-in activated on. */ private static final Path BASE_DESCRIPTOR = DOCKER.resolve("sheriff-config/gateway.yaml"); + /** + * The base descriptor plus every descriptor that is a DELIBERATE COPY of it and therefore + * inherits its {@code security_defaults} verbatim. Membership is an EXPLICIT ENUMERATION of + * literal paths: no prefix match, no directory wildcard, no {@code contains} predicate. A + * descriptor that is not named here stays subject to the strict-default assertion, so a NEW + * sibling added later fails the guard unless someone deliberately adds it to this set — the + * carve-out is fail-closed by construction and cannot widen by accident. + */ + private static final Set BASE_DERIVED_DESCRIPTORS = Set.of( + BASE_DESCRIPTOR, + // A deliberate base copy for the benchmark's empty-passthrough arm: the base document + // with the tls.passthrough_sni block — and ONLY that block — removed. The absence of + // passthrough is the SINGLE variable separating the two benchmark arms, which is the + // whole point of the arm; a second difference (softening security_defaults here to + // satisfy this guard) would silently make the no-regression comparison uncontrolled. + // So this descriptor legitimately carries the base's opt-in and is exempted by name. + DOCKER.resolve("sheriff-config-passthrough-empty/gateway.yaml")); + private static final String SECURITY_DEFAULTS_KEY = "security_defaults"; private static final String OPT_IN_KEY = "allow_get_with_content_length_body"; @@ -75,7 +113,7 @@ class GetWithBodyActivationWiringTest { * The descriptor count committed today. The glob must match at least this many, so an empty or * mis-rooted glob fails loudly instead of satisfying the per-descriptor loop vacuously. */ - private static final int COMMITTED_DESCRIPTOR_COUNT = 4; + private static final int COMMITTED_DESCRIPTOR_COUNT = 5; @Test @DisplayName("the base descriptor activates the GET-body opt-in") @@ -123,15 +161,61 @@ void siblingDescriptorsKeepTheStrictDefault() throws Exception { // Assert — pinning the siblings is what makes an accidental blanket relaxation visible. for (Path descriptor : descriptors) { - if (descriptor.equals(BASE_DESCRIPTOR)) { - continue; - } Object declared = declaredOptIn(loadYaml(descriptor)); - assertTrue(declared == null || Boolean.FALSE.equals(declared), + assertFalse(relaxesTheFramingGate(descriptor, declared), descriptor + " declares " + OPT_IN_KEY + "=" + declared - + "; only the base descriptor activates the opt-in, so a relaxation here is" - + " an accidental gateway-wide widening of the framing gate"); + + "; only the enumerated base-derived descriptors " + BASE_DERIVED_DESCRIPTORS + + " activate the opt-in, so a relaxation here is an accidental gateway-wide" + + " widening of the framing gate"); + } + } + + @Test + @DisplayName("the base-copy carve-out is fail-closed: a non-enumerated sibling declaring the opt-in still fails") + void theCarveOutStaysFailClosedForANonEnumeratedSibling() { + // Arrange — a sibling of exactly the shape a future instance would take, which nobody has + // enumerated in BASE_DERIVED_DESCRIPTORS. + Path unenumerated = DOCKER.resolve("sheriff-config-not-enumerated/gateway.yaml"); + + // Act + Assert (negative control) — declaring the opt-in from a descriptor the set does not + // name IS a violation. Without this, the carve-out could silently widen into a blanket + // exemption (a prefix or "contains" predicate) and the guard would stop guarding. + assertTrue(relaxesTheFramingGate(unenumerated, Boolean.TRUE), + unenumerated + " is not enumerated in " + BASE_DERIVED_DESCRIPTORS + + ", so declaring " + OPT_IN_KEY + "=true from it must still fail the guard"); + assertFalse(relaxesTheFramingGate(unenumerated, null), + "a non-enumerated sibling that declares nothing keeps the strict default and must pass"); + assertFalse(relaxesTheFramingGate(unenumerated, Boolean.FALSE), + "a non-enumerated sibling that declares " + OPT_IN_KEY + "=false must pass"); + + // Assert (matched positive control) — each ENUMERATED base-derived descriptor may declare it. + for (Path baseDerived : BASE_DERIVED_DESCRIPTORS) { + assertFalse(relaxesTheFramingGate(baseDerived, Boolean.TRUE), + baseDerived + " is an enumerated base-derived descriptor and inherits the base's" + + " " + OPT_IN_KEY + " activation"); + // A carve-out entry naming a path that no longer exists is a silent hole: the guard would + // keep exempting a descriptor nobody can see. Pin every entry to a committed file. + assertTrue(Files.isRegularFile(baseDerived), + baseDerived + " is enumerated in BASE_DERIVED_DESCRIPTORS but is not a committed" + + " descriptor; remove the stale carve-out entry rather than leaving a hole"); + } + } + + /** + * The guard predicate shared by the committed-descriptor sweep and its negative control: whether + * {@code descriptor} widens the framing gate. Only the descriptors ENUMERATED BY LITERAL PATH in + * {@link #BASE_DERIVED_DESCRIPTORS} are exempt; every other descriptor must keep the strict + * default (the key absent, or declared {@code false}). + * + * @param descriptor the descriptor path, compared by equality against the enumeration + * @param declared the declared opt-in value, or {@code null} when absent + * @return {@code true} when the descriptor relaxes the gate without being an enumerated base copy + */ + private static boolean relaxesTheFramingGate(Path descriptor, Object declared) { + if (BASE_DERIVED_DESCRIPTORS.contains(descriptor)) { + return false; } + return declared != null && !Boolean.FALSE.equals(declared); } /** diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/TlsEdgeActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/TlsEdgeActivationWiringTest.java index 84ec7e71..695af5cc 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/TlsEdgeActivationWiringTest.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/TlsEdgeActivationWiringTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; @@ -27,9 +28,12 @@ import java.net.URI; import java.nio.file.Files; import java.nio.file.Path; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.Set; import org.yaml.snakeyaml.Yaml; import org.junit.jupiter.api.DisplayName; @@ -50,6 +54,13 @@ * every component inert. A per-component unit test structurally cannot see that gap; this descriptor * assertion is the lowest-level regression guard that can. *

+ * The {@code sheriff-config-passthrough-empty} overlay is the deliberate inverse and is guarded the + * same way: the benchmark's empty-mode arm only measures D1's zero-overhead default while that + * instance declares NO {@code passthrough_sni}, performs no internal-port split, differs from the + * base descriptor in that one key and nothing else, and shares the primary's benchmark upstream. All + * four are structural facts here rather than review conventions — the arm was previously pointed at + * the primary instance, so the no-regression gate was comparing a configuration against itself. + *

* It parses the committed descriptors only (YAML / properties / POM text) and asserts the activation * is present — it starts no container and reaches no network. * @@ -63,6 +74,27 @@ class TlsEdgeActivationWiringTest { private static final Path DOCKER = MODULE.resolve("src/main/docker"); private static final Path CERTS = DOCKER.resolve("certificates"); + /** The shared base descriptor every overlay is derived from. */ + private static final Path BASE_DESCRIPTOR = DOCKER.resolve("sheriff-config/gateway.yaml"); + + /** + * The overlay backing the benchmark's empty-passthrough arm — the base descriptor with the + * {@code tls.passthrough_sni} block, and only that block, removed. + */ + private static final Path EMPTY_PASSTHROUGH_DESCRIPTOR = + DOCKER.resolve("sheriff-config-passthrough-empty/gateway.yaml"); + + /** The compose service that mounts {@link #EMPTY_PASSTHROUGH_DESCRIPTOR}. */ + private static final String EMPTY_PASSTHROUGH_SERVICE = "api-sheriff-passthrough-empty"; + + /** The primary gateway service the empty arm is measured against. */ + private static final String PRIMARY_SERVICE = "api-sheriff"; + + private static final String PASSTHROUGH_SNI_KEY = "passthrough_sni"; + private static final String INTERNAL_SSL_PORT_KEY = "QUARKUS_HTTP_SSL_PORT"; + private static final String UPSTREAM_KEY = "TOPOLOGY_UPSTREAM"; + private static final String STATIC_BACKEND_SERVICE = "nginx-static"; + private static final String PASSTHROUGH_SNI = "passthrough.test.example"; private static final String FAULT_SNI = "fault.test.example"; @@ -70,10 +102,10 @@ class TlsEdgeActivationWiringTest { @DisplayName("the mounted gateway.yaml declares a non-empty passthrough_sni mapping the test SNIs to aliases") void primaryGatewayDeclaresPassthroughSni() throws Exception { // Arrange - Map tls = tlsBlock(DOCKER.resolve("sheriff-config/gateway.yaml")); + Map tls = tlsBlock(BASE_DESCRIPTOR); // Act - Object passthrough = tls.get("passthrough_sni"); + Object passthrough = tls.get(PASSTHROUGH_SNI_KEY); // Assert — the front listener (and the runtime Host-smuggle guard) only activate on a // non-empty passthrough_sni that names the test hostnames the ITs open connections to. @@ -92,8 +124,8 @@ void primaryGatewayDeclaresPassthroughSni() throws Exception { @DisplayName("every passthrough_sni alias resolves base-path-free in topology.properties") void passthroughAliasesResolveBasePathFree() throws Exception { // Arrange - Map tls = tlsBlock(DOCKER.resolve("sheriff-config/gateway.yaml")); - Object passthrough = tls.get("passthrough_sni"); + Map tls = tlsBlock(BASE_DESCRIPTOR); + Object passthrough = tls.get(PASSTHROUGH_SNI_KEY); assertInstanceOf(Map.class, passthrough, "tls.passthrough_sni must be a map"); @SuppressWarnings("unchecked") Map sniMap = (Map) passthrough; @@ -139,11 +171,12 @@ void composePerformsPortSplitAndPublishesMtlsInstance() throws Exception { // Act — the primary gateway must move its terminated Quarkus HTTPS listener to the internal // port so the SNI front owns the public port without a bind conflict. - List primaryEnv = environment(services, "api-sheriff"); + Map primaryEnv = environmentEntries(services, PRIMARY_SERVICE); // Assert - assertTrue(primaryEnv.contains("QUARKUS_HTTP_SSL_PORT=8444"), - "the primary api-sheriff service must set QUARKUS_HTTP_SSL_PORT=8444 (internal-port split)"); + assertEquals("8444", primaryEnv.get(INTERNAL_SSL_PORT_KEY), + "the primary api-sheriff service must set " + INTERNAL_SSL_PORT_KEY + + "=8444 (internal-port split)"); Object mtlsService = services.get("api-sheriff-mtls"); assertNotNull(mtlsService, "a dedicated api-sheriff-mtls gateway instance must be defined"); @@ -157,6 +190,90 @@ void composePerformsPortSplitAndPublishesMtlsInstance() throws Exception { assertTrue(publishesMtlsPort, "the mTLS instance must publish host port 10444 for MtlsHandshakeIT"); } + @Test + @DisplayName("the empty-passthrough overlay declares no passthrough_sni at all") + void emptyPassthroughOverlayDeclaresNoPassthroughSni() throws Exception { + // Arrange + Map tls = tlsBlock(EMPTY_PASSTHROUGH_DESCRIPTOR); + + // Act + Object passthrough = tls.get(PASSTHROUGH_SNI_KEY); + + // Assert — this is the mirror image of primaryGatewayDeclaresPassthroughSni. A non-empty + // mapping here STARTS the accept-time SNI front listener, which is precisely the thing the + // `passthroughRelayEmpty` arm claims is never created; the arm would then measure the + // identical listener stack as the baseline it is compared against. An empty mapping is + // tolerated because it activates nothing, but the key is expected to be absent outright. + assertTrue(passthrough == null || (passthrough instanceof Map sni && sni.isEmpty()), + EMPTY_PASSTHROUGH_DESCRIPTOR + " must declare no tls." + PASSTHROUGH_SNI_KEY + + " (or an empty one), otherwise the SNI front listener starts and the" + + " empty-mode benchmark arm measures the configuration it exists to exclude," + + " was: " + passthrough); + } + + @Test + @DisplayName("the empty-passthrough instance performs no internal-port split") + void emptyPassthroughInstanceSetsNoInternalSslPort() throws Exception { + // Arrange + Map environment = environmentEntries(composeServices(), EMPTY_PASSTHROUGH_SERVICE); + + // Assert — the split exists only to free the public port for a front listener. With no + // passthrough_sni there is no front listener, so relocating the terminated listener would + // move it off 8443 with nothing left listening on the published port and the instance would + // never reach readiness. + assertFalse(environment.containsKey(INTERNAL_SSL_PORT_KEY), + "the '" + EMPTY_PASSTHROUGH_SERVICE + "' service must set no " + INTERNAL_SSL_PORT_KEY + + " — with no front listener the terminated listener must keep the public port," + + " was: " + environment.get(INTERNAL_SSL_PORT_KEY)); + } + + @Test + @DisplayName("the empty-passthrough overlay differs from the base by passthrough_sni and nothing else") + void emptyPassthroughOverlayDiffersFromTheBaseOnlyByPassthroughSni() throws Exception { + // Arrange — compare PARSED structures, not file text, so comment and formatting differences + // (the overlay carries its own header) are correctly irrelevant. + Map base = loadYaml(BASE_DESCRIPTOR); + Map variant = loadYaml(EMPTY_PASSTHROUGH_DESCRIPTOR); + + // Act — remove the one key the overlay exists to drop. + Object baseTls = base.get("tls"); + assertInstanceOf(Map.class, baseTls, BASE_DESCRIPTOR + " must declare a tls block"); + ((Map) baseTls).remove(PASSTHROUGH_SNI_KEY); + + // Assert — the single-variable property the whole comparison rests on. Any second difference + // (a security_defaults tweak, a diverged anchor, a drifted issuer) silently turns the + // no-regression gate into an uncontrolled comparison, because the measured delta would then + // carry that difference too rather than passthrough activation alone. + assertEquals(base, variant, EMPTY_PASSTHROUGH_DESCRIPTOR + " must be " + BASE_DESCRIPTOR + + " with tls." + PASSTHROUGH_SNI_KEY + " removed and nothing else changed; a second" + + " difference makes the two benchmark arms differ in more than the one variable" + + " under measurement"); + } + + @Test + @DisplayName("the empty-passthrough instance shares the primary's benchmark upstream") + void emptyPassthroughInstanceSharesThePrimaryBenchmarkUpstream() throws Exception { + // Arrange + Map services = composeServices(MODULE.resolve("docker-compose.benchmark.yml")); + String primaryUpstream = environmentEntries(services, PRIMARY_SERVICE).get(UPSTREAM_KEY); + String emptyUpstream = environmentEntries(services, EMPTY_PASSTHROUGH_SERVICE).get(UPSTREAM_KEY); + + // Assert — the ADR-0012 "identical upstreams" parity precondition, asserted as an EQUALITY + // between the two declared values rather than against a hard-coded literal, so repointing the + // benchmark at a different backend keeps the two arms in lockstep instead of breaking here. + assertNotNull(primaryUpstream, + "the '" + PRIMARY_SERVICE + "' service must declare " + UPSTREAM_KEY + + " in docker-compose.benchmark.yml"); + assertEquals(primaryUpstream, emptyUpstream, + "the '" + EMPTY_PASSTHROUGH_SERVICE + "' service must declare the same " + UPSTREAM_KEY + + " as '" + PRIMARY_SERVICE + "'; a different backend on one side folds that" + + " backend's cost into the comparison and is read as passthrough overhead"); + assertTrue(dependsOn(services, EMPTY_PASSTHROUGH_SERVICE).contains(STATIC_BACKEND_SERVICE), + "the '" + EMPTY_PASSTHROUGH_SERVICE + "' service must declare '" + STATIC_BACKEND_SERVICE + + "' among its depends_on, or the benchmark can start against a backend that" + + " is not up yet"); + } + @Test @DisplayName("the client and wrong-CA mTLS keystores are provisioned") void mtlsKeystoresExist() { @@ -193,22 +310,73 @@ private static Map tlsBlock(Path gatewayYaml) throws IOException return (Map) tls; } - @SuppressWarnings("unchecked") private static Map composeServices() throws IOException { - Map doc = loadYaml(MODULE.resolve("docker-compose.yml")); + return composeServices(MODULE.resolve("docker-compose.yml")); + } + + @SuppressWarnings("unchecked") + private static Map composeServices(Path composeFile) throws IOException { + Map doc = loadYaml(composeFile); Object services = doc.get("services"); - assertInstanceOf(Map.class, services, "docker-compose.yml must declare services"); + assertInstanceOf(Map.class, services, composeFile + " must declare services"); return (Map) services; } - @SuppressWarnings("unchecked") - private static List environment(Map services, String service) { + /** + * A compose service's environment as key/value pairs, accepting BOTH compose forms — the + * {@code - KEY=VALUE} list this stack uses today and the {@code KEY: VALUE} mapping compose also + * accepts. Reading only the list form would let a form switch silently turn an absence assertion + * vacuously green. + * + * @param services the parsed {@code services} block + * @param service the service name + * @return the declared environment, empty when the service declares none + */ + private static Map environmentEntries(Map services, String service) { Object node = services.get(service); - assertNotNull(node, "docker-compose.yml must declare the '" + service + "' service"); - Map serviceMap = (Map) node; - Object env = serviceMap.get("environment"); - assertInstanceOf(List.class, env, "the '" + service + "' service environment must be a list"); - return ((List) env).stream().map(String::valueOf).toList(); + assertNotNull(node, "compose must declare the '" + service + "' service"); + assertInstanceOf(Map.class, node, "the '" + service + "' service must be a mapping"); + Object env = ((Map) node).get("environment"); + Map entries = new LinkedHashMap<>(); + switch (env) { + case Map mapForm -> mapForm.forEach((key, value) -> entries.put(String.valueOf(key), String.valueOf(value))); + case Iterable listForm -> { + for (Object entry : listForm) { + String text = String.valueOf(entry); + int split = text.indexOf('='); + if (split < 0) { + entries.put(text, ""); + } else { + entries.put(text.substring(0, split), text.substring(split + 1)); + } + } + } + case null, default -> assertNull(env, "the '" + service + "' service environment must be a list or a mapping, was: " + env); + } + return entries; + } + + /** + * A compose service's {@code depends_on} names, accepting BOTH the short list form and the long + * {@code service: {condition: ...}} mapping form this stack uses in different files. + * + * @param services the parsed {@code services} block + * @param service the service name + * @return the declared dependency names, empty when the service declares none + */ + private static Set dependsOn(Map services, String service) { + Object node = services.get(service); + assertNotNull(node, "compose must declare the '" + service + "' service"); + assertInstanceOf(Map.class, node, "the '" + service + "' service must be a mapping"); + Object declared = ((Map) node).get("depends_on"); + Set names = new LinkedHashSet<>(); + switch (declared) { + case Map mapForm -> mapForm.keySet().forEach(key -> names.add(String.valueOf(key))); + case Iterable listForm -> listForm.forEach(entry -> names.add(String.valueOf(entry))); + case null, default -> assertNull(declared, + "the '" + service + "' service depends_on must be a list or a mapping, was: " + declared); + } + return names; } @SuppressWarnings("unchecked")