hashgraph / hashgraph/solo-weaver

fix(network/policy): make compound classification rules precede address-only rules (bn-backfill vs bn-partner-out)

Open
#924 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.