Compare a firewall reload with what the running firewall was built from (#1955)

config/config.go

@@ -86,6 +86,21 @@ func (c *C) InitialLoad() bool {
 	return c.oldSettings == nil
 }
 
+// Render returns keys and their values as yaml, for a subsystem to remember what it applied. Empty means they couldn't
+// be rendered and should never match.
+func (c *C) Render(keys ...string) string {
+	m := make(map[string]any, len(keys))
+	for _, k := range keys {
+		m[k] = c.Get(k)
+	}
+	b, err := yaml.Marshal(m)
+	if err != nil {
+		c.l.Debug("Failed to render config", "keys", keys, "error", err)
+		return ""
+	}
+	return string(b)
+}
+
 // HasChanged checks if the underlying structure of the provided key has changed after a config reload. The value of
 // k in both the old and new settings will be serialized, the result of the string comparison is returned.
 // If k is an empty string the entire config is tested.

firewall.go

@@ -66,6 +66,9 @@ type Firewall struct {
 	rules        string
 	rulesVersion uint16
 
+	// source is the rendered firewall config this firewall was built from, the cert networks are kept above
+	source string
+
 	defaultLocalCIDRAny bool
 	incomingMetrics     firewallMetrics
 	outgoingMetrics     firewallMetrics
@@ -246,6 +249,7 @@ func NewFirewallFromConfig(l *slog.Logger, cs *CertState, c *config.C) (*Firewal
 		return nil, err
 	}
 
+	fw.source = c.Render("firewall")
 	return fw, nil
 }
 

interface.go

@@ -537,11 +537,12 @@ func (f *Interface) reloadFirewall(c *config.C, r *config.Reload) {
 		curCert = cs.getCertificate(cert.Version1)
 	}
 
-	// The firewall builds its routableNetworks set from the certificate's UnsafeNetworks at construction.
-	// Check to see if that set has changed, and if so, rebuild the firewall.
-	certUnsafeChanged := curCert != nil && !slices.Equal(curCert.UnsafeNetworks(), f.firewall.unsafeNetworks)
-
-	if !c.HasChanged("firewall") && !certUnsafeChanged {
+	// Compare with what the running firewall was built from, not the last config, so a firewall that failed to build
+	// is tried again. The cert is the one this reload brought, PKI registers its reload callback before this one
+	src := c.Render("firewall")
+	if src != "" && src == f.firewall.source && curCert != nil &&
+		slices.Equal(curCert.Networks(), f.firewall.assignedNetworks) &&
+		slices.Equal(curCert.UnsafeNetworks(), f.firewall.unsafeNetworks) {
 		f.l.Debug("No firewall config change detected")
 		return
 	}

interface_test.go

@@ -135,8 +135,63 @@ func TestReloadFirewall_BadRulesFailTheReload(t *testing.T) {
 	f := &Interface{pki: pki, firewall: fw, l: l}
 	cfg.RegisterReloadCallback(f.reloadFirewall)
 
-	res := cfg.ReloadConfigString(rules("nope"))
+	// The running firewall still isn't built from what the config asks for, so the same reload fails again
+	for range 2 {
+		res := cfg.ReloadConfigString(rules("nope"))
+		require.NoError(t, res.Err)
+		assert.True(t, res.Failed)
+		assert.Same(t, fw, f.firewall)
+	}
+
+	res := cfg.ReloadConfigString(rules("tcp"))
 	require.NoError(t, res.Err)
-	assert.True(t, res.Failed)
-	assert.Same(t, fw, f.firewall)
+	assert.False(t, res.Failed)
+	assert.NotSame(t, fw, f.firewall)
+}
+
+// Every input the firewall is built from rebuilds it when it changes, and nothing else does
+func TestReloadFirewall_RebuildsOnAnyInput(t *testing.T) {
+	l := test.NewLogger()
+	networks := []netip.Prefix{netip.MustParsePrefix("10.0.0.1/24")}
+	base := "firewall:\n  inbound:\n    - port: any\n      proto: any\n      host: any\n"
+	setup := func() (*config.C, *Interface, *PKI) {
+		pki := &PKI{}
+		pki.cs.Store(&CertState{v2Cert: &dummyCert{version: cert.Version2, networks: networks}, initiatingVersion: cert.Version2})
+		cfg := config.NewC(l)
+		require.NoError(t, cfg.LoadString(base))
+		fw, err := NewFirewallFromConfig(l, pki.getCertState(), cfg)
+		require.NoError(t, err)
+		f := &Interface{pki: pki, firewall: fw, l: l}
+		cfg.RegisterReloadCallback(f.reloadFirewall)
+		return cfg, f, pki
+	}
+	setCert := func(pki *PKI, networks, unsafe []netip.Prefix) {
+		pki.cs.Store(&CertState{v2Cert: &dummyCert{version: cert.Version2, networks: networks, unsafeNetworks: unsafe}, initiatingVersion: cert.Version2})
+	}
+
+	cases := map[string]struct {
+		cfg  string
+		cert func(*PKI)
+		want bool
+	}{
+		"nothing":         {base + "punchy:\n  punch: true\n", nil, false},
+		"rules":           {base + "  outbound:\n    - port: any\n      proto: any\n      host: any\n", nil, true},
+		"inbound_action":  {base + "  inbound_action: reject\n", nil, true},
+		"conntrack":       {base + "  conntrack:\n    tcp_timeout: 1m\n", nil, true},
+		"networks":        {base, func(p *PKI) { setCert(p, []netip.Prefix{netip.MustParsePrefix("10.0.0.2/24")}, nil) }, true},
+		"unsafe_networks": {base, func(p *PKI) { setCert(p, networks, []netip.Prefix{netip.MustParsePrefix("192.168.0.0/24")}) }, true},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			cfg, f, pki := setup()
+			old := f.firewall
+			if tc.cert != nil {
+				tc.cert(pki)
+			}
+			res := cfg.ReloadConfigString(tc.cfg)
+			require.NoError(t, res.Err)
+			assert.False(t, res.Failed)
+			assert.Equal(t, tc.want, f.firewall != old)
+		})
+	}
 }