From 04a7feae873282b047a6664c9120cc3aed7f4684 Mon Sep 17 00:00:00 2001 From: joey-huckabee <138994589+joey-huckabee@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:30:49 -0500 Subject: [PATCH] feat: add a qualified preview request-limiting profile Apply three independent budgets to a served tree: request rate, concurrent connections, and per-connection bandwidth. A deployment that sets only one leaves the others unbounded. Both limits answer 429 rather than the NGINX default 503, which is indistinguishable from an outage and invites clients to retry harder against a server already shedding load. The health endpoint sits outside every limit: a limited liveness probe turns a traffic spike into a restart, removing capacity exactly when it is needed. The limit outcome is logged; the limit key is not. Recording the outcome supports capacity and abuse analysis, while recording the raw client address per request would add a personal identifier to an access stream that is otherwise free of them. Documents the two ways these limits silently stop working: keyed on the direct peer address they become a global cap behind a proxy, and keyed on a client-controlled header they cease to exist while still looking configured. Three findings from running the scenario: - `limit_req` is evaluated before `limit_conn`, so a rate-rejected request records the connection limit as NOT_EVALUATED. The connection budget is therefore measured before the rate budget is spent, and the field meaning is documented so NOT_EVALUATED is not misread as "allowed". - A response that fits the socket buffer is handed to the kernel immediately and never holds a connection, so no concurrency builds up however slowly the client reads. The payload now exceeds `limit_rate_after` and the profile's own bandwidth limit paces it. - The served directory came from `mktemp -d`, which is 0700 and cannot be traversed by the container identity, so every request answered 403 before any limit applied. Log assertions can now require a field value and, where a scenario issues identical requests on purpose, be satisfied by any one of the matching events. --- CHANGELOG.md | 10 ++ docs/CONFIGURATION-PROFILES.md | 74 ++++++++++- docs/LOGGING.md | 6 +- docs/ROADMAP.md | 6 +- docs/SUPPORT.md | 1 + docs/USE-CASES.md | 6 +- examples/profiles/rate-limited/nginx.conf | 144 ++++++++++++++++++++++ tests/profiles.sh | 118 +++++++++++++++++- tests/test_profile_logs.py | 39 ++++++ tests/validate_profile_logs.py | 132 ++++++++++++++++---- 10 files changed, 496 insertions(+), 40 deletions(-) create mode 100644 examples/profiles/rate-limited/nginx.conf diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a8e64d..5fd71e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -140,6 +140,16 @@ but container releases use the upstream-derived format documented in - Fixed a race in the reload test that read `/proc/PID/status` for a worker that had already exited, which aborted the listing instead of skipping the vanished process. +- Added a qualified preview request-limiting profile applying independent + request-rate, concurrent-connection, and per-connection bandwidth budgets, + answering `429` rather than the default `503`, keeping the health endpoint + outside every limit, and recording limit outcomes without recording the limit + key; tests prove rate rejection, connection rejection specifically, and that + health checks keep answering while a client's request budget is exhausted. +- Documented that a limit keyed on the direct peer address becomes a global cap + behind a proxy, that keying on a client-controlled header removes the limit + entirely, and that `limit_req` is evaluated before `limit_conn` so a + rate-rejected request records the connection limit as not evaluated. - Added a qualified preview WebSocket-proxying profile that derives the upstream connection disposition from a map rather than copying it from the client, scopes the long idle timeout to the upgrade location, and never diff --git a/docs/CONFIGURATION-PROFILES.md b/docs/CONFIGURATION-PROFILES.md index 68b544e..84aace6 100644 --- a/docs/CONFIGURATION-PROFILES.md +++ b/docs/CONFIGURATION-PROFILES.md @@ -1,9 +1,9 @@ # Qualified HTTP and TLS configuration profiles The repository provides minimal static-serving, HTTP reverse-proxy, HTTP -load-balancing, and WebSocket-proxying configurations under +load-balancing, WebSocket-proxying, and request-limiting configurations under `examples/profiles`. Static serving, HTTP reverse proxy, HTTP load balancing, -WebSocket proxying, +WebSocket proxying, request and connection limiting, TLS termination, mutual TLS, and verified HTTPS upstream profiles are exercised on native AMD64 and ARM64 runners with rootless Podman and then with Docker compatibility execution. They remain **preview/unqualified** until an immutable @@ -61,6 +61,76 @@ Its access-event schema is: | `body_bytes_sent` | integer | Response-body bytes sent. | | `request_time` | number | Total request duration in seconds. | +## Request-rate and connection limiting + +[`examples/profiles/rate-limited/nginx.conf`](../examples/profiles/rate-limited/nginx.conf) +applies three separate budgets to a served tree: request rate, concurrent +connections, and per-connection bandwidth. They are independent, and a +deployment that sets only one leaves the others unbounded. + +### The limit key decides whether the limit exists + +The key is `$binary_remote_addr`, the direct peer address. + +Behind a load balancer or ingress controller that address is the *proxy*, so +every client shares one bucket and a per-client limit silently becomes a global +cap. A deployment in that position needs a key derived from a forwarded address +it actually trusts, through `realip` with a trusted-proxy list or an equivalent +reviewed mechanism. + +Never key a limit on a header the client controls. A client that chooses its +own key gets a fresh bucket for every request, and the limit stops existing +while continuing to look configured. + +The zones are shared across workers and are sized in advance. An exhausted zone +fails closed and rejects new clients, so size for the expected distinct-client +count rather than for steady-state traffic. + +### Status code + +Both limits answer `429`. The NGINX default is `503`, which is +indistinguishable from an outage and invites clients to retry harder against a +server that is already shedding load. + +### Evaluation order matters when reading logs + +`limit_req` is evaluated before `limit_conn`. Once the rate limit is rejecting, +the connection limit is never reached, and its field records `NOT_EVALUATED` +rather than `PASSED`. A reader who treats `NOT_EVALUATED` as "allowed" will +conclude the connection limit is inactive when it is simply downstream of a +limit that is already firing. + +### The health endpoint is outside both limits + +A limited health endpoint turns a traffic spike into a failed liveness probe +and a restart, which removes capacity exactly when it is needed. + +### Access-event schema + +The profile emits the common fields plus: + +| Field | JSON type | Meaning | +| --- | --- | --- | +| `limit_req_result` | string | `PASSED`, `DELAYED`, `REJECTED`, a dry-run variant, or `NOT_EVALUATED`. | +| `limit_conn_result` | string | `PASSED`, `REJECTED`, a dry-run variant, or `NOT_EVALUATED`. | + +The limit *key* is deliberately not logged. Recording the outcome supports +capacity and abuse analysis; recording the raw client identifier for every +request adds a personal identifier to an access stream that is otherwise free +of them. + +### Qualified behaviour + +The tests prove that a request inside both budgets passes, that exhausting the +request rate produces `429` recorded as `REJECTED`, that concurrent requests +beyond the connection maximum are rejected by the connection limit +specifically, and that the health endpoint keeps answering while the client's +request budget is exhausted. + +They do **not** qualify tuning for any particular workload, zone sizing under +real client populations, behaviour once a zone is exhausted, or the interaction +between these limits and an upstream rate limiter. + ## WebSocket proxying [`examples/profiles/websocket/nginx.conf`](../examples/profiles/websocket/nginx.conf) diff --git a/docs/LOGGING.md b/docs/LOGGING.md index a4620a0..6881fd2 100644 --- a/docs/LOGGING.md +++ b/docs/LOGGING.md @@ -83,9 +83,9 @@ and [`container/conf.d/default.conf`](../container/conf.d/default.conf). ## Qualified preview HTTP and TLS formats -The static, HTTP reverse-proxy, HTTP load-balancing, WebSocket-proxying, TLS -termination, mutual-TLS, and verified-upstream examples implement the safer -structured contract described above. +The static, HTTP reverse-proxy, HTTP load-balancing, WebSocket-proxying, +request-limiting, TLS termination, mutual-TLS, and verified-upstream examples +implement the safer structured contract described above. They use JSON escaping, validate a bounded `X-Request-ID` or generate `$request_id`, and log `$uri` rather than the query- bearing request target. Tests parse every emitted access event, exercise JSON diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 390e01a..650e81a 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -71,13 +71,13 @@ parallel only where it does not assume an unfrozen NGINX package or module set. ## Package 3: supported configurations and TLS - [ ] Provide tested, minimal examples for extended health/readiness - endpoints, rate limits, connection limits, and ClickHouse HTTP proxying. + endpoints and ClickHouse HTTP proxying. - [ ] Extend the qualified defaults to DNS resolution, upstream verification, and the remaining profiles without silently breaking general-purpose use. - [ ] Document configuration mounting, validation, reload, rollback, logging, troubleshooting, and secret redaction. -- [ ] Implement and test structured logging for ClickHouse, extended health - endpoints, and request and connection limiting. +- [ ] Implement and test structured logging for ClickHouse and extended + health endpoints. - [ ] Qualify runtime collection, rotation ownership, pipeline failure, and retention evidence for the selected logging platform. - [ ] Qualify lifecycle-alert delivery and exact platform cryptographic-policy diff --git a/docs/SUPPORT.md b/docs/SUPPORT.md index 9554571..0d85b9e 100644 --- a/docs/SUPPORT.md +++ b/docs/SUPPORT.md @@ -29,6 +29,7 @@ Absence from a matrix means unqualified, not implicitly compatible. | HTTP reverse-proxy profile | Preview/unqualified | Restricted-runtime, safe-header, logging, and upstream-failure tests exist; HTTPS upstreams and platform controls are outside this profile. | | HTTP load-balancing profile | Preview/unqualified | Distribution, passive failure handling, bounded retries, and failover logging are tested on native AMD64/ARM64 Podman and Docker compatibility; capacity, latency, draining, and affinity are not qualified. | | WebSocket proxying profile | Preview/unqualified | Upgrade forwarding, derived connection disposition, `101` relay, and unaffected plain HTTP are tested; frame exchange, session duration, and concurrent session capacity are not. | +| Request and connection limiting profile | Preview/unqualified | Rate rejection, connection rejection, bandwidth pacing, and unlimited health checks are tested; workload tuning, zone sizing, and exhausted-zone behaviour are not. | | TLS termination and mTLS profiles | Preview/unqualified | TLS 1.2/1.3, client authentication, leaf renewal, CRL enforcement, and negative cases are tested; production PKI operations and exact-host cryptographic policy remain unqualified. | | Verified HTTPS upstream profile | Preview/unqualified | Chain, hostname, SNI, revocation, overlapping-CA rotation, and restricted-runtime behavior are tested; deployment DNS, egress, and PKI remain operator-owned. | | Linux AMD64 and ARM64 | Preview/unqualified | Native CI exists; release-candidate evidence is not complete. | diff --git a/docs/USE-CASES.md b/docs/USE-CASES.md index 3e3e3b6..e5bc095 100644 --- a/docs/USE-CASES.md +++ b/docs/USE-CASES.md @@ -7,9 +7,9 @@ negative cases, runtime restrictions, and operational guidance are tested against the released image. The development image has tested preview profiles for static content, HTTP and -verified-HTTPS upstream proxying, HTTP load balancing, WebSocket proxying, TLS -termination, and mutual TLS, including health endpoints and structured logging -on unprivileged ports. The other +verified-HTTPS upstream proxying, HTTP load balancing, WebSocket proxying, +request and connection limiting, TLS termination, and mutual TLS, including +health endpoints and structured logging on unprivileged ports. The other profiles below are design targets for the first release unless stated otherwise. See [Qualified HTTP and TLS configuration profiles](CONFIGURATION-PROFILES.md) for the exact implemented boundary. diff --git a/examples/profiles/rate-limited/nginx.conf b/examples/profiles/rate-limited/nginx.conf new file mode 100644 index 0000000..92644ca --- /dev/null +++ b/examples/profiles/rate-limited/nginx.conf @@ -0,0 +1,144 @@ +worker_processes auto; +pid /tmp/nginx.pid; +error_log /dev/stderr notice; + +events { + worker_connections 1024; +} + +http { + include /etc/nginx/mime.types; + default_type application/octet-stream; + + map $http_x_request_id $correlation_id { + "~^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$" $http_x_request_id; + default $request_id; + } + + # A connection that never produced a request has no method: a failed TLS + # handshake, a malformed request line, or a client that disconnects before + # the request is read. Those are not application requests, so they stay in + # the error stream instead of emitting a structured access event with empty + # fields. + map $request_method $is_application_request { + default 1; + "" 0; + } + + # A location without a limit leaves these variables empty. Naming that case + # keeps every field in the access event non-empty, so "no value" and "the + # limit did not apply here" stay distinguishable to a reader. + map $limit_req_status $limit_req_result { + default $limit_req_status; + "" NOT_EVALUATED; + } + + map $limit_conn_status $limit_conn_result { + default $limit_conn_status; + "" NOT_EVALUATED; + } + + # The limit key is the direct peer address. + # + # Behind a load balancer or ingress controller that address is the proxy, + # so every client shares one bucket and the limit becomes a global cap + # rather than a per-client one. A deployment in that position needs a + # reviewed key derived from a forwarded address it actually trusts. + # + # Never key a limit on a header the client can set. A client that chooses + # its own key gets a fresh bucket per request and the limit stops existing. + # `realip` with a trusted-proxy list, or an equivalent reviewed mechanism, + # is what makes a forwarded address usable as a key. + # + # The zones are shared across workers. 10m holds roughly 160k IPv4 states; + # size them for the expected distinct-client count, because an exhausted + # zone fails closed and rejects new clients. + limit_req_zone $binary_remote_addr zone=requests_per_client:10m rate=10r/s; + limit_conn_zone $binary_remote_addr zone=connections_per_client:10m; + + # 429 tells a client it was rate limited. The NGINX default is 503, which + # is indistinguishable from an outage and invites clients to retry harder. + limit_req_status 429; + limit_conn_status 429; + limit_req_log_level warn; + limit_conn_log_level warn; + + log_format profile_json escape=json + '{"timestamp":"$time_iso8601",' + '"request_id":"$correlation_id",' + '"method":"$request_method",' + '"uri":"$uri",' + '"protocol":"$server_protocol",' + '"status":$status,' + '"body_bytes_sent":$body_bytes_sent,' + '"request_time":$request_time,' + '"limit_req_result":"$limit_req_result",' + '"limit_conn_result":"$limit_conn_result"}'; + access_log /dev/stdout profile_json if=$is_application_request; + + client_body_temp_path /tmp/nginx-client-body; + proxy_temp_path /tmp/nginx-proxy; + fastcgi_temp_path /tmp/nginx-fastcgi; + uwsgi_temp_path /tmp/nginx-uwsgi; + scgi_temp_path /tmp/nginx-scgi; + + server_tokens off; + client_max_body_size 1m; + client_body_timeout 10s; + client_header_timeout 10s; + keepalive_timeout 30s; + send_timeout 30s; + + server { + listen 8080 default_server; + listen [::]:8080 default_server; + server_name _; + + root /srv/www; + autoindex off; + + add_header X-Content-Type-Options nosniff always; + add_header X-Request-ID $correlation_id always; + + # The health endpoint is deliberately outside both limits. A limited + # health check turns a traffic spike into a failed liveness probe and + # a restart, which removes capacity exactly when it is needed. + location = /healthz { + access_log off; + default_type text/plain; + return 200 "ok\n"; + } + + location / { + # `burst` absorbs normal bunching; `nodelay` serves the burst + # immediately instead of queueing it, so a well-behaved client is + # not slowed down while an abusive one is still capped. Without + # `nodelay` the burst is spread at the configured rate, which looks + # like latency to the caller. + limit_req zone=requests_per_client burst=20 nodelay; + + # Concurrency is a separate budget from rate. A handful of slow + # downloads can hold every worker connection without ever + # exceeding the request rate. + limit_conn connections_per_client 10; + + # Per-connection bandwidth is the third budget. Without it a + # single client can hold a connection inside the connection limit + # while consuming the link, and the server cannot reclaim the + # socket because it has already handed the body to the kernel. + # + # `limit_rate_after` leaves small responses untouched, so ordinary + # pages are unaffected and only long transfers are paced. + limit_rate_after 1m; + limit_rate 512k; + + if ($request_method !~ ^(GET|HEAD)$) { + return 405; + } + + location ~ (?:^|/)\. { + return 403; + } + } + } +} diff --git a/tests/profiles.sh b/tests/profiles.sh index 7f6eab9..292d140 100644 --- a/tests/profiles.sh +++ b/tests/profiles.sh @@ -22,12 +22,14 @@ balancer="${prefix}-balancer" ws_network="${prefix}-ws-network" ws_backend="${prefix}-ws-backend" ws_proxy="${prefix}-ws-proxy" +limited="${prefix}-limited" tmp_root="${PROFILE_TMPDIR:-/tmp}" static_headers=$(mktemp "${tmp_root}/nginx-static-headers.XXXXXX") proxy_headers=$(mktemp "${tmp_root}/nginx-proxy-headers.XXXXXX") proxy_body=$(mktemp "${tmp_root}/nginx-proxy-body.XXXXXX") balancer_body=$(mktemp "${tmp_root}/nginx-balancer-body.XXXXXX") ws_body=$(mktemp "${tmp_root}/nginx-ws-body.XXXXXX") +limited_root=$(mktemp -d "${tmp_root}/nginx-limited-root.XXXXXX") no_new_privileges="no-new-privileges:true" if grep -qi podman <<< "$("${runtime}" --version 2>&1)"; then @@ -37,12 +39,13 @@ fi cleanup() { "${runtime}" rm --force "${static}" "${proxy}" "${backend}" \ "${pool_a}" "${pool_b}" "${balancer}" \ - "${ws_backend}" "${ws_proxy}" \ + "${ws_backend}" "${ws_proxy}" "${limited}" \ >/dev/null 2>&1 || true "${runtime}" network rm "${network}" >/dev/null 2>&1 || true "${runtime}" network rm "${ws_network}" >/dev/null 2>&1 || true rm -f -- "${static_headers}" "${proxy_headers}" "${proxy_body}" \ "${balancer_body}" "${ws_body}" + rm -rf -- "${limited_root}" } trap cleanup EXIT @@ -454,11 +457,120 @@ validate_event "${ws_proxy}" \ --status 101 \ --connection-upgrade upgrade +# --------------------------------------------------------------------------- +# Request-rate and connection limiting +# --------------------------------------------------------------------------- + +printf 'limited-profile-ok\n' > "${limited_root}/index.html" +# The payload has to exceed `limit_rate_after` so the profile's own bandwidth +# limit paces it. A response that fits in the socket buffer is handed to the +# kernel immediately and the connection is never actually held, so no +# concurrency can build up no matter how slowly the client reads. +head -c 4194304 /dev/zero | tr '\0' 'x' > "${limited_root}/payload.bin" +# mktemp -d creates the directory 0700, which the container identity cannot +# traverse, so the served tree needs an explicit mode rather than the default. +chmod 0755 "${limited_root}" +chmod 0644 "${limited_root}"/* + +run_restricted "${limited}" 10009 \ + --publish 127.0.0.1::8080 \ + --volume "${examples_dir}/rate-limited/nginx.conf:/etc/nginx/nginx.conf:ro" \ + --volume "${limited_root}:/srv/www:ro" +limited_binding=$("${runtime}" port "${limited}" 8080/tcp) +limited_port=${limited_binding##*:} +limited_url="http://127.0.0.1:${limited_port}" +wait_for_http "${limited_url}/healthz" "${limited}" +assert_process_security "${limited}" +"${runtime}" exec "${limited}" nginx -t -q -c /etc/nginx/nginx.conf + +# A single request well inside the budget must pass both limits. +test "$(curl --silent --output "${null_device}" --write-out '%{http_code}' \ + --header 'X-Request-ID: limits.pass-1' \ + "${limited_url}/index.html")" = 200 +validate_event "${limited}" \ + --profile rate-limited \ + --uri /index.html \ + --request-id limits.pass-1 \ + --status 200 \ + --require-field limit_req_result=PASSED \ + --require-field limit_conn_result=PASSED + +# Concurrency is a separate budget, and it is measured before the rate budget +# is spent. `limit_req` runs first, so once the rate limit is rejecting, the +# connection limit is never evaluated and records NOT_EVALUATED. +# +# The requests arrive together: the rate burst admits 20 of them, and the +# connection limit then rejects everything past its own maximum. Several +# events therefore share this correlation ID with different outcomes, and the +# assertion requires that at least one of them was rejected by the connection +# limit specifically. +conn_urls=() +for _ in $(seq 1 24); do + conn_urls+=("${limited_url}/payload.bin") +done +conn_codes=$(curl --silent --output "${null_device}" \ + --write-out '%{http_code}\n' \ + --parallel --parallel-immediate --parallel-max 24 \ + --limit-rate 4k --max-time 20 \ + --header 'X-Request-ID: limits.conn-1' \ + "${conn_urls[@]}" || true) +if test "$(grep -c '^429$' <<< "${conn_codes}")" -lt 1; then + printf '%s\n' "${conn_codes}" >&2 + echo "The connection limit did not reject any concurrent request" >&2 + exit 1 +fi +validate_event "${limited}" \ + --profile rate-limited \ + --uri /payload.bin \ + --request-id limits.conn-1 \ + --status 429 \ + --allow-repeated \ + --require-field limit_conn_result=REJECTED + +# Exhaust the request-rate budget. One curl invocation reuses the connection, +# so the requests arrive far faster than the configured rate; the burst is +# large enough that a slow runner still exceeds it. +burst_urls=() +for _ in $(seq 1 200); do + burst_urls+=("${limited_url}/index.html") +done +burst_codes=$(curl --silent --output "${null_device}" \ + --write-out '%{http_code}\n' \ + --header 'X-Request-ID: limits.burst-1' \ + "${burst_urls[@]}") +test "$(grep -c '^200$' <<< "${burst_codes}")" -ge 1 +if test "$(grep -c '^429$' <<< "${burst_codes}")" -lt 1; then + echo "The request-rate limit did not reject any request in the burst" >&2 + exit 1 +fi + +# The health endpoint stays outside the limit, so it must still answer while +# the client's request budget is exhausted. +test "$(curl --silent --output "${null_device}" --write-out '%{http_code}' \ + "${limited_url}/healthz")" = 200 + +# The burst shares one correlation ID on purpose: a rejected request must be +# recorded as rejected rather than silently dropped. +validate_event "${limited}" \ + --profile rate-limited \ + --uri /index.html \ + --request-id limits.burst-1 \ + --status 429 \ + --allow-repeated \ + --require-field limit_req_result=REJECTED + +limited_logs=$("${runtime}" logs "${limited}" 2>&1) +if grep -Fq '/healthz' <<< "${limited_logs}"; then + echo "The rate-limited health endpoint unexpectedly wrote an access event" >&2 + exit 1 +fi + "${runtime}" stop --time 10 "${static}" "${proxy}" "${balancer}" "${ws_proxy}" \ - >/dev/null + "${limited}" >/dev/null test "$("${runtime}" inspect --format '{{.State.ExitCode}}' "${static}")" = 0 test "$("${runtime}" inspect --format '{{.State.ExitCode}}' "${proxy}")" = 0 test "$("${runtime}" inspect --format '{{.State.ExitCode}}' "${balancer}")" = 0 test "$("${runtime}" inspect --format '{{.State.ExitCode}}' "${ws_proxy}")" = 0 +test "$("${runtime}" inspect --format '{{.State.ExitCode}}' "${limited}")" = 0 -echo "Static, reverse-proxy, load-balancer, and websocket profile qualification passed for ${image}" +echo "Static, reverse-proxy, load-balancer, websocket, and rate-limited profile qualification passed for ${image}" diff --git a/tests/test_profile_logs.py b/tests/test_profile_logs.py index aca399d..3a012b2 100644 --- a/tests/test_profile_logs.py +++ b/tests/test_profile_logs.py @@ -222,6 +222,45 @@ def test_websocket_rejects_a_client_supplied_disposition(self) -> None: with self.assertRaises(logs.ProfileLogError): logs.parse_events(encoded(invalid), "websocket") + def test_rate_limited_requires_recognised_limit_outcomes(self) -> None: + limits = {"limit_req_result": "PASSED", "limit_conn_result": "PASSED"} + expected = event(**limits) + self.assertEqual( + logs.parse_events(encoded(expected), "rate-limited"), [expected] + ) + for outcome in ("REJECTED", "DELAYED", "NOT_EVALUATED"): + with self.subTest(outcome=outcome): + valid = event( + limit_req_result=outcome, limit_conn_result="PASSED" + ) + self.assertEqual( + logs.parse_events(encoded(valid), "rate-limited"), [valid] + ) + + def test_rate_limited_rejects_unknown_limit_outcomes(self) -> None: + for changes in ( + {"limit_req_result": "", "limit_conn_result": "PASSED"}, + {"limit_req_result": "passed", "limit_conn_result": "PASSED"}, + {"limit_req_result": "PASSED", "limit_conn_result": "DELAYED"}, + {"limit_req_result": "PASSED", "limit_conn_result": "unknown"}, + ): + with self.subTest(changes=changes): + with self.assertRaises(logs.ProfileLogError): + logs.parse_events(encoded(event(**changes)), "rate-limited") + + def test_repeated_identity_is_rejected_unless_allowed(self) -> None: + # A scenario that issues identical requests on purpose opts in; every + # other scenario must still fail when an identity is ambiguous. + repeated = [event(), event()] + with self.assertRaisesRegex(logs.ProfileLogError, "found 2"): + logs.select_events(repeated, "/resource", "request.valid-1", 200) + self.assertEqual( + logs.select_events( + repeated, "/resource", "request.valid-1", 200, allow_repeated=True + ), + repeated, + ) + def test_exactly_one_scenario_event_is_required(self) -> None: observed = [event(), event(request_id="other")] self.assertEqual( diff --git a/tests/validate_profile_logs.py b/tests/validate_profile_logs.py index c6d1143..cadc0b0 100644 --- a/tests/validate_profile_logs.py +++ b/tests/validate_profile_logs.py @@ -29,6 +29,18 @@ "upstream_response_time", } WEBSOCKET_FIELDS = {"connection_upgrade"} +LIMIT_FIELDS = {"limit_req_result", "limit_conn_result"} +# NGINX reports these outcomes; the profile maps the empty value, which means +# the limit was not evaluated for that location, onto an explicit token. +LIMIT_REQ_RESULTS = { + "PASSED", + "DELAYED", + "REJECTED", + "DELAYED_DRY_RUN", + "REJECTED_DRY_RUN", + "NOT_EVALUATED", +} +LIMIT_CONN_RESULTS = {"PASSED", "REJECTED", "REJECTED_DRY_RUN", "NOT_EVALUATED"} TLS_FIELDS = { "tls_protocol", "tls_cipher", @@ -41,6 +53,7 @@ "reverse-proxy", "load-balancer", "websocket", + "rate-limited", "tls-termination", "mutual-tls", "tls-upstream", @@ -107,11 +120,13 @@ def parse_events( "tls-upstream", } websocket = profile == "websocket" + limited = profile == "rate-limited" tls = profile in {"tls-termination", "mutual-tls"} fields = ( COMMON_FIELDS | (UPSTREAM_FIELDS if upstream else set()) | (WEBSOCKET_FIELDS if websocket else set()) + | (LIMIT_FIELDS if limited else set()) | (TLS_FIELDS if tls else set()) ) events = [] @@ -137,6 +152,11 @@ def parse_events( # the log or the map was changed without updating this contract. if event["connection_upgrade"] not in {"upgrade", "close"}: fail("connection_upgrade must be 'upgrade' or 'close'") + if limited: + if event["limit_req_result"] not in LIMIT_REQ_RESULTS: + fail("limit_req_result is not a recognised limit outcome") + if event["limit_conn_result"] not in LIMIT_CONN_RESULTS: + fail("limit_conn_result is not a recognised limit outcome") if tls: for field in TLS_FIELDS: if not isinstance(event[field], str): @@ -165,10 +185,20 @@ def upstream_attempts(event: dict[str, object]) -> int: return sum(len(part.split(" : ")) for part in addresses.split(", ")) -def select_event( - events: Iterable[dict[str, object]], uri: str, request_id: str, status: int -) -> dict[str, object]: - """Require exactly one event matching the scenario identity.""" +def select_events( + events: Iterable[dict[str, object]], + uri: str, + request_id: str, + status: int, + allow_repeated: bool = False, +) -> list[dict[str, object]]: + """Return the events matching the scenario identity. + + A scenario that deliberately issues many identical requests, such as + exhausting a limit, cannot give each one its own correlation ID. Those + scenarios set `allow_repeated`, and the caller then requires that at least + one of the matches satisfies the remaining assertions. + """ matches = [ event for event in events @@ -176,9 +206,18 @@ def select_event( and event["request_id"] == request_id and event["status"] == status ] - if len(matches) != 1: + if not matches: + fail("expected exactly one matching event; found 0") + if len(matches) != 1 and not allow_repeated: fail(f"expected exactly one matching event; found {len(matches)}") - return matches[0] + return matches + + +def select_event( + events: Iterable[dict[str, object]], uri: str, request_id: str, status: int +) -> dict[str, object]: + """Require exactly one event matching the scenario identity.""" + return select_events(events, uri, request_id, status)[0] def main() -> int: @@ -195,6 +234,21 @@ def main() -> int: choices=("upgrade", "close"), help="require this derived connection disposition on the matching event", ) + parser.add_argument( + "--require-field", + action="append", + default=[], + metavar="NAME=VALUE", + help="require the matching event to carry this exact field value", + ) + parser.add_argument( + "--allow-repeated", + action="store_true", + help=( + "permit several events to share the scenario identity, for a " + "scenario that issues identical requests on purpose" + ), + ) parser.add_argument( "--upstream-attempts", type=int, @@ -205,30 +259,56 @@ def main() -> int: ) args = parser.parse_args() + requirements = [] + for requirement in args.require_field: + name, separator, expected = requirement.partition("=") + if not separator: + parser.error("--require-field expects NAME=VALUE") + requirements.append((name, expected)) + try: events = parse_events(sys.stdin.read(), args.profile, args.forbidden) - event = select_event(events, args.uri, args.request_id, args.status) + matches = select_events( + events, args.uri, args.request_id, args.status, args.allow_repeated + ) except ProfileLogError as exc: parser.error(str(exc)) - if event["method"] != "GET": - parser.error("matching event has an unexpected request method") - if args.connection_upgrade is not None: - if "connection_upgrade" not in event: - parser.error("profile does not record a connection disposition") - if event["connection_upgrade"] != args.connection_upgrade: - parser.error( - f"expected connection_upgrade {args.connection_upgrade}; " - f"observed {event['connection_upgrade']}" - ) - if args.upstream_attempts is not None: - if "upstream_addr" not in event: - parser.error("profile does not record upstream attempts") - observed = upstream_attempts(event) - if observed != args.upstream_attempts: - parser.error( - f"expected {args.upstream_attempts} upstream attempts; " - f"observed {observed}" - ) + + def unmet(event: dict[str, object]) -> str | None: + """Return why this event fails the assertions, or None if it passes.""" + if event["method"] != "GET": + return "matching event has an unexpected request method" + if args.connection_upgrade is not None: + if "connection_upgrade" not in event: + return "profile does not record a connection disposition" + if event["connection_upgrade"] != args.connection_upgrade: + return ( + f"expected connection_upgrade {args.connection_upgrade}; " + f"observed {event['connection_upgrade']}" + ) + for name, expected in requirements: + if name not in event: + return f"matching event has no field {name}" + if str(event[name]) != expected: + return f"expected {name} {expected}; observed {event[name]}" + if args.upstream_attempts is not None: + if "upstream_addr" not in event: + return "profile does not record upstream attempts" + observed = upstream_attempts(event) + if observed != args.upstream_attempts: + return ( + f"expected {args.upstream_attempts} upstream attempts; " + f"observed {observed}" + ) + return None + + # When several events share the scenario identity on purpose, the + # assertions describe the outcome under test rather than every request that + # happened to carry the same correlation ID, so one satisfying event is the + # contract. Reporting the first reason keeps the failure readable. + reasons = [unmet(event) for event in matches] + if all(reason is not None for reason in reasons): + parser.error(reasons[0]) print(f"validated {args.profile} structured access event") return 0