From ed4554eb6b24a4e011b797d17b79896f4ee4d495 Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Wed, 14 Oct 2026 17:12:52 +0000 Subject: [PATCH] kind: run the rate limiters on Redis by default, as deployments do Kind deployed space-redis in both bring-up paths, but no chart set SPACE_REDIS_URL, so the cluster activation limiter and the per-client concurrency limiter ran on their fail-open (`misconfigured`) arms in every run except the one rate-limiter scenario, which patched the URL into space-api itself. The charts now render SPACE_REDIS_URL=redis://space-redis:6379 where the code reads it: space-api and space-admin-api (both limiters are built above the --surface branch, in AppState::from_pools_with_lifecycle) and space-cluster-controller (NativePublicOperationExecutor builds its own activation limiter from ClusterSandboxAdmissionConfig::from_env). The internal-api binary links no reader, so it gets nothing. The activation limiter runs at the binary defaults and per-client concurrency runs in `observe`, which deployments pin; observe needs no allowlist, since unrecognised ids are metered in the shared `unspecified` bucket. The infra/space/cluster installer reuses these charts but provisions no Redis, so its defaults keep redis.url empty for both, and an empty URL now renders no SPACE_REDIS_URL: an install that names no store renders as before, with both limiters off. Its values.example.yaml shows how to name the caller's store, and templates/validate.yaml refuses the Kind endpoint and API and controller URLs that differ (they share the activation bucket). Redis stays plaintext: the workspace's Redis clients are built with `tls-rustls-webpki-roots`, so they trust only webpki roots and would refuse a per-cluster CA, and the `#insecure` fragment would be an environment-only switch. The README's Differences table records that and the internal-api omission. Redis gets a PING readiness probe, and Tilt now starts space-api after it (bootstrap.sh already waited for its rollout before the API). local.rate_limiters_enforce_and_fail_open no longer writes the URL. It checks the API has one, shrinks the public API's caps (keeping the burst test hook, since the default burst of 60 would take 60+ VM boots to drain) and switches per-client concurrency to enforce, scales Redis to zero for the fail-open half, and adds a recovery phase: once Redis is back, a connect burst draws a per-client 429 again. The controller's copy of the activation caps is left at the defaults, and the scenario says why: it sends only ECW requests, which the API admits itself. The scenario's faults are recoverable when its own cleanup does not run or fails: - The public-API mutation Lease records the standing limiter env and the Redis replica count before either changes. A record is cleared only once its restore succeeded. - Every acquisition of that Lease, including of a released one, puts back whatever is still recorded. A Lease with a repair can also be taken over from a holder proven dead (same user and host, pid gone), so a killed run does not block the next one for the Lease's hour. - Every write under the Lease first renews it in one patch guarded by uid, resourceVersion and holder, which also sets or clears the records. A runner resumed after a successor took over the Lease fails there instead of undoing the successor's fault or erasing its records. - Before every scenario on the kind backend, `space_smoke run` reads the Lease. If a fault is recorded, it acquires the Lease to repair it, or refuses to run while a live holder has it up. The worker-cordon Lease's repair also runs on a released Lease now. Also removes kind_stack_ensure_redis, whose only caller script is gone, and corrects the reference README's sentence about how Kind sets the URL. --- Tiltfile | 12 +- infra/space/aws/reference/README.md | 5 +- .../cluster/chart/templates/validate.yaml | 9 + infra/space/cluster/chart/values.yaml | 12 + infra/space/cluster/values.example.yaml | 7 + infra/space/kind/README.md | 3 + .../space-api-local/templates/deployment.yaml | 5 + .../kind/charts/space-api-local/values.yaml | 26 +- .../templates/deployment.yaml | 5 + .../values.yaml | 10 + .../templates/deployment.yaml | 7 +- .../kind/charts/space-redis-local/values.yaml | 8 +- infra/space/kind/scripts/bootstrap.sh | 5 +- scripts/tests/lib/bootstrap-kind-stack.sh | 36 +- space_smoke/__main__.py | 9 +- space_smoke/adapters.py | 352 ++++++++++++++++-- space_smoke/rate_limiters.py | 80 +++- ...cal_rate_limiters_enforce_and_fail_open.py | 33 +- space_smoke/services.py | 21 ++ 19 files changed, 528 insertions(+), 117 deletions(-) diff --git a/Tiltfile b/Tiltfile index 60df93b8ad..63adb85e42 100644 --- a/Tiltfile +++ b/Tiltfile @@ -513,10 +513,12 @@ k8s_resource( # API server. # Per-sandbox pods need the shim wired into containerd before the first create -# lands. +# lands, and the rate limiters' store should answer before the first create +# reaches them (they would admit it open, not fail it). SPACE_API_DEPS = ["space-postgres", "label-kind-workers", "label-kind-infra", "advertise-kind-worker-network-slots", "install-space-shim", - "internal-rpc-tls", "space-sandbox-network-policy", "space-db-migrate"] + "internal-rpc-tls", "space-sandbox-network-policy", "space-db-migrate", + "space-redis"] # No port_forward: localhost:8080 is the cluster's host port mapping onto the # space-api-entry NodePort (kind-cluster.yaml.tmpl), which spreads connections # across the autoscaled replicas as a deployment's load balancer does. @@ -644,9 +646,9 @@ k8s_resource( ) # Redis: shared store for the two API rate limiters (cluster activation -# admission + per-client concurrency). Public docker.io image pulled directly by -# the kind nodes (no `kind load`). Idle unless SPACE_REDIS_URL points at it; the -# rate-limiter smoke (scripts/tests/smoke-kind-rate-limiters.sh) wires it up. +# admission + per-client concurrency), which space-api, space-admin-api and +# space-cluster-controller reach through SPACE_REDIS_URL. Public docker.io image +# pulled directly by the kind nodes (no `kind load`). space-api waits for it. k8s_yaml(helm_release("space", "infra/space/kind/charts/space-redis-local")) k8s_resource( "space-redis", diff --git a/infra/space/aws/reference/README.md b/infra/space/aws/reference/README.md index d0c99cff97..e31d75bebe 100644 --- a/infra/space/aws/reference/README.md +++ b/infra/space/aws/reference/README.md @@ -158,8 +158,9 @@ retagged image. They run in the `space` namespace, and each writes a Secret a SPACE deployment can read. `space-postgres` holds `POSTGRES_USER`, `POSTGRES_PASSWORD`, `POSTGRES_DB` and `POSTGRES_CONNECTION_STRING`, the same Secret and keys SPACE's services read in the Kind harness. `space-valkey` -holds `SPACE_REDIS_URL`, the variable SPACE's API reads its rate-limiter store -from; Kind sets that variable directly rather than from a Secret. +holds `SPACE_REDIS_URL`, the variable SPACE's API and cluster-controller read +their rate-limiter store from; the Kind charts render that variable as a +literal on the API, admin API and cluster-controller rather than from a Secret. Terraform generates both passwords and keeps them in `addons/` state. Valkey holds SPACE's rate-limiter counters, a guardrail rather than a source diff --git a/infra/space/cluster/chart/templates/validate.yaml b/infra/space/cluster/chart/templates/validate.yaml index 26fe42e177..2c00506e64 100644 --- a/infra/space/cluster/chart/templates/validate.yaml +++ b/infra/space/cluster/chart/templates/validate.yaml @@ -120,6 +120,15 @@ rejects the Kind value itself. {{- if or (not $cc.snapshotEndpoint) (eq (toString $cc.snapshotEndpoint) "http://object-store:8333") -}} {{- $fail = append $fail "space-cluster-controller-local.snapshotEndpoint is required (the Kind object store is not installed)" -}} {{- end -}} +{{- /* The rate limiters' Redis is optional; this installer provides none. */ -}} +{{- $apiRedis := toString (($api.redis | default dict).url | default "") -}} +{{- $ccRedis := toString (($cc.redis | default dict).url | default "") -}} +{{- if or (contains "space-redis" $apiRedis) (contains "space-redis" $ccRedis) -}} +{{- $fail = append $fail "redis.url must not name the Kind store space-redis (this installer does not install Redis): leave it empty or name the caller's store" -}} +{{- end -}} +{{- if ne $apiRedis $ccRedis -}} +{{- $fail = append $fail "space-api-local.redis.url and space-cluster-controller-local.redis.url must be the same: both consume the cluster activation bucket" -}} +{{- end -}} {{- end -}} {{- /* Node services: the object store. */ -}} diff --git a/infra/space/cluster/chart/values.yaml b/infra/space/cluster/chart/values.yaml index 61412a2245..5732e265b9 100644 --- a/infra/space/cluster/chart/values.yaml +++ b/infra/space/cluster/chart/values.yaml @@ -28,6 +28,18 @@ space-api-local: entry: type: ClusterIP ecwHosts: "" + # This installer provisions no Redis, so the rate limiters' store is off + # unless the caller names one (values.example.yaml): an unset URL renders no + # SPACE_REDIS_URL and both limiters fail open, as they did before Kind turned + # its store on. templates/validate.yaml refuses the Kind endpoint and a + # URL that differs from space-cluster-controller-local's. + redis: + url: "" + +space-cluster-controller-local: + # Shares the API's cluster-activation bucket, so it names the same store. + redis: + url: "" space-data-plane-local: entry: diff --git a/infra/space/cluster/values.example.yaml b/infra/space/cluster/values.example.yaml index 96454f48c1..604fdcf9bd 100644 --- a/infra/space/cluster/values.example.yaml +++ b/infra/space/cluster/values.example.yaml @@ -50,6 +50,11 @@ space-api-local: dataPlanePublicUrl: "" entry: type: ClusterIP + # Optional: the rate limiters' Redis, which this installer does not provide. + # Leave it unset to run without rate limiting; set the same URL under + # space-cluster-controller-local, which shares the activation bucket. + # redis: + # url: rediss://redis.example.com:6379 space-internal-api-local: clusterId: *clusterId @@ -84,6 +89,8 @@ space-cluster-controller-local: snapshotRegion: us-east-1 snapshotBucket: snapshots snapshotCredentialsSecretName: space-object-store + # redis: + # url: rediss://redis.example.com:6379 # the same URL as space-api-local space-sandbox-network-policy: # Every private range sandboxes must not reach: pod, Service and node CIDRs diff --git a/infra/space/kind/README.md b/infra/space/kind/README.md index ec7cf04eb2..badd3d8dc6 100644 --- a/infra/space/kind/README.md +++ b/infra/space/kind/README.md @@ -995,6 +995,7 @@ kind cluster: space-local (kind-cluster.yaml.tmpl — 6 nodes) ├── worker3 [pool=infra, space.pplx.net/infra=true] │ ├── space-postgres (StatefulSet; PostgreSQL 17 serving TLS only, non-superuser master user) │ ├── space-object-store (versitygw S3 gateway + bucket-init Job) +│ ├── space-redis (Deployment; the rate limiters' store, which api, admin-api and cluster-controller reach through SPACE_REDIS_URL) │ ├── space-api (Deployment, 2–3 replicas by HPA; SPACE_K8S_CONTROL_PLANE=true) │ ├── space-admin-api (Deployment; the same binary on the dedicated admin surface) │ ├── space-internal-api (Deployment; ClusterApi — node registration + template releases) @@ -1402,6 +1403,8 @@ asks of the hosting deployment. | Server logs lock waits; the application roles' `lock_timeout` is off (`SPACE_DB_APP_LOCK_TIMEOUT` `0`) | `log_lock_waits = on`; `SPACE_DB_APP_LOCK_TIMEOUT` is unset, which leaves `lock_timeout` off the same way. Other server parameters are the image's defaults rather than a managed parameter group | | The migration Job fails fast: one attempt, no restarts, its deadline passed to the migrator | The same `space-db-migrate` Job shape (`backoffLimit: 0`, `restartPolicy: Never`, `activeDeadlineSeconds` = `SPACE_DB_MIGRATE_JOB_DEADLINE_SECONDS`), bootstrap-roles → migrate → bootstrap-roles. `bootstrap.sh` stops on a failed Job with its logs; rerunning it replaces the Job | | S3 for snapshots + template artifacts | In-cluster versitygw (real S3 protocol, sigv4, path-style) | +| Rate limiters (cluster activation admission at the binary defaults, per-client concurrency in `observe`) on a Redis-compatible store reached over TLS (`rediss://`), its URL delivered through the `space-redis` Secret (`app_deploy.tf`) | the same limiters, modes and defaults on the in-cluster `space-redis` Deployment (one in-memory replica on the infra node), reached over plaintext `redis://space-redis:6379` rendered as a literal: the workspace's Redis clients trust only the webpki roots (`tls-rustls-webpki-roots` in `Cargo.toml`), so they would refuse a CA `internal-rpc-tls.sh` could issue, and the `#insecure` URL fragment would be an environment-only switch. `local.rate_limiters_enforce_and_fail_open` shrinks the caps and scales the store to zero for its fail-open half | +| `SPACE_REDIS_URL` on api, admin-api, cluster-controller and internal-api | on api, admin-api and cluster-controller only: the limiters that read it live in `crates/libs/api` (api and admin-api build both, the cluster-controller's public-operation dispatcher builds the activation limiter), and nothing `space-internal-api-svc` links reads it | | `space-credmgr-authority` Deployment whose platform KEK comes from an external intermediate-key broker, with TLS from its own certificate (`credmgr_authority_tls.tf`; the direct KMS backend is gone, `credmgr_authority_kms.tf`) | two replicas on the same broker path against the in-repo broker fixture (`space-kind-key-broker`), which derives keys instead of protecting them; TLS from a self-signed certificate `internal-rpc-tls.sh` issues per cluster | | Internal RPC CA and listener leaves issued by Terraform and synced by External Secrets (`internal_rpc_tls.tf`) | the same Secrets issued per cluster by `scripts/internal-rpc-tls.sh`; the ingress activation leaf names `space-api` rather than `space-ingress-activation` | | volume-manager credentials via credmgr STS/IRSA | static `SPACE_OBJECT_STORE_STATIC_*` creds (the documented local escape hatch) | diff --git a/infra/space/kind/charts/space-api-local/templates/deployment.yaml b/infra/space/kind/charts/space-api-local/templates/deployment.yaml index 6989fb45ca..79a2af3f4b 100644 --- a/infra/space/kind/charts/space-api-local/templates/deployment.yaml +++ b/infra/space/kind/charts/space-api-local/templates/deployment.yaml @@ -197,6 +197,11 @@ spec: value: {{ .Values.clusterActivationAdmission.enabled | quote }} - name: SPACE_API_CLIENT_CONCURRENCY_MODE value: {{ .Values.clientConcurrency.mode | quote }} + {{- if .Values.redis.url }} + # Both limiters' shared store; see `redis` in values.yaml. + - name: SPACE_REDIS_URL + value: {{ .Values.redis.url | quote }} + {{- end }} {{- if .Values.eppo.secretName }} - name: EPPO_SDK_KEY valueFrom: diff --git a/infra/space/kind/charts/space-api-local/values.yaml b/infra/space/kind/charts/space-api-local/values.yaml index adb0eca720..41e7bb442e 100644 --- a/infra/space/kind/charts/space-api-local/values.yaml +++ b/infra/space/kind/charts/space-api-local/values.yaml @@ -170,10 +170,34 @@ dataPlanePublicUrl: "" # default the binary ships. Writing a value here would be a copy of a Rust # constant with nothing linking the two, and would silently override it the day # it changes. The client-concurrency mode accepts `off` as the hard off. +# +# The one exception is the client-concurrency mode, which deployments pin to +# `observe` rather than inherit: it is their configured value, not a copy of +# the binary's default, and it stays `observe` if that default ever moves. +# Observe needs no allowlist to classify traffic: an id outside +# SPACE_API_CLIENT_CONCURRENCY_KNOWN_CLIENTS, or none, lands in the shared +# `unspecified` bucket, which is metered like any other +# (`ClientKey::classify` in crates/libs/api/src/client_concurrency), so the +# allowlist stays at its empty default. clusterActivationAdmission: enabled: "" clientConcurrency: - mode: "" + mode: "observe" + +# The store both limiters keep their state in: the space-redis Deployment +# (charts/space-redis-local). Each limiter reads SPACE_REDIS_URL when the +# process builds its state, above the `--surface` branch +# (`AppState::from_pools_with_lifecycle`), so the admin API reads it too: +# `ClusterSandboxAdmissionConfig::from_env` and +# `ClientConcurrencyConfig::from_env`. Without it both fail open as +# `misconfigured`. Plaintext `redis://`, unlike a deployment's +# `rediss://`: the clients trust only the webpki roots (the +# `tls-rustls-webpki-roots` feature in the workspace Cargo.toml), so they +# would refuse a per-cluster CA. Must equal `redis.url` in the +# cluster-controller chart, whose copy of the activation limiter shares the +# bucket. +redis: + url: "redis://space-redis:6379" eppo: secretName: "" diff --git a/infra/space/kind/charts/space-cluster-controller-local/templates/deployment.yaml b/infra/space/kind/charts/space-cluster-controller-local/templates/deployment.yaml index 69b4eea8c7..de3c6e9281 100644 --- a/infra/space/kind/charts/space-cluster-controller-local/templates/deployment.yaml +++ b/infra/space/kind/charts/space-cluster-controller-local/templates/deployment.yaml @@ -180,6 +180,11 @@ spec: # `api.clusterActivationAdmission.enabled`. - name: SPACE_CLUSTER_ACTIVATION_ADMISSION_ENABLED value: {{ .Values.clusterActivationAdmission.enabled | quote }} + {{- if .Values.redis.url }} + # The limiter's shared store; see `redis` in values.yaml. + - name: SPACE_REDIS_URL + value: {{ .Values.redis.url | quote }} + {{- end }} {{- range $name, $reconciler := .Values.reconcilers }} {{- range $key, $value := (default (dict) $reconciler.env) }} {{- if ne $key "SPACE_SNAPSHOT_RETENTION_SWEEPER_MIN_AGE_SECS" }} diff --git a/infra/space/kind/charts/space-cluster-controller-local/values.yaml b/infra/space/kind/charts/space-cluster-controller-local/values.yaml index 0a33de2e0c..90f7a10d44 100644 --- a/infra/space/kind/charts/space-cluster-controller-local/values.yaml +++ b/infra/space/kind/charts/space-cluster-controller-local/values.yaml @@ -18,6 +18,16 @@ clusterId: "kind-local" clusterActivationAdmission: enabled: "" +# The activation limiter's store. The public-operation dispatcher +# (`NativePublicOperationExecutor::new`) builds its limiter from +# `ClusterSandboxAdmissionConfig::from_env`, which reads SPACE_REDIS_URL; +# without it this copy admits every queued activation as `misconfigured`. +# **Must equal `redis.url` in `charts/space-api-local/values.yaml`**: both +# copies consume from one bucket key, so a second store would double the +# cluster's rate. This process runs no per-client concurrency limiter. +redis: + url: "redis://space-redis:6379" + # Telemetry defaults come from the space-kind-telemetry library. telemetry: {} diff --git a/infra/space/kind/charts/space-redis-local/templates/deployment.yaml b/infra/space/kind/charts/space-redis-local/templates/deployment.yaml index 42c3cbc9de..9637247ec3 100644 --- a/infra/space/kind/charts/space-redis-local/templates/deployment.yaml +++ b/infra/space/kind/charts/space-redis-local/templates/deployment.yaml @@ -48,9 +48,12 @@ spec: - name: redis containerPort: {{ .Values.service.port }} protocol: TCP + # Ready once the server answers PING, not merely once its port + # accepts connections: the limiters fail open on any store error, so + # a ready endpoint should be one that serves their commands. readinessProbe: - tcpSocket: - port: redis + exec: + command: ["redis-cli", "-p", "{{ .Values.service.port }}", "ping"] initialDelaySeconds: 2 periodSeconds: 5 livenessProbe: diff --git a/infra/space/kind/charts/space-redis-local/values.yaml b/infra/space/kind/charts/space-redis-local/values.yaml index b05104a2f3..bf60e07813 100644 --- a/infra/space/kind/charts/space-redis-local/values.yaml +++ b/infra/space/kind/charts/space-redis-local/values.yaml @@ -1,7 +1,7 @@ # Redis for Kind's cluster activation and per-client concurrency limiters. -# Both use SPACE_REDIS_URL. This chart provides a local store for -# scripts/tests/smoke-kind-rate-limiters.sh; without a configured URL the -# limiters fail open and this pod idles. +# space-api, space-admin-api and space-cluster-controller reach it through +# SPACE_REDIS_URL (their charts' `redis.url`), as deployed services reach +# theirs, so every Kind run exercises both limiters against a real store. # # Single-node, in-memory only (no RDB/AOF persistence): the limiter state is a # guardrail, not a source of truth, and a wiped store just refills the buckets. @@ -14,7 +14,7 @@ image: datadogTeam: team-space service: - # DNS name the api-server reaches via SPACE_REDIS_URL=redis://space-redis:6379. + # DNS name in the API and cluster-controller charts' `redis.url`. name: space-redis port: 6379 diff --git a/infra/space/kind/scripts/bootstrap.sh b/infra/space/kind/scripts/bootstrap.sh index 571f8954ad..46974e82c3 100755 --- a/infra/space/kind/scripts/bootstrap.sh +++ b/infra/space/kind/scripts/bootstrap.sh @@ -374,8 +374,9 @@ log "waiting for object-store Deployment to be ready" kubectl --context "$KCTX" -n "$NAMESPACE" rollout status deploy/space-object-store --timeout="${TIMEOUT}s" # Redis — shared store for the API rate limiters (cluster activation admission -# + per-client concurrency). Idle unless SPACE_REDIS_URL points at it; the -# rate-limiter smoke (scripts/tests/smoke-kind-rate-limiters.sh) wires that up. +# + per-client concurrency), which space-api, space-admin-api and +# space-cluster-controller reach through SPACE_REDIS_URL. Ready before any of +# them starts, so the first create meets the limiters, not their fail-open arm. helm_apply space "$KIND_DIR/charts/space-redis-local" log "waiting for redis Deployment to be ready" kubectl --context "$KCTX" -n "$NAMESPACE" rollout status deploy/space-redis --timeout="${TIMEOUT}s" diff --git a/scripts/tests/lib/bootstrap-kind-stack.sh b/scripts/tests/lib/bootstrap-kind-stack.sh index 17be5b190d..2199aa612f 100755 --- a/scripts/tests/lib/bootstrap-kind-stack.sh +++ b/scripts/tests/lib/bootstrap-kind-stack.sh @@ -24,9 +24,8 @@ # consuming the timeout; a Tilt bound to a deleted cluster is restarted. # # Public entry points: `bootstrap_kind_stack`, `kind_stack_prepare_cluster`, plus -# `kind_stack_sweep_stale_node_tombstones`, `kind_stack_psql`, and -# `kind_stack_ensure_redis` for suites that have to repeat a bring-up step in -# their own setup or cleanup. +# `kind_stack_sweep_stale_node_tombstones` and `kind_stack_psql` for suites that +# have to repeat a bring-up step in their own setup or cleanup. # # Inputs (env vars, all optional with defaults set by the caller): # API_URL — health-check URL (e.g. http://localhost:8080) @@ -664,37 +663,6 @@ kind_stack_psql() { psql -U "$KIND_STACK_PG_USER" -d "$KIND_STACK_PG_DB" -tA -v ON_ERROR_STOP=1 "$@" } -# Redis, the store behind the two API rate limiters. Part of the harness -# (`bootstrap.sh` / the Tiltfile), so this renders the chart only when it is -# missing — which keeps the normal path from re-applying it while still letting a -# suite run against a stack that predates the chart. It always scales to 1 and -# waits, so it also recovers a prior run that died with Redis scaled to zero. -# -# Here rather than in each suite because it is bring-up, not assertion. Two -# suites had the same twelve lines when it was extracted; one of them -# (`smoke-kind-node-selector-extras.sh`) dropped its Redis setup later in the same -# change, once the token bucket turned out not to affect either of its phases — so -# `smoke-kind-rate-limiters.sh` is the only caller today. -kind_stack_ensure_redis() { - local deploy="${1:-space-redis}" - # Declared here rather than in the bring-up loop above, because a suite - # running with SKIP_BOOTSTRAP=1 reaches this function and never that loop. - if ! command -v helm >/dev/null 2>&1; then - echo "ERROR: helm is required to ensure the Redis chart is applied" >&2 - return 1 - fi - # Targeting flags last, per the repository's kubectl convention. None of - # these is an `exec`, where the flags would have to precede `--`. - local target=(-n "${NAMESPACE:-space}" --context "kind-${KIND_CLUSTER}") - if ! kubectl get "deploy/$deploy" "${target[@]}" >/dev/null 2>&1; then - helm template space "$REPO_ROOT/infra/space/kind/charts/space-redis-local" \ - --namespace "${NAMESPACE:-space}" \ - | kubectl apply -f - "${target[@]}" >/dev/null - fi - kubectl scale "deploy/$deploy" --replicas=1 "${target[@]}" >/dev/null 2>&1 || true - kubectl rollout status "deploy/$deploy" --timeout=120s "${target[@]}" >/dev/null -} - # Wait for `space-api` to report `/readyz=200`, up to a caller-chosen bound. # 0 when it did, 1 on timeout — the caller decides whether that is fatal. # diff --git a/space_smoke/__main__.py b/space_smoke/__main__.py index 14aff54f17..402a1461c5 100644 --- a/space_smoke/__main__.py +++ b/space_smoke/__main__.py @@ -13,7 +13,7 @@ from space_smoke.registry import Scenario from space_smoke.runtime import SmokeConfigurationError, execute_scenario from space_smoke.scenarios import REGISTRY -from space_smoke.services import MissingToolError, SmokeServices +from space_smoke.services import MissingToolError, SmokeServices, repair_orphaned_kind_faults from space_smoke.sharding import select_shard from space_smoke.suite import ( RESULT_PREFIX, @@ -91,6 +91,13 @@ def _run(scenario_id: str) -> int: except SmokeConfigurationError as error: print(f"ERROR: {error}", file=sys.stderr) return 2 + # Outside the scenario's budget: the repair may wait out two rollouts. + try: + if repair_orphaned_kind_faults(os.environ): + print("REPAIRED: a limiter fault an earlier run left recorded", flush=True) + except Exception as error: # noqa: BLE001 - a fault left up makes any verdict untrustworthy + print(f"ERROR: {type(error).__name__}: {error}", file=sys.stderr) + return 2 result = execute_scenario( title=scenario.title, body=lambda context: scenario.run(context, services), diff --git a/space_smoke/adapters.py b/space_smoke/adapters.py index e2b865ab69..e38563c370 100644 --- a/space_smoke/adapters.py +++ b/space_smoke/adapters.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import contextlib import hashlib import importlib import ipaddress @@ -217,6 +218,68 @@ def _lease_document(output: str) -> dict[str, object]: return document +def _lease_annotations(document: Mapping[str, object]) -> Mapping[str, object]: + metadata = document.get("metadata") + annotations = metadata.get("annotations") if isinstance(metadata, dict) else None + return annotations if isinstance(annotations, dict) else {} + + +def _require_limiter_env_keys(values: Mapping[str, object]) -> None: + from space_smoke.rate_limiters import LIMITER_ENV_KEYS + + unknown = sorted(set(values) - set(LIMITER_ENV_KEYS)) + if unknown: + raise ValueError(f"refusing non-limiter public API env keys: {unknown}") + + +def _recorded_limiter_env(recorded: str) -> dict[str, str | None]: + """The standing limiter env a Lease recorded, refused unless every entry is restorable.""" + + try: + standing = json.loads(recorded) + except json.JSONDecodeError as error: + raise SmokeConfigurationError("the recorded limiter env is not JSON") from error + if not isinstance(standing, dict) or not all( + value is None or (isinstance(value, str) and (not value or _LIMITER_ENV_VALUE.fullmatch(value))) + for value in standing.values() + ): + raise SmokeConfigurationError("the recorded limiter env holds a value a restore cannot write") + _require_limiter_env_keys(standing) + return standing + + +def _holder_process_gone(holder: object) -> bool: + """True only for a holder that was a process of this user on this host and has exited. + + A smoke killed mid-fault leaves its Lease live for the full duration; a + successor that can prove the holder dead need not wait that out. Any doubt + (another host or user, a malformed identity, a live or reused pid) is + treated as alive. + """ + + if not isinstance(holder, str): + return False + fields = dict(part.partition("=")[::2] for part in holder.split(";")) + pid = fields.get("pid", "") + if ( + fields.get("uid") != str(os.getuid()) + or fields.get("host") != socket.gethostname() + or not pid.isdigit() + ): + return False + try: + os.kill(int(pid), 0) + except ProcessLookupError: + return True + except OSError: + return False + return False + + +def _json_pointer_token(key: str) -> str: + return key.replace("~", "~0").replace("/", "~1") + + def _lease_parts( document: Mapping[str, object], ) -> tuple[dict[str, object], dict[str, object]]: @@ -329,9 +392,20 @@ def _sql_identifier_ish(value: str) -> str: BUILDER_ROUTING_LABEL = "space.pplx.net/routing-smoke" OCI_BUILD_CACHE_DIR = "/space/cache/firecracker/builds/oci" CONTAINERD_BUILDS_NAMESPACE = "space-builds" -# The worker-cordon Lease records which node its holder cordoned, so a takeover -# of an expired Lease can uncordon it. Nothing in SPACE itself ever uncordons. +# The worker-cordon Lease records which node its holder cordoned, so the next +# holder can uncordon it if that holder never did. Nothing in SPACE itself ever +# uncordons. CORDONED_NODE_ANNOTATION = "space.pplx.net/smoke-cordoned-node" +# The public-API mutation Lease records what the rate-limiter scenario is about +# to change before it changes it: the limiter env the public API Deployment +# held (JSON, null for unset) and the Redis replica count it scales down from. +# A record is cleared only once its restore succeeded. The next holder of the +# Lease puts back whatever is still recorded, and every Kind scenario run first +# makes sure nothing is (`repair_orphaned_limiter_faults`), so a run killed or +# failing mid-fault does not leave later scenarios on smoke-sized caps or +# without the limiters' store. +LIMITER_ENV_ANNOTATION = "space.pplx.net/smoke-limiter-env" +REDIS_REPLICAS_ANNOTATION = "space.pplx.net/smoke-redis-replicas" DRAIN_KEEP_ANNOTATION = "space.pplx.net/drain-keep" NODE_GROUP_BOUNDARY_LABEL = "node-restriction.kubernetes.io/space-node-group" NODE_GROUP_GATE_ENV = "SPACE_SANDBOX_NODE_GROUP_PLACEMENT" @@ -668,6 +742,7 @@ def __init__( self.environment = dict(environment or os.environ) self._retention_lease_ownership: _LeaseOwnership | None = None self._cordon_lease_ownership: _LeaseOwnership | None = None + self._public_api_lease_ownership: _LeaseOwnership | None = None # The public API autoscaler's bounds before `scale_public_api` pinned # them: (name, uid, minReplicas, maxReplicas). self._public_api_autoscaler_bounds: tuple[str, str, int, int] | None = None @@ -1058,19 +1133,27 @@ def _acquire_named_mutation_lease( name: str, subject: str, *, - on_expired_takeover: Callable[[Mapping[str, object], _LeaseOwnership], None] - | None = None, + repair: Callable[[Mapping[str, object], _LeaseOwnership], None] | None = None, ) -> _LeaseOwnership: """Create or take over `name`; refuse while another live holder exists. - `on_expired_takeover` runs with the dead holder's document and the new - ownership only AFTER the resourceVersion-guarded patch has succeeded. - Two contenders can both read the same expired Lease; running repairs - before the patch would let the loser undo a fault the winner has since - started. The callback must fence its own writes on `ownership`. + `repair` runs with the previous document and the new ownership on every + acquisition of an existing Lease, released or expired, and only AFTER + the resourceVersion-guarded patch has succeeded. A holder whose cleanup + failed releases with its records still on the Lease, so a released + Lease is no proof that nothing is left to undo. Two contenders can both + read the same Lease; running repairs before the patch would let the + loser undo a fault the winner has since started. The callback must + fence its own writes on `ownership`. A Lease with a repair may also be + taken over from a holder `_holder_process_gone` proves dead, since the + repair undoes what that holder left. If the repair fails, the Lease is + released with its records for the next holder to retry. """ - holder = f"uid={os.getuid()};pid={os.getpid()};token={secrets.token_hex(16)}" + holder = ( + f"uid={os.getuid()};pid={os.getpid()};host={socket.gethostname()};" + f"token={secrets.token_hex(16)}" + ) acquired_at = datetime.now(UTC) spec = { "holderIdentity": holder, @@ -1119,22 +1202,25 @@ def _acquire_named_mutation_lease( ) if acquired_at < _parse_kube_timestamp(renewed) + timedelta( seconds=duration - ): + ) and not (repair is not None and _holder_process_gone(current_holder)): raise SmokeConfigurationError( f"another smoke holds the {subject} Lease on this kind " f"cluster (owner {current_holder})" ) previous = lease lease = self._replace_lease_spec(name, metadata, spec, subject=subject) - took_over = bool(current_holder) and on_expired_takeover is not None metadata, _ = _lease_parts(lease) ownership = _LeaseOwnership( name=name, uid=metadata["uid"], holder=holder, subject=subject ) - if observed.returncode == 0 and took_over: - assert on_expired_takeover is not None - on_expired_takeover(previous, ownership) + if observed.returncode == 0 and repair is not None: + try: + repair(previous, ownership) + except BaseException: + with contextlib.suppress(Exception): + self._release_lease(ownership) + raise return ownership def _release_lease(self, ownership: _LeaseOwnership) -> None: @@ -1158,7 +1244,9 @@ def _release_lease(self, ownership: _LeaseOwnership) -> None: ownership.name, metadata, released, subject=ownership.subject ) - def _assert_lease_owned(self, ownership: _LeaseOwnership, action: str) -> None: + def _assert_lease_owned( + self, ownership: _LeaseOwnership, action: str + ) -> tuple[dict[str, object], dict[str, object]]: current = self.kubectl("get", "lease", ownership.name, "-o", "json") if current.returncode != 0: raise SmokeConfigurationError( @@ -1182,6 +1270,59 @@ def _assert_lease_owned(self, ownership: _LeaseOwnership, action: str) -> None: f"refusing {action} without the caller's live mutation Lease " f"({ownership.subject})" ) + return metadata, spec + + def _renew_lease( + self, + ownership: _LeaseOwnership, + action: str, + annotations: Mapping[str, str | None] | None = None, + ) -> None: + """Renew our live Lease and set or clear `annotations` in one guarded patch. + + Callers renew right before each write a successor could own. A runner + stopped past its Lease's expiry may resume after a successor took over + and repaired its records; the patch tests uid, resourceVersion and + holder, so that runner fails here instead of undoing the successor's + fault or erasing its records, and a renewed Lease cannot expire under + the write that follows. + """ + + metadata, _ = self._assert_lease_owned(ownership, action) + existing = metadata.get("annotations") + existing = existing if isinstance(existing, dict) else None + patch: list[dict[str, object]] = [ + {"op": "test", "path": "/metadata/uid", "value": metadata["uid"]}, + { + "op": "test", + "path": "/metadata/resourceVersion", + "value": metadata["resourceVersion"], + }, + {"op": "test", "path": "/spec/holderIdentity", "value": ownership.holder}, + { + "op": "add", + "path": "/spec/renewTime", + "value": _kube_timestamp(datetime.now(UTC)), + }, + ] + for key, value in (annotations or {}).items(): + path = f"/metadata/annotations/{_json_pointer_token(key)}" + if value is None: + if existing is not None and key in existing: + patch.append({"op": "remove", "path": path}) + continue + if existing is None: + patch.append({"op": "add", "path": "/metadata/annotations", "value": {}}) + existing = {} + patch.append({"op": "add", "path": path, "value": value}) + result = self.kubectl( + "patch", "lease", ownership.name, "--type=json", "--patch", json.dumps(patch) + ) + if result.returncode != 0: + raise SmokeConfigurationError( + f"{ownership.subject} Lease changed under {action}; refusing to write: " + f"{result.detail() or ''}" + ) def _annotate_lease(self, name: str, key: str, value: str | None) -> None: assignment = f"{key}-" if value is None else f"{key}={value}" @@ -1494,11 +1635,11 @@ def acquire_worker_cordon_mutation(self) -> Callable[[], None]: def repair_orphaned_cordon( previous: Mapping[str, object], ownership: _LeaseOwnership ) -> None: - # The dead holder recorded the node it cordoned; nothing in SPACE - # ever uncordons, so undo it. This runs only after we own the - # Lease, and re-checks that ownership right before the write, so a - # contender that lost the takeover can never uncordon the winner's - # fault. + # A previous holder that died, or whose uncordon failed, left the + # node it cordoned recorded; nothing in SPACE ever uncordons, so + # undo it. This runs only after we own the Lease, and re-checks + # that ownership right before the write, so a contender that lost + # the takeover can never uncordon the winner's fault. metadata = previous.get("metadata") annotations = ( metadata.get("annotations") if isinstance(metadata, dict) else None @@ -1517,7 +1658,7 @@ def repair_orphaned_cordon( self._annotate_lease(name, CORDONED_NODE_ANNOTATION, None) ownership = self._acquire_named_mutation_lease( - name, "worker cordon mutation", on_expired_takeover=repair_orphaned_cordon + name, "worker cordon mutation", repair=repair_orphaned_cordon ) self._cordon_lease_ownership = ownership @@ -3790,12 +3931,106 @@ def delete_template_as_admin(self, alias: str) -> str | None: # --- rate limiters ----------------------------------------------------- - def acquire_public_api_mutation(self) -> Callable[[], None]: + def _public_api_lease_name(self) -> str: context_hash = hashlib.sha256(self.config.context.encode()).hexdigest()[:16] + return f"space-smoke-public-api-{context_hash}" + + def acquire_public_api_mutation(self) -> Callable[[], None]: ownership = self._acquire_named_mutation_lease( - f"space-smoke-public-api-{context_hash}", "public API deployment mutation" + self._public_api_lease_name(), + "public API deployment mutation", + repair=self._repair_recorded_limiter_faults, ) - return lambda: self._release_lease(ownership) + self._public_api_lease_ownership = ownership + + def release() -> None: + self._release_lease(ownership) + if self._public_api_lease_ownership is ownership: + self._public_api_lease_ownership = None + + return release + + def _repair_recorded_limiter_faults( + self, previous: Mapping[str, object], ownership: _LeaseOwnership + ) -> None: + # A previous holder recorded what it changed before changing it, and + # clears a record only after the restore succeeded. Every write here + # renews the Lease first, so a contender that lost the acquisition + # never undoes the winner's fault. + annotations = _lease_annotations(previous) + recorded_replicas = annotations.get(REDIS_REPLICAS_ANNOTATION) + if isinstance(recorded_replicas, str) and recorded_replicas: + if not recorded_replicas.isdigit(): + raise SmokeConfigurationError("the recorded Redis replica count is not a count") + self._renew_lease(ownership, "an orphaned Redis scale-down repair") + self._scale_redis_deployment(max(int(recorded_replicas), 1)) + self.wait_redis_rollout(timeout=120) + self._renew_lease( + ownership, "clearing the Redis record", {REDIS_REPLICAS_ANNOTATION: None} + ) + recorded_env = annotations.get(LIMITER_ENV_ANNOTATION) + if isinstance(recorded_env, str) and recorded_env: + standing = _recorded_limiter_env(recorded_env) + self._renew_lease(ownership, "an orphaned limiter env repair") + self._apply_public_api_env( + standing, "restore the limiter env a previous smoke Lease holder left" + ) + self._renew_lease( + ownership, "clearing the limiter env record", {LIMITER_ENV_ANNOTATION: None} + ) + + def repair_orphaned_limiter_faults(self) -> bool: + """Undo a limiter fault an earlier run left recorded, or refuse to proceed. + + Every Kind scenario runs this first: a run killed while Redis was down + or the caps were shrunk leaves its records on the public-API mutation + Lease, and only the three scenarios that take that Lease would repair + them. One Lease read when nothing is recorded. A recorded fault is + repaired by acquiring the Lease, which is possible once it is released, + expired or held by a holder proven dead; a live holder is a smoke + mid-fault, and the caller must not run against it. True if a repair ran. + """ + + contexts = self.executor.run(["kubectl", "config", "get-contexts", "-o", "name"], timeout=15) + if contexts.returncode == 0 and self.config.context not in contexts.stdout.split(): + # No cluster yet: the run bootstraps a fresh one, which has no faults. + return False + observed = self.kubectl("get", "lease", self._public_api_lease_name(), "-o", "json", timeout=15) + if observed.returncode != 0: + if "notfound" in observed.detail().lower(): + return False + raise SmokeConfigurationError( + "could not inspect the public API mutation Lease for an orphaned limiter fault: " + f"{observed.detail() or ''}" + ) + document = _lease_document(observed.stdout) + annotations = _lease_annotations(document) + if not any(annotations.get(key) for key in (LIMITER_ENV_ANNOTATION, REDIS_REPLICAS_ANNOTATION)): + return False + try: + release = self.acquire_public_api_mutation() + except SmokeConfigurationError as error: + _, spec = _lease_parts(document) + raise SmokeConfigurationError( + "refusing to run while the public API mutation Lease records a limiter fault " + f"(holder {spec.get('holderIdentity') or ''}): {error}" + ) from error + release() + return True + + def _held_public_api_lease(self, action: str) -> _LeaseOwnership: + ownership = self._public_api_lease_ownership + if ownership is None: + raise SmokeConfigurationError( + f"refusing {action} without the caller's live mutation Lease" + ) + return ownership + + def _lease_annotation(self, ownership: _LeaseOwnership, key: str) -> str | None: + current = self.kubectl("get", "lease", ownership.name, "-o", "json") + _require_ok(current, f"read the {ownership.subject} Lease") + value = _lease_annotations(_lease_document(current.stdout)).get(key) + return value if isinstance(value, str) and value else None def _public_api_container(self, name: str) -> str: container = _require_ok( @@ -3819,11 +4054,7 @@ def public_api_env(self, keys: Sequence[str]) -> dict[str, str | None]: return self._deployment_env_literals(self._public_api_deployment(), keys) def _apply_public_api_env(self, values: Mapping[str, str | None], action: str) -> None: - from space_smoke.rate_limiters import LIMITER_ENV_KEYS - - unknown = sorted(set(values) - set(LIMITER_ENV_KEYS)) - if unknown: - raise ValueError(f"refusing non-limiter public API env keys: {unknown}") + _require_limiter_env_keys(values) self._require_isolated_kind("public API env override") assignments = [ f"{key}-" if value is None else f"{key}={value}" for key, value in values.items() @@ -3839,13 +4070,40 @@ def _apply_public_api_env(self, values: Mapping[str, str | None], action: str) - self.wait_public_api_rollout(timeout=240) def set_public_api_env(self, overrides: Mapping[str, str]) -> None: + from space_smoke.rate_limiters import LIMITER_ENV_KEYS + for key, value in overrides.items(): if not _LIMITER_ENV_VALUE.fullmatch(value): raise ValueError(f"refusing limiter env value {value!r} for {key}") + _require_limiter_env_keys(overrides) + action = "a public API limiter override" + ownership = self._held_public_api_lease(action) + # Record first, and only the first time: a run that dies between here + # and the write leaves a record of values still standing, which the + # next holder restores to no effect, and a second override in the same + # run must not record the first one's values as standing. + record: dict[str, str | None] = {} + if self._lease_annotation(ownership, LIMITER_ENV_ANNOTATION) is None: + standing = self.public_api_env(LIMITER_ENV_KEYS) + record[LIMITER_ENV_ANNOTATION] = json.dumps(standing, sort_keys=True) + self._renew_lease(ownership, action, record) self._apply_public_api_env(overrides, "set the public API limiter env") def restore_public_api_env(self, standing: Mapping[str, str | None]) -> None: + """Put the standing env back, then clear its record; both only while the Lease is ours. + + A runner that lost its Lease refuses: the successor that took it over + has already restored the record, and may have a fault of its own up. + A failed restore keeps the record for the next holder. + """ + + action = "restoring the public API limiter env" + ownership = self._held_public_api_lease(action) + self._renew_lease(ownership, action) self._apply_public_api_env(standing, "restore the public API limiter env") + self._renew_lease( + ownership, "clearing the limiter env record", {LIMITER_ENV_ANNOTATION: None} + ) def wait_public_api_ready(self, *, timeout: float) -> None: """`/readyz`, not `/healthz`: readiness gates the Service on the reflector's initial LIST.""" @@ -3873,22 +4131,42 @@ def redis_replicas(self) -> int: return int(previous) if previous.isdigit() else 1 def scale_redis(self, replicas: int) -> int: + """Scale the limiters' store, only while the public-API Lease is ours. + + Every Kind API, admin API and cluster-controller replica runs its + limiters against this store, so scaling it to zero is recorded on the + Lease first, where the next holder scales it back if this run does + not. A runner that lost its Lease refuses either way, so it cannot + undo or erase a successor's fault. + """ + self._require_isolated_kind("Redis scaling") if replicas < 0 or replicas > 1: raise ValueError(f"refusing to scale Redis to {replicas} replicas") - previous = _require_ok( - self.kubectl( - "get", "deploy", KIND_REDIS_DEPLOYMENT, "-o", "jsonpath={.spec.replicas}", timeout=15 - ), - "read Redis replicas", - ).strip() + action = f"scaling Redis to {replicas}" + ownership = self._held_public_api_lease(action) + previous = self.redis_replicas() + if replicas == 0: + record: dict[str, str | None] = {} + if self._lease_annotation(ownership, REDIS_REPLICAS_ANNOTATION) is None: + record[REDIS_REPLICAS_ANNOTATION] = str(previous) + self._renew_lease(ownership, action, record) + self._scale_redis_deployment(0) + else: + self._renew_lease(ownership, action) + self._scale_redis_deployment(replicas) + self._renew_lease( + ownership, "clearing the Redis record", {REDIS_REPLICAS_ANNOTATION: None} + ) + return previous + + def _scale_redis_deployment(self, replicas: int) -> None: _require_ok( self.kubectl( "scale", f"deploy/{KIND_REDIS_DEPLOYMENT}", f"--replicas={replicas}", timeout=30 ), f"scale Redis to {replicas}", ) - return int(previous) if previous.isdigit() else 1 def wait_redis_unreachable(self, *, timeout: float) -> None: """Pod gone, endpoints empty, then a settle so the API's pooled connection dies.""" diff --git a/space_smoke/rate_limiters.py b/space_smoke/rate_limiters.py index b12d01cc71..58758a6718 100644 --- a/space_smoke/rate_limiters.py +++ b/space_smoke/rate_limiters.py @@ -1,16 +1,24 @@ """The two Redis-backed API rate limiters, proven end to end on a real cluster. -`smoke-kind-rate-limiters.sh` pointed the kind `space-api` at Redis, enabled -both limiters with low caps in enforce mode, and drove real HTTP traffic at -them: sequential creates until the cluster activation token bucket answered +Kind runs both limiters against its Redis by default, as deployments do: the +charts render `SPACE_REDIS_URL` on the API, admin API and cluster-controller, +with the activation limiter at the binary defaults and per-client concurrency +in `observe`. This scenario shrinks the caps and switches per-client +concurrency to enforce on the public API, then drives real HTTP traffic at +them: sequential creates until the cluster activation token bucket answers 429 `Server rate limit:`, concurrent creates and connects for one -`X-Client-ID` until the per-client concurrency limiter answered 429 -`Per-client rate limit:`, then Redis scaled to zero to show both limiters -admit when their store is unreachable. This module is that suite in typed -form: the Deployment env override is a named, captured-and-restored, kind-only -operation behind one cluster Lease; the Redis fault has its compensation -armed before dispatch; every sandbox the probes admit carries run metadata -the teardown kills by. The control surface is [`RateLimiterControl`]; the raw +`X-Client-ID` until the per-client concurrency limiter answers 429 +`Per-client rate limit:`, Redis scaled to zero to show both limiters admit +when their store is unreachable, and Redis back to show they enforce again. + +The Deployment env override is a named, captured-and-restored, kind-only +operation behind one cluster Lease, which records the standing env and the +Redis replica count before either changes and clears each record only once +its restore succeeded. If this run is killed or a restore fails, the next +Kind scenario puts back what is recorded before it starts (see +`KindHarness.repair_orphaned_limiter_faults`). The Redis fault has its +compensation armed before dispatch; every sandbox the probes admit carries +run metadata the teardown kills by. The control surface is [`RateLimiterControl`]; the raw requests go through [`RateLimitProbe`] so a 429's `Retry-After` survives. """ @@ -37,9 +45,10 @@ PER_CLIENT_LIMIT_MESSAGE = "Per-client rate limit:" # The env this scenario may write on the public API Deployment, and nothing -# else. `enable` refuses anything outside it before kubectl runs. +# else. `enable` refuses anything outside it before kubectl runs. The store's +# URL is not among them: the chart renders it, and the fail-open half takes +# the store itself away instead. LIMITER_ENV_KEYS: tuple[str, ...] = ( - "SPACE_REDIS_URL", "SPACE_CLUSTER_ACTIVATION_ADMISSION_ENABLED", "SPACE_CLUSTER_ACTIVATION_ADMISSION_BURST", "SPACE_CLUSTER_ACTIVATION_ADMISSION_REFILL_RPS", @@ -47,7 +56,7 @@ "SPACE_API_CLIENT_CONCURRENCY_MAX_PER_CLIENT", "SPACE_API_CLIENT_CONCURRENCY_KNOWN_CLIENTS", ) -KIND_REDIS_URL = "redis://space-redis:6379" +REDIS_URL_ENV = "SPACE_REDIS_URL" def is_limiter_rejection(outcome: LimiterProbeOutcome, message: str) -> bool: @@ -118,12 +127,13 @@ def enable_limiters( cluster_refill_rps: str = "0.1", rollout_timeout: float = 240, ) -> None: - """Point the API at Redis and turn both limiters on in enforce mode. + """Shrink both limiters' caps and put per-client concurrency in enforce mode. The standing values are captured first and restored at scope end (or eagerly by `restore_limiters`), so a chart or operator value comes back rather than being deleted. The Lease serializes cooperating - smokes on the same Deployment. + smokes on the same Deployment and records the standing values for a + successor if this run dies. """ if not (1 <= per_client_cap <= 8 and 1 <= cluster_burst <= 8): @@ -132,6 +142,12 @@ def enable_limiters( self._template = template release = self._control.acquire_public_api_mutation() self._release_lease = self._scope.defer("release the public API mutation lease", release) + store = self._control.public_api_env((REDIS_URL_ENV,))[REDIS_URL_ENV] + self._fixture.verify( + bool(store), + f"the public API has no {REDIS_URL_ENV}; both limiters would fail open, not enforce", + ) + self._fixture.record("limiter_store", store) standing = self._control.public_api_env(LIMITER_ENV_KEYS) self._fixture.record("limiter_env_before", dict(standing)) @@ -142,9 +158,17 @@ def restore() -> None: self._restore_env = self._scope.defer( "restore the public API limiter env", restore, critical=True ) + # Only the public API's copy of the activation caps shrinks. The + # cluster-controller's `NativePublicOperationExecutor` consumes the + # same bucket at the chart's defaults, and the token-bucket script + # refills by each caller's own burst and rate, so a native activation + # during the drain would top the bucket up. None runs here: this + # scenario sends only ECW requests, which the API admits itself, and + # the suite runs one scenario at a time. Overriding the controller + # too would add a second Deployment rollout and a second record to + # restore for a race the suite cannot produce. self._control.set_public_api_env( { - "SPACE_REDIS_URL": KIND_REDIS_URL, "SPACE_CLUSTER_ACTIVATION_ADMISSION_ENABLED": "true", "SPACE_CLUSTER_ACTIVATION_ADMISSION_BURST": str(cluster_burst), "SPACE_CLUSTER_ACTIVATION_ADMISSION_REFILL_RPS": cluster_refill_rps, @@ -329,6 +353,30 @@ def bring_redis_back(self) -> None: self._redis_back = None release.release() + def expect_enforcing_again( + self, sandbox_id: str, *, callers: int = 20, budget: float = 60 + ) -> None: + """Once the store is back, a connect burst draws a per-client 429 again. + + Connects rather than creates: the slot is reserved before the sandbox + is fetched, so the verdict costs no VM boots. The API's pooled + connections died with the old pod; the pool replaces them on use, so a + first burst may still fail open. Bursts repeat until one is limited or + the budget runs out, and the number it took is recorded. + """ + + deadline = self.now() + budget + bursts = 0 + while True: + bursts += 1 + outcomes = self.burst_connects(sandbox_id, callers) + limited = any(is_limiter_rejection(o, PER_CLIENT_LIMIT_MESSAGE) for o in outcomes) + if limited or self.now() >= deadline: + break + time.sleep(2) + self._fixture.record("recovery_bursts", bursts) + self.expect_per_client_limited(outcomes, label="per_client_after_recovery") + def expect_fail_open(self, outcomes: list[LimiterProbeOutcome]) -> None: """With the store down neither limiter may reject; other statuses are not judged.""" diff --git a/space_smoke/scenarios/local_rate_limiters_enforce_and_fail_open.py b/space_smoke/scenarios/local_rate_limiters_enforce_and_fail_open.py index 9c2866bdb1..44936a9412 100644 --- a/space_smoke/scenarios/local_rate_limiters_enforce_and_fail_open.py +++ b/space_smoke/scenarios/local_rate_limiters_enforce_and_fail_open.py @@ -1,4 +1,4 @@ -"""Both API rate limiters reject past their caps and admit when Redis is gone.""" +"""Both API rate limiters reject past their caps, admit when Redis is gone, and recover.""" import space_smoke as smoke @@ -6,12 +6,14 @@ FAIL_OPEN_CALLERS = 4 -# The shell suite this replaces reconfigured `deploy/space-api` with `kubectl -# set env` and reverted it in a trap. Here the override is a captured, -# Lease-guarded, kind-only operation whose restore is armed before the first -# write, the Redis fault has its scale-back armed before dispatch, and every -# sandbox the probes admit carries run metadata the teardown kills by. Caps -# are low because each admitted create boots a real Firecracker VM. +# The limiters already run against the chart-rendered Redis. The cap override +# is a captured, Lease-guarded, kind-only operation whose restore is armed +# before the first write and recorded on the Lease, where the next scenario +# finds it if this run never restores; the Redis fault is recorded the same +# way and has its scale-back armed before dispatch; every sandbox the probes +# admit carries run metadata the teardown kills by. +# Caps are low because each admitted create boots a real Firecracker VM: at +# the default burst the drain would need sixty. @smoke.scenario( "local.rate_limiters_enforce_and_fail_open", # Two API rollouts (enable, restore), a Redis scale-down/up, and about ten @@ -19,15 +21,17 @@ sandbox=smoke.SandboxConfig(timeout=1800), ) def rate_limiters_enforce_and_fail_open(test: smoke.KindEcw) -> None: - """With both limiters enabled in enforce mode against Redis, sequential - creates drain the cluster activation bucket to a 429 `Server rate limit:` - with Retry-After; twenty concurrent creates and twenty concurrent connects - for one client id each draw at least one 429 `Per-client rate limit:` with - Retry-After; and once Redis is unreachable neither limiter rejects.""" + """With both limiters' caps shrunk and per-client concurrency in enforce + mode, sequential creates drain the cluster activation bucket to a 429 + `Server rate limit:` with Retry-After; twenty concurrent creates and twenty + concurrent connects for one client id each draw at least one 429 + `Per-client rate limit:` with Retry-After; once Redis is unreachable + neither limiter rejects; and once it is back the per-client limiter + rejects again.""" limiters = test.rate_limiters - with test.phase("enable both limiters on the public API, low caps, enforce"): + with test.phase("shrink both limiters' caps on the public API, enforce per-client"): limiters.enable_limiters(template=test.sandbox_template_name()) with test.phase("preflight: one create is admitted or rate-limited"): @@ -57,5 +61,8 @@ def rate_limiters_enforce_and_fail_open(test: smoke.KindEcw) -> None: limiters.expect_fail_open(outcomes) limiters.bring_redis_back() + with test.phase("recovery: Redis back, the per-client limiter rejects again"): + limiters.expect_enforcing_again(target) + with test.phase("restore the public API limiter env"): limiters.restore_limiters() diff --git a/space_smoke/services.py b/space_smoke/services.py index 752c478c56..400fcc31e6 100644 --- a/space_smoke/services.py +++ b/space_smoke/services.py @@ -589,6 +589,27 @@ def from_environment( ) +def repair_orphaned_kind_faults(environment: Mapping[str, str]) -> bool: + """On the kind backend, undo a limiter fault an earlier run left, or refuse. + + Runs before every scenario, whatever its fixture: a scenario that takes no + Lease would otherwise run against shrunk caps or a store scaled to zero. + """ + + values = dict(environment) + if values.get("SPACE_SMOKE_BACKEND", KIND_BACKEND) != KIND_BACKEND: + return False + harness = KindHarness( + SubprocessExecutor(base_env=values), + KindConfig( + cluster=values.get("KIND_CLUSTER", "space-local"), + api_url=values.get("API_URL", "http://127.0.0.1:8080"), + ), + environment=values, + ) + return harness.repair_orphaned_limiter_faults() + + @dataclass(frozen=True) class _NativeScenarioSandboxes: factory: CommandSandboxFactory