mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-23 15:19:08 +02:00
[client,management] Skip route firewall rule computation when no firewall (#7624)
* [client,management] Skip route firewall rule computation when no firewall A peer that runs with the firewall disabled has no ACL manager and no firewall to program, so nothing ever reads RoutesFirewallRules: the only consumers are acl.Manager, which is reached solely when e.acl is set, and the legacy-management probe in updateNetworkMap, which is guarded by a non-nil firewall. Building those rules is the most expensive part of a sync on a peer that routes many network resources. On a 15k-peer deployment a debug bundle showed getPeerNetworkResourceFirewallRules accounting for 62% of the allocations of Calculate, and Calculate for effectively all of the allocations of handleSync, which was taking 3.2s on average and holding the engine lock for the duration. Let the caller ask Calculate to leave the rules out. The client passes its existing DisableFirewall setting; the management server keeps the default and still produces them. RoutesFirewallRulesIsEmpty is set from the resulting empty list, so a receiver that would otherwise infer legacy management from an empty rule set does not misread the skip. * [client,management] Cover the skip flag through the envelope Review feedback on #7624. The components test compared only the length of the peer firewall rules, so a change to their content would have passed while the message claimed they came out unchanged. Compare the slices. The skip path was also only exercised by setting the field directly on the components, which bypasses the envelope conversion where RoutesFirewallRulesIsEmpty is derived. That bit is what keeps the client from reading skipped rules as a legacy management server, so it gets a test that goes through EnvelopeToNetworkMap with the flag set. * [management] Give the router a peer ACL so the rule comparison bites Review feedback on #7624. peer-router-1 appears in no peer ACL in the shared fixture, so its FirewallRules came out empty and the equality assertion compared two empty slices — it would have passed even if the peer rules were dropped entirely. Add a policy covering the router and require the baseline to be non-empty before comparing.
This commit is contained in:
@@ -245,7 +245,7 @@ func computeMode(t *testing.T, ctx context.Context, mode Mode, nmData *networkma
|
||||
peerGroups := maps.Keys(nmData.GetPeerGroups(peerID))
|
||||
resp := mgmtgrpc.ToComponentSyncResponse(ctx, nil, nil, nil, peer, nil, nil, components, nil,
|
||||
dnsDomain, nil, nmData.AccountSettings, nil, peerGroups, dnsFwdPort)
|
||||
res, err := networkmap.EnvelopeToNetworkMap(ctx, resp.NetworkMapEnvelope, peer.Key, dnsDomain)
|
||||
res, err := networkmap.EnvelopeToNetworkMap(ctx, resp.NetworkMapEnvelope, peer.Key, dnsDomain, false)
|
||||
require.NoError(t, err, "expand envelope")
|
||||
return res.NetworkMap
|
||||
default:
|
||||
|
||||
@@ -175,6 +175,63 @@ func TestNetworkMapComponents_NetworkResourceRoutes_RouterPeer(t *testing.T) {
|
||||
assert.NotEmpty(t, nm.RoutesFirewallRules, "router peer should have route firewall rules for the resource")
|
||||
}
|
||||
|
||||
// A receiver without a firewall asks Calculate to skip the route firewall
|
||||
// rules. Everything the rest of the sync consumes — routes, peers, peer
|
||||
// firewall rules — must come out unchanged.
|
||||
func TestNetworkMapComponents_SkipRouteFirewallRules(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
account := createComponentTestAccount()
|
||||
|
||||
// The shared fixture leaves peer-router-1 out of every peer ACL, so its
|
||||
// FirewallRules would be empty and the comparison below vacuous. Give the
|
||||
// router a policy of its own.
|
||||
account.Policies = append(account.Policies, &types.Policy{
|
||||
ID: "policy-router", Name: "Router connectivity", Enabled: true,
|
||||
Rules: []*types.PolicyRule{{
|
||||
ID: "rule-router", Name: "Allow all <-> router", Enabled: true,
|
||||
Action: types.PolicyTrafficActionAccept, Protocol: types.PolicyRuleProtocolALL,
|
||||
Bidirectional: true,
|
||||
Sources: []string{"group-all"}, Destinations: []string{"group-all"},
|
||||
}},
|
||||
})
|
||||
|
||||
validated := allPeersValidated(account)
|
||||
|
||||
components := account.GetPeerNetworkMapComponents(
|
||||
ctx,
|
||||
"peer-router-1",
|
||||
account.GetPeersCustomZone(ctx, "netbird.io"),
|
||||
nil,
|
||||
validated,
|
||||
account.GetResourcePoliciesMap(),
|
||||
account.GetResourceRoutersMap(),
|
||||
account.GetActiveGroupUsers(),
|
||||
)
|
||||
|
||||
full := components.Calculate(ctx)
|
||||
require.NotEmpty(t, full.RoutesFirewallRules, "baseline: router peer must get route firewall rules")
|
||||
require.NotEmpty(t, full.FirewallRules, "baseline: router peer must get peer firewall rules")
|
||||
|
||||
components.SkipRouteFirewallRules = true
|
||||
skipped := components.Calculate(ctx)
|
||||
|
||||
assert.Empty(t, skipped.RoutesFirewallRules, "route firewall rules must not be computed when skipped")
|
||||
assert.ElementsMatch(t, routeNetworks(full.Routes), routeNetworks(skipped.Routes),
|
||||
"skipping route firewall rules must not change the routes")
|
||||
assert.ElementsMatch(t, peerIDs(full.Peers), peerIDs(skipped.Peers),
|
||||
"skipping route firewall rules must not change the peers to connect")
|
||||
assert.Equal(t, full.FirewallRules, skipped.FirewallRules,
|
||||
"peer firewall rules are unrelated and must come out unchanged")
|
||||
}
|
||||
|
||||
func routeNetworks(routes []*nmdata.Route) []string {
|
||||
networks := make([]string, 0, len(routes))
|
||||
for _, r := range routes {
|
||||
networks = append(networks, r.Network.String())
|
||||
}
|
||||
return networks
|
||||
}
|
||||
|
||||
func TestNetworkMapComponents_NetworkResourceRoutes_UnrelatedPeer(t *testing.T) {
|
||||
account := createComponentTestAccount()
|
||||
validated := allPeersValidated(account)
|
||||
|
||||
Reference in New Issue
Block a user