hashgraph / hashgraph/solo-weaver
fix(network/policy): make compound classification rules precede address-only rules (bn-backfill vs bn-partner-out)
- Dominant language
- Go
- Stars
- 3
- Forks
- 0
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 47
Description
## Problem
The traffic-shaper renders two egress classification rules into the `inet weaver`
`forward` chain that can both match traffic to the same peer IP:
- `bn-partner-out` — matches `ip daddr @bn-partner-out tcp sport @bn-partner-out_ports`
(the BN's responses to inbound partner subscribers)
- `bn-backfill` — matches `ip daddr . tcp dport @bn-backfill` (compound
address·port; outbound backfill requests)
A peer that is both an inbound partner and an outbound backfill target is a
member of **both** sets. Both rules are terminating (`… meta priority set
accept`), so whichever is emitted first wins. Today `bn-backfill` is emitted
first and backfill traffic is correctly classified as reserve-egress (`1:60`),
**but only incidentally**: Tier-3 classification rules are ordered
alphabetically by set name (`internal/network/policy/render.go:43`), and
`"bn-backfill"` sorts before `"bn-partner-out"`. There is no rule that orders
more-specific (address·port) matches ahead of address-only matches.
Consequences:
- A future set rename (e.g. anything that sorts an address-only rule ahead of a
compound rule for an overlapping peer) would **silently misclassify** backfill
traffic into the wrong tc class, with no failing test.
- `checkNoOverlap` (`internal/network/policy/policy.go:259`) keys only on
`(direction, ports)`, so it does not treat address-only vs compound
dual-membership as an overlap and would not catch the regression.
- The golden test (`TestRender_GoldenMatchesBNInstallSet`) captures the current
order incidentally but asserts nothing about the intent;
`TestRender_TierOrderInvariants` only asserts specific-before-fallthrough, not
compound-before-address-only.
Surfaced while reviewing the outbound-partner backfill fix in #921, which is what
first makes `bn-backfill` populate against a real BN and thus creates the
dual-membership scenario in practice.
## Proposed fix
- Make the ordering **intentional**: within the classification tier, order
compound (`ipv4_addr . inet_service`) rules ahead of address-only rules for the
same direction, so a more-specific match always precedes a broader one that
could shadow it. (Alternatively, encode an explicit precedence weight on the
policy rather than relying on name sort.)
- Add a dedicated invariant test asserting that, for a peer IP present in both
`bn-partner-out` and `bn-backfill`, the `bn-backfill` rule is emitted before
(and therefore wins over) the `bn-partner-out` rule — independent of set names.
- Consider extending `checkNoOverlap` awareness so address-only vs compound
dual-membership is a recognized (and asserted-safe) case rather than invisible.
## Acceptance
- [ ] Classification-rule ordering guarantees compound address·port rules precede
address-only rules for the same direction, by design rather than by
incidental name sort.
- [ ] A test fails if a peer in both `bn-backfill` and `bn-partner-out` would be
classified as partner rather than backfill (i.e. the address-only rule is
emitted first).
- [ ] The guarantee survives a set rename that would otherwise flip the
alphabetical order.
Contributor guide
Research direction
Start in internal/network/policy/render.go, where Tier-3 classification rules are ordered, and review internal/network/policy/policy.go around checkNoOverlap. Run TestRender_TierOrderInvariants and TestRender_GoldenMatchesBNInstallSet, then add an invariant covering a peer shared by bn-backfill and bn-partner-out. Done means compound address-port rules precede address-only rules independently of set names and the ordering test fails if reversed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100