chore(decdn_node,charts): sync config-schema surface to decdn main @ d3bc7da7 - #49
Merged
Merged
Conversation
…d3bc7da7 Re-sync the role's and chart's node.toml key surface to the current `main` of decdn/decdn (was 0b94efe4, 32 commits back). Every config section is `#[serde(deny_unknown_fields)]` with no serde aliases, so a stale key is a startup crash-loop. Field delta (verified against crates/common/src/config/types.rs): - REMOVE payment.delivery_floor and blockchain.rate_bounds_poll_interval_sec. Upstream dropped the seller-side rate-floor clamp and the rate-bounds watcher: the node no longer bounds its own sell rate — a rate below the on-chain delivery floor still sells and settles, with only its vote-weight byte credit clamped at redemption. - observability.otlp_endpoint: OTLP span export is now always compiled in (the `otlp` cargo feature is gone), and the endpoint must be exactly `http://host:port` — https://, a missing port, a path/query/fragment and userinfo are all rejected at config resolution. No other types.rs change; the 421614 deployment manifest is unchanged since d306cc5c, so host_vars addresses and the manual install method stand. Touched: templates/node.toml.j2 (drop both emit blocks), defaults/main.yml, tasks/main.yml (drop both from the numeric asserts; tighten the otlp_endpoint shape assert to http://host:port, case-insensitive scheme), README.md, host_vars/decdn-node-1 economics comment, galaxy/CHANGELOG.md, the chart's ci/ci-values.yaml, and the molecule oracle: gen-schema-keys.py header bumped to d3bc7da7, regenerated schema-keys.txt (155 paths), schema+default converge and the default verify's delivery_floor check. Verified: make lint, lint-ansible, lint-helm (incl. DECDN_CLI=… so all three ci-values files pass a real `decdn config validate` at d3bc7da7), security, and all six molecule scenarios. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Endpoint validation issues and requested regression coverage remain unresolved, along with missing ADR citations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR re-syncs the Ansible role and Helm chart with decdn d3bc7da7, removing obsolete settings and tightening OTLP endpoint validation.
Changes:
- Removes obsolete payment and rate-bounds configuration.
- Updates endpoint validation, schema inventories, and test fixtures.
- Refreshes chart configuration and operator documentation.
Review identified unresolved endpoint-validation coverage/grammar issues and missing ADR citations in documentation comments.
File summaries
| File | Summary |
|---|---|
charts/decdn-node/ci/ci-values.yaml |
Removes obsolete chart keys. |
ansible/roles/decdn_node/templates/node.toml.j2 |
Stops rendering removed settings. |
ansible/roles/decdn_node/tasks/main.yml |
Updates numeric and OTLP validation. |
ansible/roles/decdn_node/README.md |
Updates schema and operator guidance. |
ansible/roles/decdn_node/defaults/main.yml |
Removes obsolete defaults and updates comments. |
ansible/molecule/schema/files/schema-keys.txt |
Refreshes the schema key inventory. |
ansible/molecule/schema/files/gen-schema-keys.py |
Updates the upstream schema revision. |
ansible/molecule/schema/converge.yml |
Removes obsolete test variables. |
ansible/molecule/default/verify.yml |
Removes the obsolete assertion. |
ansible/molecule/default/converge.yml |
Removes obsolete configuration. |
ansible/inventory/host_vars/decdn-node-1/main.yml |
Updates economics commentary. |
ansible/galaxy/CHANGELOG.md |
Documents removed variables and endpoint changes. |
Review details
Suppressed comments (2)
ansible/galaxy/CHANGELOG.md:14
- The changelog rationale also describes the seller-side rate-floor behavior but cites only an upstream commit, not the ADR that is the repository's required source for protocol/economic claims (
AGENTS.md:12-14). Add the authoritative ADR reference or keep this release note focused on the removed keys and schema incompatibility.
- `decdn_delivery_floor` and `decdn_rate_bounds_poll_interval_sec`: upstream
(decdn/decdn @ d3bc7da7) dropped `payment.delivery_floor` and
`blockchain.rate_bounds_poll_interval_sec` with the seller-side rate-floor clamp,
so emitting either is a startup failure. Setting them now has no effect.
ansible/roles/decdn_node/tasks/main.yml:993
- The host character class still permits
:, sohttp://example:bad:4317passes this guard even though it is not anhttp://host:portendpoint; only bracketed IPv6 should contain colons. Also, Python's$anchor can match before a final newline, so a value ending in\ncan pass despite the documented whitespace rejection. Use separate bracketed-IPv6/non-IPv6 host alternatives and a strict end anchor so this assertion enforces the stated shape.
or (decdn_otlp_endpoint is match('^http://[^\"\\s\\\\/@?#]+:[0-9]+/?$', ignorecase=true))
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… molecule Review follow-up on #49. The endpoint guard admitted two values upstream rejects: - `http://host:bad:4317` — the host class allowed `:`, so a multi-colon authority passed. Only a bracketed IPv6 literal may carry a colon; upstream's `url::Url::parse` reads `bad:4317` as the port and refuses. The host is now an alternation: bracketed IPv6, or a colon-free host. - `http://host:4317\n` — Python's `$` also matches BEFORE a final newline, so a trailing-newline value passed the very guard that exists to stop a newline breaking the rendered TOML basic string. Anchored with `\Z`. The second hole was also in the pre-existing `user_agent` guard next to it (same assert, same TOML-injection hazard), so both anchors move to `\Z`. Coverage — the PR description's templar table was not a repository test: - molecule/validation gains one case per rejection class (`otlp-https`, `otlp-noport`, `otlp-path`, `otlp-userinfo`, `otlp-colon`, `otlp-newline`), each recorded only when the role aborts at its own `Validate optional` assert, and all six added to `_decdn_expected` so a loosened pattern fails the suite. (`block` takes no `loop`, hence one case apiece.) - molecule/default covers the accept half: `HTTP://[::1]:4317` — bracketed IPv6 plus an uppercase scheme, which upstream compares case-insensitively — must survive validation and render verbatim, asserted in the exact-value table. Also cite the governing ADRs for the rate-floor behaviour this PR described in operator-facing files (AGENTS.md: this repo is not a source of truth for protocol claims). ADR 003 §Rate-floor enforcement and ADR 005 §Rate bounds are authoritative; ADR 019 §Step 3.3 is the operator-facing rate choice. Touches the role README, defaults, host_vars comment and the collection CHANGELOG. Verified: make lint, make lint-ansible, and all six molecule scenarios; the validation scenario's exact-set tally now includes the six new tags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…se test gaps Multi-agent review follow-up on #49. Every item below was verified against the real d3bc7da7 binary (`decdn config validate`) before changing anything. Wrong protocol claim (the one that mattered): - defaults/main.yml said "nothing bounds this node's own SELLER quote". False — MAX_RATE_PER_MB = 1000 bounds it at the wire layer, `decdn config validate --rate-per-mb 1001` refuses, and ADR 005 §Rate bounds / ADR 019 §Step 3.3 (cited four lines above it) say so. The role's own range assert for that knob allows up to u64::MAX, so the comment was the only thing an operator reads first. otlp_endpoint port is now bounded 1..=65535: - `http://c:0` and `http://c:70000` passed the guard; upstream refuses both. An out-of-range port renders VALID TOML, so without this only the daemon caught it — and the `decdn config validate` gate is skipped under `make check`, which is exactly where the role's asserts are the whole contract. Every other port knob in the role is already checked at 1..=65535, so this is a consistency fix. Leading zeros stay legal (upstream accepts `:04317`). Test gaps, each proven by mutating the guard and watching the suite stay green: - otlp-userinfo used `user:pass@collector`, which the COLON rule rejects — so the '@' exclusion had no coverage at all. Now `user@collector`: drop '@' from the host class and a credential-bearing endpoint would render into the 0640 node.toml. - The `\Z` anchor on user_agent had no case (the existing quote case passes under `$` too). Added `string-newline`, and `otlp-port` for the new bound. - The seven string-knob rescues matched `^Validate optional`, a TEN-task family, so a regression failing that assert unconditionally could record every tag and "prove" broken guards. They now match the exact task name. Honesty fixes, no behaviour change: - The guard is a deliberate SUPERSET of upstream's parser (it still admits `[:::]` and non-idna hosts, which the config-validate gate catches); the comment now says so instead of implying parity. A differential run of 771 candidates against the real binary found 0 cases where the role rejects what upstream accepts. - fail_msg said "no path" while a bare trailing "/" is accepted; CHANGELOG claimed the node "no longer reads the on-chain rate bounds at all" — narrowed to the ADR's own wording, since ADR 019's Phase 4 table still lists the floor as loaded at startup (upstream doc lag, reported separately). - Dropped a stale `_bad` sentence, disambiguated a bare "that ADR", and refreshed the schema key-floor comment (render is 128, so the slack is 3, not ~10). Verified: make lint, make lint-ansible, all six molecule scenarios, and DECDN_CLI=… make lint-helm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The otlp-userinfo case originally used `http://user:pass@collector:4317`, which tripped KICS's HIGH "Passwords And Secrets - Password in URL" query (CWE-798) and failed `make security` in CI. 889bf7c already replaced the value with `http://user@collector:4317` for a test-coverage reason; record the security reason next to it too, so the next person writing a userinfo fixture does not reintroduce the credential form. Comment-only. Verified: make lint, make security (exit 0, HIGH: 0 in both scans). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Re-syncs the
decdn_noderole and thedecdn-nodeHelm chart to the currentmainofdecdn/decdn(d3bc7da7); the last sync (#44) pinned0b94efe4, 32 commits back. Every config section is#[serde(deny_unknown_fields)]with no serde aliases, so a stale key is a startup crash-loop, not a warning.Field delta (verified against
crates/common/src/config/types.rs)payment.delivery_floorandblockchain.rate_bounds_poll_interval_sec. Upstream dropped the seller-side rate-floor clamp and the rate-bounds watcher (refactor(node,common)!: drop the seller-side rate-floor quote clamp decdn#2037): the node no longer bounds its own sell rate — a rate below the on-chain delivery floor still sells and settles, with only its vote-weight byte credit clamped at redemption.observability.otlp_endpoint— OTLP span export is now always compiled in (theotlpcargo feature is gone), and the endpoint must be exactlyhttp://host:port:https://, a missing port, a path/query/fragment and userinfo are all rejected at config resolution.No other
types.rschange. The421614deployment manifest is unchanged sinced306cc5c, so the host_vars addresses and the manual install method stand.Changed
templates/node.toml.j2; both vars out ofdefaults/main.ymland the two numeric asserts intasks/main.yml; theotlp_endpointshape assert tightened to^http://[^"\s\\/@?#]+:[0-9]+/?$withignorecase=true(upstream matches the scheme case-insensitively); README schema pin and knob docs.gen-schema-keys.pyheader bumped tod3bc7da7, regeneratedschema-keys.txt(157 → 155 paths), the two keys dropped fromschema/converge.ymlanddefault/converge.yml, and thepayment.delivery_floor (explicit 0 must emit)check dropped fromdefault/verify.yml(that branch stays covered bycache.gc_interval_sec).ci/ci-values.yamldrops both keys; the sharedcheck-schema-keys.py+schema-keys.txtrun over the rendered ConfigMap too.galaxy/CHANGELOG.mdUnreleased entry for the two removed operator vars and the stricter endpoint;host_vars/decdn-node-1economics comment rewritten (no more "the daemon overwrites delivery_floor from getRateBounds()").Verification
make lint,make lint-ansible(production profile, 0 failures),make security.make lint-helmandDECDN_CLI=… make lint-helmagainst a freshly builtd3bc7da7binary, so all three ci-values files pass a realdecdn config validate(CI has no real-binary check for the chart path).make molecule— all six scenarios green.HTTP://localhost:4317/andhttp://[::1]:4317accepted;https://x:4317,http://x,http://x:4318/v1/traces,http://u@x:4317, plus quote/backslash/whitespace injections rejected.Note for review
A stale
decdn_delivery_floorleft in an operator's inventory is now silently ignored rather than failing loud — consistent with how the earlierswap_*removal was handled. Happy to add an explicit retired-var assert if we'd rather fail the deploy.🤖 Generated with Claude Code