Conversation
Two gaps that both follow from not relaying `Host`. The upstream had no way to learn what the client asked for. It now gets `X-Forwarded-Host` and `X-Forwarded-Proto`, neither overwriting a value that arrived, so Doppel behind another proxy keeps the authority the client really used rather than substituting an internal one -- the same treatment `X-Forwarded-For` already had. The proto is `http`: Doppel terminates no TLS, and `https` would name a hop that does not exist. A redirect was relayed with the upstream's own authority in `Location`, so a client following it left Doppel from that point on -- past every injected fault and every mock, with nothing logged and nothing failing. The test keeps passing and stops testing anything. nginx has `proxy_redirect` for this and Apache `ProxyPassReverse`, both on by default, which is why this defaults to on too. `Location` is resolved against the URL actually requested upstream and, if it lands under the proxy's base, returned as a root-relative path -- so Doppel never has to guess its own public name. Measured end to end: a target under the base becomes `/orders/7?page=2` and curl follows it back into Doppel; one above the base becomes absolute. That second case fixes a quieter bug, where `Location: /login` under a base of `/v2/` had the client come back asking for `/v2/login`. `rewrite_redirects: false` relays byte for byte, for a client under test for its redirect handling rather than against a degraded backend. Both pinned by mutation: making the helper a no-op, overwriting an incoming value, ignoring the setting and never rewriting each fail a different test. The conformance fixture carries `rewrite_redirects: false` so both stores round-trip the new column -- dropping it on load fails the revision check. The migrate test read `schema version 1` from a literal, so the first migration ever added broke it; it now reads the newest version from the embedded migrations.
The default had been flipped to `false` and the whole suite stayed green:
every test builds a `CompiledProxy` literal with the field set explicitly,
so nothing went through `compile_proxy` to notice. Two tests now compile a
configuration that says nothing about it and assert `true`, and one that
says `false` and asserts it carries through -- so the first cannot pass by
the field being hardcoded either. Verified by flipping the default back:
both fail.
The documentation pass that found it also turned up three things:
Mocks had no reference table. Every other part of the configuration has
one; `mocks[]` was documented only by example, and `request` and `proxy`
appeared nowhere as fields with types and defaults. Added, and the YAML in
it was run through `config validate` and then served -- `GET
/pricing/42/?page=3` answers `{"id": "42", "page": "3"}` with the header
the table promises.
`overview/concepts.md` still said both faults "apply before Doppel decides
what will answer the request", which 0.2.0 made false: the mock is decided
first and the proxy's loss does not touch a mocked request.
`observability.md` described `latency_injected_ms` as "which faults fired".
It is the wait actually taken, which reads 0 when the upstream already
spent longer than the target while the counter still counts the roll --
the one case where the two disagree, and worth saying so.
`main.example.yaml` gains `rewrite_redirects` next to `replace`, matching
how the example spells out other defaults rather than leaving them implied.
The configuration had no machine-readable description, so every mistake waited for a `config validate` run. `doppel-config.schema.json` at the repository root is generated from the same `utoipa::ToSchema` derives the admin API's OpenAPI document uses -- no second description of the types to keep in step -- and `doppel config schema` prints it. Editors read it through a `yaml-language-server` modeline, which `main.example.yaml` now carries; the release attaches it as a signed asset, so a deployment can validate against the schema for exactly the version it runs. Every field gained a doc comment, because the schema is read as a tooltip and a field without one says nothing. A test enumerates them rather than sampling. `utoipa` buries the description of an `Option<T>` inside the `oneOf` branch beside the `$ref`; `hoist_descriptions` lifts it back to the property, which is where anything looks. `scripts/config_schema.py` regenerates the file and `--check` fails when it is stale. Both CI and a Rust drift test run that check, deliberately: one fails for whoever runs the suite, the other names the fix in a step title. There is a `regenerate-config-schema` skill, and `pre-release-check` now runs it. Verified against `jsonschema`: the schema is valid 2020-12, accepts `main.example.yaml`, and rejects `percentage: 45`, `method: get`, `port: 0`, an unknown key, a missing `url` and a removed `server` section. Three behaviour changes come with it. An empty or absent `proxies` list is legal. Rule V5 refused it, which meant a fresh deployment could not start until its proxies were written -- so the two ways to add one, reload and the admin API, were unreachable exactly when they were most useful. A request meanwhile is answered `503 NO_PROXIES_CONFIGURED`: the configuration is unfinished, the client did nothing wrong, and a `404` would send whoever is debugging the client into the client. Deleting the last proxy over the API is allowed for the same reason. `type: tcp` is refused while the document is parsed rather than by rule V7. `ProxyKind` lost the variant, so the runtime, the store and the schema no longer carry a case that could never be reached; a hand-written `Deserialize` keeps the message saying it is unimplemented rather than merely unaccepted. Names no longer admit `.`, and the caps are 64 in general and 32 for a proxy. Dropping the dot removed two more rules with it: a name is a directory component, so `.hidden` and `..` each needed refusing, and neither can now be written. A proxy name is capped tighter because it travels further -- a directory, a metric label, a log field, and a header value on every request. `Name` is generic over its cap so the two share one character set, one trait surface and one schema that reports whichever limit applies. V5 and V7 retire, leaving fourteen rules.
`access` could name any token or group that existed. `admin.groups` says which of them a configuration is allowed to reference, here and in a proxy's overrides, and rule V36 checks it. `["*"]` is the default and allows any; `["admin", "ci"]` allows exactly those; `[]` allows none. `public` is never governed by it -- it is the absence of a subject rather than a name, and a deployment locked down to `[]` still has to be able to say "anyone may read this". The default is permissive, which is the opposite of how `access` itself defaults. That is deliberate: `access` defaults to `admin` because the cost of being wrong is unauthenticated writes, and this defaults to `*` because the cost of being wrong is an operator unable to name their own groups. Running it caught an error in my own documentation. `groups: []` is stronger than "no custom groups": every action defaults to the `admin` group, so an empty list makes the defaults themselves violations and all six actions have to be spelled `public`. The docs said "or left at its default", which is exactly wrong. Corrected in three places -- the page, the field's doc comment and the example -- and the page now shows the six violations that result. V27 and V36 read the same places, so `access_sites` walks them once and both rules run per site. They were a loop each, which is how a rule added later covers the admin block and quietly forgets the per-proxy overrides. `groups` is an `Option`, skipped when absent, for the same reason `rewrite_redirects` is: the revision is derived from the canonical YAML, so materialising a default would have made every configuration stored before this field existed fail its own revision check on the first load after the upgrade. Migration 0003 adds a nullable column with no DDL default, and says so. Two schema fixes found by checking what it actually rejects rather than that it exists. `AllowedGroup` had no pattern, so `groups: ["not a name!"]` validated; it now composes one from the name rules, which are exposed as `name::CHARACTERS` and `name::MIN` so there is one spelling of the class. And `Name<64>` and `Name<32>` both reported as `Name`, the second silently overwriting the first, so the schema claimed a proxy name could be 64 characters -- `ToSchema::name` now gives each cap its own component. Checked with `jsonschema`: a 33-character proxy name is rejected and a 40-character token name is not.
`access_of` wrapped `&config.admin.access` and was the only reader of it
left once V27 and V36 moved onto `access_sites`; a function that says less
than the field access it replaces is a name to look up for nothing.
The `Name` tests had a local `type Default = Name<MAX>`, which shadows the
trait of the same name in that module -- a reader meeting `Default::parse`
has to work out which of the two it is. Renamed to `AnyName`.
Also verified the claim the configuration reference makes about the admin
API, rather than reasoning about it: with `groups: ["admin"]`, posting a
proxy whose `access` names `user` answers
HTTP 400 {"code": "CONFIG_INVALID", "message": "configuration is
invalid: proxies[1].access.read: `user` is not an allowed group:
`admin.groups` allows only `admin`"}
and the same proxy naming `admin` answers `201`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated back-merge of
mainintodevelop.Latest commit on main:
562e13e-- feat: forward the client's authority, and point redirects back at DoppelOpened by
Back-merge main -> develop, run31376660194.Merge this (or turn on auto-merge) to keep
developin step withmain. Resolve any conflicts here rather than on develop.