diff --git a/internal/iptables/iptables.go b/internal/iptables/iptables.go index 6e986c8..0a1ec2f 100644 --- a/internal/iptables/iptables.go +++ b/internal/iptables/iptables.go @@ -87,10 +87,23 @@ func GenerateIptableRulesFromNetworkPolicies(policies v1alpha1.EgressNetworkPoli // allow peer to communicate with itself fmt.Sprintf("-A %s -d %s -j ACCEPT", peerChain, peerIp), + } - // allow peer to communicate with kube-dns (UDP and TCP for large DNS responses) - fmt.Sprintf("-A %s -d %s -p UDP --dport 53 -j ACCEPT", peerChain, kubeDnsIp), - fmt.Sprintf("-A %s -d %s -p TCP --dport 53 -j ACCEPT", peerChain, kubeDnsIp), + // allow peer to communicate with kube-dns (UDP and TCP for large DNS responses). + // kubeDnsIp may carry more than one address (e.g. an anycast resolver pair) as a + // comma-separated list. iptables-restore takes a single address per -d and + // tokenises on whitespace, so interpolating the list raw produces + // `-d 1.2.3.4, 5.6.7.8` and the whole restore aborts with "Bad argument" — + // leaving the ruleset half-applied and forwarding broken. Emit one rule per address. + for _, dns := range strings.Split(kubeDnsIp, ",") { + dns = strings.TrimSpace(dns) + if dns == "" { + continue + } + rules = append(rules, + fmt.Sprintf("-A %s -d %s -p UDP --dport 53 -j ACCEPT", peerChain, dns), + fmt.Sprintf("-A %s -d %s -p TCP --dport 53 -j ACCEPT", peerChain, dns), + ) } for _, policy := range policies { diff --git a/internal/iptables/iptables_test.go b/internal/iptables/iptables_test.go index 0ffa5fe..797dfd5 100644 --- a/internal/iptables/iptables_test.go +++ b/internal/iptables/iptables_test.go @@ -73,6 +73,29 @@ func TestIptableRules(t *testing.T) { -A 10-8-0-9 -d 10.8.0.9 -j ACCEPT -A 10-8-0-9 -d 100.64.0.10 -p UDP --dport 53 -j ACCEPT -A 10-8-0-9 -d 100.64.0.10 -p TCP --dport 53 -j ACCEPT +# end of rules for peer 10.8.0.9`, + }, + { + // Regression: an anycast DNS pair arrives as a comma+space separated + // list ("172.31.255.253, 172.31.255.254"). Interpolated raw it yields + // `-d 172.31.255.253, 172.31.255.254`, which iptables-restore tokenises + // on whitespace and rejects with "Bad argument `172.31.255.254'" — + // aborting the whole restore and leaving forwarding rules broken. + // Each address must get its own rule. + name: "Multiple DNS servers emit one rule per address", + peerIp: "10.8.0.9", + kubeDnsIp: "172.31.255.253, 172.31.255.254", + wgServerIp: "10.8.0.1", + networkPolicies: v1alpha1.EgressNetworkPolicies{}, + expectedIptableRules: `# start of rules for peer 10.8.0.9 +:10-8-0-9 - [0:0] +-A FORWARD -s 10.8.0.9 -j 10-8-0-9 +-A 10-8-0-9 -d 10.8.0.1 -p icmp -j ACCEPT +-A 10-8-0-9 -d 10.8.0.9 -j ACCEPT +-A 10-8-0-9 -d 172.31.255.253 -p UDP --dport 53 -j ACCEPT +-A 10-8-0-9 -d 172.31.255.253 -p TCP --dport 53 -j ACCEPT +-A 10-8-0-9 -d 172.31.255.254 -p UDP --dport 53 -j ACCEPT +-A 10-8-0-9 -d 172.31.255.254 -p TCP --dport 53 -j ACCEPT # end of rules for peer 10.8.0.9`, }, { diff --git a/internal/wireguard/liveness.go b/internal/wireguard/liveness.go index 5717c85..6d98001 100644 --- a/internal/wireguard/liveness.go +++ b/internal/wireguard/liveness.go @@ -467,6 +467,7 @@ type peerInfo struct { Keepalive time.Duration Address string Mode string // explicit per-peer routeLiveness ("" = inherit) + HasRoutes bool // peer declares downstream routes (v4 or v6) } // SetPeers resolves each peer's effective mode (peer > instance > cluster default) @@ -475,7 +476,15 @@ type peerInfo struct { func (c *LivenessController) SetPeers(instanceMode string, peers []peerInfo) { c.mu.Lock() for _, p := range peers { - c.mode[p.PublicKey] = resolveMode(p.Mode, instanceMode, c.clusterDefault) + m := resolveMode(p.Mode, instanceMode, c.clusterDefault) + // Gating only ever adds or withholds a peer's downstream routes, so a peer + // that declares none has nothing to gate. Left gated it would still flip + // live/down (road-warrior laptops do this constantly) and every flip calls + // apply() → a full state push, for no routing benefit. Force ungated. + if !p.HasRoutes { + m = ModeDisabled + } + c.mode[p.PublicKey] = m } c.mu.Unlock() for _, p := range peers { @@ -503,7 +512,13 @@ func PeerInfos(state agent.State) []peerInfo { if p.Spec.PersistentKeepalive != nil && *p.Spec.PersistentKeepalive > 0 { k = time.Duration(*p.Spec.PersistentKeepalive) * time.Second } - out = append(out, peerInfo{PublicKey: p.Spec.PublicKey, Keepalive: k, Address: p.Spec.Address, Mode: p.Spec.RouteLiveness}) + out = append(out, peerInfo{ + PublicKey: p.Spec.PublicKey, + Keepalive: k, + Address: p.Spec.Address, + Mode: p.Spec.RouteLiveness, + HasRoutes: len(p.Spec.Routes) > 0 || len(p.Spec.RoutesV6) > 0, + }) } return out } diff --git a/internal/wireguard/liveness_test.go b/internal/wireguard/liveness_test.go index 773dbdd..759492c 100644 --- a/internal/wireguard/liveness_test.go +++ b/internal/wireguard/liveness_test.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr" "github.com/nccloud/wireguard-operator/api/v1alpha1" + "github.com/nccloud/wireguard-operator/internal/agent" ) func TestBuildController_AlwaysBuiltAndClusterDefaultUngated(t *testing.T) { @@ -56,9 +57,11 @@ func TestController_PerPeerModeResolution(t *testing.T) { lastUp: map[string]bool{}, } // instance=active; DC1 explicitly disabled (fallback); DC2 inherits active. + // Both are site peers carrying downstream routes — without routes there would + // be nothing to gate and both would resolve to disabled. c.SetPeers("active", []peerInfo{ - {PublicKey: "dc1", Mode: "disabled", Keepalive: 25 * time.Second, Address: "172.31.255.11"}, - {PublicKey: "dc2", Mode: "", Keepalive: 25 * time.Second, Address: "172.31.255.12"}, + {PublicKey: "dc1", Mode: "disabled", Keepalive: 25 * time.Second, Address: "172.31.255.11", HasRoutes: true}, + {PublicKey: "dc2", Mode: "", Keepalive: 25 * time.Second, Address: "172.31.255.12", HasRoutes: true}, }) if c.modeFor("dc1") != ModeDisabled { t.Errorf("dc1 explicit disabled, got %s", c.modeFor("dc1")) @@ -71,6 +74,77 @@ func TestController_PerPeerModeResolution(t *testing.T) { } } +func TestController_RouteLessPeerIsNeverGated(t *testing.T) { + // A peer with no routes has nothing to install or withdraw, so gating it can + // never change forwarding — it only produces pointless liveness transitions, + // each of which triggers an apply()/state push. Road-warrior peers (laptops + // that come and go) are exactly this shape, and their churn was driving + // fleet-wide state pushes. Such peers must be ungated regardless of mode. + clk := ts(1000) + pass := newPassiveLiveness(3, func() time.Time { return clk }) + c := &LivenessController{ + passive: pass, + active: newActiveLiveness(pass, 3, 15*time.Second, func() time.Time { return clk }, proberFunc(func(string) {})), + clusterDefault: ModeActive, + mode: map[string]Mode{}, + lastUp: map[string]bool{}, + } + c.SetPeers("active", []peerInfo{ + {PublicKey: "roadwarrior", Mode: "", Keepalive: 25 * time.Second, Address: "172.31.255.8", HasRoutes: false}, + {PublicKey: "site", Mode: "", Keepalive: 25 * time.Second, Address: "172.31.255.3", HasRoutes: true}, + }) + if got := c.modeFor("roadwarrior"); got != ModeDisabled { + t.Errorf("route-less peer must be ungated, got mode %q", got) + } + if !c.IsLive("roadwarrior") { + t.Error("route-less peer must always be live (nothing to gate)") + } + // A peer that does carry routes still inherits the active mode. + if got := c.modeFor("site"); got != ModeActive { + t.Errorf("peer with routes should inherit active, got %q", got) + } +} + +func TestController_RouteLessPeerIgnoresExplicitActive(t *testing.T) { + // Even an explicit routeLiveness=active is meaningless without routes. + clk := ts(1000) + pass := newPassiveLiveness(3, func() time.Time { return clk }) + c := &LivenessController{ + passive: pass, + active: newActiveLiveness(pass, 3, 15*time.Second, func() time.Time { return clk }, proberFunc(func(string) {})), + clusterDefault: ModeDisabled, + mode: map[string]Mode{}, + lastUp: map[string]bool{}, + } + c.SetPeers("", []peerInfo{ + {PublicKey: "rw", Mode: "active", Keepalive: 25 * time.Second, Address: "172.31.255.9", HasRoutes: false}, + }) + if got := c.modeFor("rw"); got != ModeDisabled { + t.Errorf("route-less peer must be ungated even when explicitly active, got %q", got) + } +} + +func TestPeerInfos_DerivesHasRoutesFromSpec(t *testing.T) { + state := agent.State{Peers: []v1alpha1.WireguardPeer{ + {Spec: v1alpha1.WireguardPeerSpec{PublicKey: "none", Address: "172.31.255.8"}}, + {Spec: v1alpha1.WireguardPeerSpec{PublicKey: "v4", Address: "172.31.255.3", Routes: []string{"10.254.0.0/16"}}}, + {Spec: v1alpha1.WireguardPeerSpec{PublicKey: "v6", Address: "172.31.255.4", RoutesV6: []string{"fd00::/64"}}}, + }} + got := map[string]bool{} + for _, p := range PeerInfos(state) { + got[p.PublicKey] = p.HasRoutes + } + if got["none"] { + t.Error("peer without routes should have HasRoutes=false") + } + if !got["v4"] { + t.Error("peer with IPv4 routes should have HasRoutes=true") + } + if !got["v6"] { + t.Error("peer with only IPv6 routes should have HasRoutes=true") + } +} + func TestPeerStat_FakeReaderRoundTrips(t *testing.T) { now := time.Unix(1000, 0) r := fakeReader{peers: []peerStat{{PublicKey: "k", LastHandshakeTime: now, ReceiveBytes: 42}}} @@ -181,7 +255,7 @@ func TestController_AppliesOnlyOnTransition(t *testing.T) { t.Fatalf("quiet tick must not apply, got %d", applied) } clk = ts(1100) // 100s later, > 75s window, no new bytes - c.tickOnce() // live->not-live transition + c.tickOnce() // live->not-live transition if applied != 2 { t.Fatalf("expected apply on down-transition, got %d", applied) }