Skip to content

docs(gateway): pin the rationale behind gateway config and KV-compat behaviour - #1377

Closed
kvinwang wants to merge 4 commits into
nextfrom
docs/gateway-config-rationale
Closed

kvinwang wants to merge 4 commits into
nextfrom
docs/gateway-config-rationale

Conversation

@kvinwang

Copy link
Copy Markdown
Collaborator

Split out of #1262.

Audit findings worked to a conclusion and refuted; the evidence that decides them is in the diff as tests and doc comments rather than nothing, so what was checked stays checked. No behaviour change: every hunk is a comment, a gateway.toml comment, or a test.

data_timeout_enabled = false removes the idle window, not a per-operation timer

The shipped comment read like a relaxation ("might impact performance"). ProxyConfig::idle_timeout() returns None when it is off: the buffered bridge falls back to copy_bidirectional and the spliced and kTLS relays drop their watchdog, so timeouts.total (5h) is the only bound left on a connection that transfers nothing. The comment in gateway.toml and on Timeouts::data_timeout_enabled now says so; disabling_data_timeouts_removes_the_idle_window_entirely pins it.

TLS 1.3 being off by default is deliberate, and the ticketer is not dead machinery

a5230e601e ("install rustls session ticketer for TLS 1.3 resumption") measured both arms on a 4-core gateway and states the numbers: TLS 1.3 reconnect CPS 6.2k → 9.6k with the ticketer, TLS 1.2 ~20k — 1.2 is more than twice the connection rate because it resumes from rustls' session-ID cache, which 1.3 cannot use. That commit also says "Default TLS version is unchanged", so the 1.2-only default was knowingly carried. And rustls' 1.2 is already ECDHE-only, AEAD-only, no renegotiation, no compression — what 1.3 adds here is handshake privacy and latency, not data confidentiality. The ticketer exists for operators who set tls_versions = ["1.2","1.3"].

No behaviour change: flipping the default would alter every deployment's negotiated version and its untested interaction with enable_secret_extraction / kTLS. The measurement and the trade-off are now recorded at tls_versions in gateway.toml and at the ticketer.

Config::uuid() cannot regenerate identity on an unwritable data dir

It is unreachable. ProxyInner::new calls KvStore::new(..., &config.sync.data_dir, ...) before config.uuid(), and KvStore::new routes any io error that is not UnexpectedEof/InvalidData through is_storage_failure into a hard Err("cannot open the WaveKV data dir …; refusing to start"). PermissionDenied is one of those, so the gateway exits before a uuid is ever generated.

Downstream would not accumulate ghost peers even if it did: the node id is config, so node/info/<id> is overwritten rather than appended, and wavekv's check_uuid applies to inbound requests only — the peer's next outbound sync carries the fresh record and the pair reconverges, which sync.rs:495-509 documents as the deliberate recovery path.

No code change; an_unwritable_data_dir_fails_the_boot_before_a_uuid_is_generated plus a doc comment pin the ordering invariant, so this stays safe by construction rather than by luck.

carry_unknown_fields does not grow forever

Growth is a fixed cost, not a compounding one, and the module already ruled on it: "A field this binary stops declaring is carried forever… Retiring one for real needs an explicit list of names to drop on write; until something needs to be retired, that list would have no entries to hold."

Mechanically, carried = stored_fields \ declared(T) \ encoded_fields, copied from the stored record on each write — so successive rewrites are a fixed point. A name can only enter the set by being declared by some gateway build: local writes use this binary's field list, and remote records arrive only over mTLS where authorize_peer requires the peer certificate's app-id to equal this gateway's own. The bound is the union of declared field names across gateway builds in the cluster, and no KV value type has ever retired a field, so that set is empty today.

No mechanism added — CODING_TASTE says bring the numbers before adding a limit. rewriting_a_record_forever_does_not_grow_it (50 alternating old/new rewrites, field count constant) turns "grows forever" into a pinned "does not".

The certbot backup directory is not an unbounded growth problem

Three things, none of which the finding assumed. The gateway never writes one: store_cert / new_cert_dir are reachable only from CertBot::renew_inner, and grep over gateway/ finds no auto_renew / create_cert_if_needed / store_cert — DistributedCertBot stores certificates in WaveKV, and the gateway CVM ships no certbot binary. The rate is ~4.5 directories a year: renew_days_before defaults to 10 against 90-day Let's Encrypt certs and renew_inner only calls store_cert on an actual issuance — ~6 KB each, ~27 KB/year, from the standalone CLI only. And "list_cert_public_keys reads all of them on every call" describes no call: rg across the whole tree finds zero callers outside the definitions. The archive is also the documented rollback path (live_links_can_roll_back_to_a_complete_generation).

No change. Separate observation, not acted on: WorkDir::list_certs / list_cert_public_keys and CertBot::list_certs / list_cert_public_keys are dead public API, a "delete, don't accumulate" candidate for its own PR.

Verification

cargo test -j 16 -p dstack-gateway -p certbot: 379 passed, 0 failed
cargo clippy -p dstack-gateway -p certbot --all-features -- -D warnings --allow unused_variables: clean
cargo fmt --all -- --check: clean

@kvinwang

Copy link
Copy Markdown
Collaborator Author

Closing: every hunk documents a finding the audit already refuted, and the tests restate the implementation; no behaviour change worth carrying.

@kvinwang kvinwang closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant