Preserve pre-existing ipsets and roll back partial route ACL installs

client/firewall/iptables/acl_linux.go

@@ -92,6 +92,10 @@ func (m *aclManager) AddPeerFiltering(
 		return m.addPeerRule(ip, protocol, sPort, dPort, action, "")
 	}
 
+	// A set that is already in the store backs rules installed earlier, so it must
+	// survive this call's failure.
+	_, preexisting := m.ipsetStore.ipset(ipsetName)
+
 	rules, err := m.addPeerRule(ip, protocol, sPort, dPort, action, ipsetName)
 	if err == nil {
 		return rules, nil
@@ -102,10 +106,12 @@ func (m *aclManager) AddPeerFiltering(
 		return nil, err
 	}
 
-	// The set could not be created or matched. Drop whatever was created and
+	// The set could not be created or matched. Drop the one this call created and
 	// retry the rule matching the IP directly; only if that succeeds do we know
 	// ipset was to blame and latch it off for subsequent rules.
-	m.discardIPSet(ipsetName)
+	if !preexisting {
+		m.discardIPSet(ipsetName)
+	}
 
 	rules, retryErr := m.addPeerRule(ip, protocol, sPort, dPort, action, "")
 	if retryErr != nil {

client/firewall/iptables/router_linux.go

@@ -174,6 +174,7 @@ func (r *router) AddRouteFiltering(
 		r.removeRouteRules(string(ruleKey))
 
 		if retryErr := r.installRouteRules(string(ruleKey), params, sources, false); retryErr != nil {
+			r.removeRouteRules(string(ruleKey))
 			return nil, fmt.Errorf("add route rule (ipset: %w): %w", unusable.cause, retryErr)
 		}
 
@@ -182,6 +183,11 @@ func (r *router) AddRouteFiltering(
 	}
 
 	if err != nil {
+		// Leave nothing half-installed: a later call finding the rule key would
+		// report success while some sources were never installed, which for a
+		// drop rule would leave them unblocked.
+		r.removeRouteRules(string(ruleKey))
+
 		return nil, fmt.Errorf("add route rule: %w", err)
 	}
 
@@ -221,6 +227,20 @@ func (r *router) genRouteRuleSpecs(params routeFilteringRuleParams, sources []ne
 		return nil, fmt.Errorf("apply network -d: %w", err)
 	}
 
+	specs, err := r.genSourceRules(params, sources, useIPSet, destExp)
+	if err != nil {
+		// The destination match may have taken a set reference already.
+		if decErr := r.decrementSetCounter(destExp); decErr != nil {
+			log.Debugf("release destination set after failed rule generation: %v", decErr)
+		}
+
+		return nil, err
+	}
+
+	return specs, nil
+}
+
+func (r *router) genSourceRules(params routeFilteringRuleParams, sources []netip.Prefix, useIPSet bool, destExp []string) ([][]string, error) {
 	if useIPSet || len(sources) <= 1 {
 		sourceExp, err := r.applyNetwork("-s", sourceNetwork(sources), sources)
 		if err != nil {

client/firewall/iptables/router_linux_test.go

@@ -17,6 +17,7 @@ import (
 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/firewall/test"
 	"github.com/netbirdio/netbird/client/iface"
+	nbid "github.com/netbirdio/netbird/client/internal/acl/id"
 	nbnet "github.com/netbirdio/netbird/client/net"
 	"github.com/netbirdio/netbird/shared/management/domain"
 )
@@ -532,6 +533,47 @@ func TestRouter_DestinationSetRequiresIPSet(t *testing.T) {
 	require.ErrorContains(t, err, "requires ipset")
 }
 
+// TestRouter_RouteFilteringRollsBackPartialInstall covers a fallback ACL whose
+// second rule cannot be installed. Nothing may be left behind: if the rule key
+// survived, a later call would short-circuit on it and report success while some
+// sources were never installed, leaving them unblocked for a drop rule.
+func TestRouter_RouteFilteringRollsBackPartialInstall(t *testing.T) {
+	if !isIptablesSupported() {
+		t.Skip("iptables not supported on this system")
+	}
+
+	iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
+	require.NoError(t, err)
+
+	support := newIPSetSupport()
+	support.markUnsupported(errors.New("test: pretend the kernel has no ipset"))
+
+	r, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, support)
+	require.NoError(t, err)
+	require.NoError(t, r.init(nil))
+	t.Cleanup(func() {
+		require.NoError(t, r.Reset())
+	})
+
+	// The v6 prefix is rejected by the v4 iptables binary, so the second rule of
+	// the expansion fails after the first has been installed.
+	good := netip.MustParsePrefix("172.16.0.0/16")
+	sources := []netip.Prefix{good, netip.MustParsePrefix("2001:db8::/32")}
+	destination := firewall.Network{Prefix: netip.MustParsePrefix("10.0.0.0/8")}
+
+	_, err = r.AddRouteFiltering(nil, sources, destination, firewall.ProtocolALL, nil, nil, firewall.ActionDrop)
+	require.Error(t, err, "a source that iptables rejects must fail the whole ACL")
+
+	ruleKey := nbid.GenerateRouteRuleKey(sources, destination, firewall.ProtocolALL, nil, nil, firewall.ActionDrop)
+	require.Empty(t, routeRuleSpecs(t, r, string(ruleKey)), "no rule may stay recorded")
+
+	// The rule that did get installed must be gone from the chain.
+	installed := []string{"-s", good.String(), "-d", "10.0.0.0/8", "-j", "DROP"}
+	exists, err := iptablesClient.Exists(tableFilter, chainRTFWDIN, installed...)
+	require.NoError(t, err)
+	require.False(t, exists, "the already-installed rule must be rolled back")
+}
+
 // routeRuleSpecs collects the rules recorded for one route ACL, which is more than
 // one when the ipset fallback splits it per source prefix.
 func routeRuleSpecs(t *testing.T, r *router, ruleKey string) [][]string {