hashgraph / hashgraph/solo-weaver
Traffic-shaper reconciler clears policy sets on a single empty statusz response, with no confirmation or signal
- Dominant language
- Go
- Stars
- 3
- Forks
- 0
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 47
Description
## Problem
The traffic-shaper daemon reconciles `inet weaver-workload-policy` set membership from the
block node's statusz endpoints on every successful poll. `bucketizeEndpoints`
(`internal/blocknode/shaper/reconciler.go:204-208`) seeds every owned category present with an
empty slice before filling it from the response, so a category the BN does not report in a
given poll **collapses to an empty membership immediately** — no confirmation, no debounce, no
distinction from a real transition to zero:
```go
// Every owned binding is seeded present with an empty slice, so a category the
// BN no longer reports collapses to an empty membership that clears its set
// rather than leaving stale members behind — each owned set is fully reconciled
// every tick.
```
`Apply` (`reconciler.go:117-165`) then writes that empty membership into the live kernel sets
and, in the same lock hold, rewrites `network-weaver-workload-policy.nft`
(`internal/network/policy/manager.go:683-707`, `329-337`) — so **one** tick where `GET
/statusz/inbound` or `/statusz/outbound` returns `200` with an empty `activeEndpoints` list is
enough to clear `bn-publisher`, `bn-partner-out`, `bn-restricted` and/or `bn-backfill`, both in
the kernel and on disk.
This is intentional for a BN that genuinely reports nothing for a category — it is the only
mechanism by which `bn-restricted` un-quarantines a peer, since there is no explicit
"un-restrict" signal, only absence from the category. But the client has no way to tell that
apart from a BN bug, a transient internal error that still returns `200`, or a response
truncated in flight. Any of those produces the same observable state as a legitimate all-quiet
BN.
### Why this is worse than it looks
The effect is invisible under normal conditions, which is what makes it dangerous:
- Every ingress HTB class ceils at 100% of the trunk
(`internal/network/shape/defaults.go:48-53`), and all three leaves share one trunk class
(`internal/network/shape/veth.go:48-51`). With `bn-publisher`/`bn-partner-out` cleared,
nothing stamps a `meta priority`, so every packet falls through to the HTB default class
(`reserve-ingress`, `1:30`) and simply **borrows** the idle trunk capacity.
- A throughput check during the affected window measures full line rate and looks healthy. The
loss of prioritization only becomes visible once real publisher/partner traffic returns and
contends with everything else for `1:30`'s 10% guarantee.
- `bn-restricted` clearing has the same invisibility: the quarantined peer simply stops being
dropped, with no log line calling out that a set went from non-empty to empty.
This is the same class of symptom as #1033 ("host-firewall verbs wipe daemon-owned
workload-policy set membership, silently lifting quarantine") — quarantine lifts and
classification collapses with no operator-visible signal — but a different root cause. #1033
was an unrelated command touching a table it had no business touching; this is the reconciler
itself, doing its designed job, on a single tick's worth of possibly-wrong input.
## Proposed fix
Not a blanket skip of `Apply()` on any empty response — that defeats the un-quarantine path
for `bn-restricted` and reintroduces the exact staleness problem `bucketizeEndpoints`'s comment
says the design avoids.
Instead, judge each statusz endpoint's payload as a whole before trusting its per-category
breakdown, since that is the actual granularity of a likely failure. `NetworkData` is a flat
`activeEndpoints` list with no per-category structure (`statusz_client.go:47-56`) — a category
being "empty" is always inferred by filtering that list, never an explicit field. So:
- **The whole payload is empty** (`len(activeEndpoints) == 0`) — do nothing. Leave every
category that endpoint feeds exactly as it is. This is the shape a broken handler, a
panic-recovery path, or a truncated response actually takes: the *whole* call comes back
empty, not a real payload with one category selectively missing.
- **The payload has entries, but a specific category has none of them** — trust it, and clear
that category's set as today. This is the common, expected case (no partner connections right
now; a peer's restriction genuinely lifting) and needs no protection.
This gates per **endpoint call**, not per category, since that is what the client actually
fetches independently: an empty `/statusz/inbound` response withholds `bn-publisher`,
`bn-partner-out`, `bn-restricted`, and the managed-ports sets together (all four are derived
from that one call); an empty `/statusz/outbound` withholds `bn-backfill` alone.
This is stateless — no counter or confirmation window across ticks — and its failure direction
is safe where it matters most: a fully-empty response can never wrongly lift `bn-restricted`,
since "do nothing" cannot remove a restriction that was already applied. A legitimate
category-level transition to zero (real un-quarantine, real idle category) still lands on the
very next poll, with no added latency.
**Known residual gap, accepted rather than solved here:** this does not protect against a bug
that returns a populated-but-wrong response — for example, the BN's categorization logic drops
just `restricted` while `publisher` and `partner` are still reported correctly. That failure
shape is narrower and far less likely than "the whole handler returned nothing," so it is not
worth the added complexity of a per-category confirmation window unless it turns out to happen
in practice.
Pair this with an operator-visible signal (log line, and/or a `daemon service check` /
`/status` field) the first time a poll actually clears a previously non-empty owned category —
today this happens silently either way.
## Acceptance
- [ ] A tick where `/statusz/inbound` returns `activeEndpoints: []` leaves `bn-publisher`,
`bn-partner-out`, `bn-restricted`, and the managed-ports sets untouched — live kernel and
persisted `.nft` both.
- [ ] A tick where `/statusz/outbound` returns `activeEndpoints: []` leaves `bn-backfill`
untouched.
- [ ] A tick where the payload has entries but none tagged with a given category still clears
that category's set, exactly as today — no added latency for a legitimate transition.
- [ ] The first tick that actually clears a previously non-empty owned category logs it (or
surfaces via `/status`), so this is no longer silent.
- [ ] Unit coverage in `internal/blocknode/shaper` for: whole-payload-empty (no change to any
fed category), populated-payload-missing-one-category (clears that category only), and
populated-payload-all-categories-present (updates all as today).
Contributor guide
Assessment
This issue has not been assessed yet.