[client] Read an emptied NAT list as the absent one it matches

apply() compared NATExternalIPs with reflect.DeepEqual, which calls a nil
slice and an empty slice different. Both mean the same thing — no NAT
mappings — and the two meet on a perfectly ordinary start: a profile stores
the absent list as JSON null and reads it back nil, while `netbird up` sends
CleanNATExternalIPs, an empty list, whenever NB_EXTERNAL_IP_MAP is set to
nothing, which a deployment template does by default.

So the gate saw a change where nothing changed and refused the request with
FailedPrecondition. That is the same deadlock this branch exists to remove,
reached through another field: a container with the kill switch on could not
come up, and `netbird up` reported "the daemon refused the settings update".

The DNS label list next to it already used slices.Equal, which treats nil and
empty as the same list. The NAT list now does too, and the last use of
reflect in the package goes with it.

Reported by pappz in review.
This commit is contained in:
riccardom
2026-09-29 14:32:22 +02:00
parent 2885e3771d
commit 4c0fbc0df1
2 changed files with 43 additions and 2 deletions
+8 -2
View File
@@ -10,7 +10,6 @@ import (
"os"
"os/user"
"path/filepath"
"reflect"
"runtime"
"slices"
"strings"
@@ -540,7 +539,14 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.NATExternalIPs != nil && !reflect.DeepEqual(config.NATExternalIPs, input.NATExternalIPs) {
// slices.Equal, not reflect.DeepEqual, and for the same reason the DNS
// labels below use it: DeepEqual calls a nil slice and an empty one
// different, while both mean "no NAT mappings". A profile stores the
// absent list as JSON null and reads it back nil, and `netbird up` sends
// CleanNATExternalIPs — an empty list — whenever NB_EXTERNAL_IP_MAP is set
// to nothing, so the two met on every start and the gate read a no-op as a
// settings change.
if input.NATExternalIPs != nil && !slices.Equal(config.NATExternalIPs, input.NATExternalIPs) {
log.Infof("updating NAT External IP [ %s ] (old value: [ %s ])",
strings.Join(input.NATExternalIPs, " "),
strings.Join(config.NATExternalIPs, " "))
@@ -492,3 +492,38 @@ func TestServiceURLPortIsNormalizedNumerically(t *testing.T) {
require.True(t, SameServiceURL(padded, plain))
}
// A list the profile does not have and a list the request empties are the same
// thing: no NAT mappings, no DNS labels. The profile stores an absent list as
// JSON null and reads it back as a nil slice, while `netbird up` sends the
// emptied list — CleanNATExternalIPs / CleanDNSLabels — whenever the matching
// environment variable is set to nothing, which a deployment template does by
// default. Judging nil and empty as different made the gate refuse that start,
// which is the very deadlock this branch exists to remove, on another field.
func TestWouldChangeIgnoresAnEmptiedListThatWasAlreadyAbsent(t *testing.T) {
path := filepath.Join(t.TempDir(), "lists.json")
_, err := UpdateOrCreateConfig(ConfigInput{ConfigPath: path, ManagementURL: DefaultManagementURL})
require.NoError(t, err)
stored, err := GetExistingConfig(path)
require.NoError(t, err)
require.Nil(t, stored.NATExternalIPs, "the fixture is only useful while the stored list is absent")
require.Nil(t, stored.DNSLabels)
changed, err := stored.WouldChange(ConfigInput{NATExternalIPs: make([]string, 0)})
require.NoError(t, err)
require.False(t, changed, "emptying a NAT list the profile never had is not a change")
changed, err = stored.WouldChange(ConfigInput{DNSLabels: domain.List{}})
require.NoError(t, err)
require.False(t, changed, "emptying a DNS label list the profile never had is not a change")
// A list that does hold something still moves when the request empties it.
withEntries, err := UpdateConfig(ConfigInput{ConfigPath: path, NATExternalIPs: []string{"1.2.3.4"}})
require.NoError(t, err)
require.Equal(t, []string{"1.2.3.4"}, withEntries.NATExternalIPs)
changed, err = withEntries.WouldChange(ConfigInput{NATExternalIPs: make([]string, 0)})
require.NoError(t, err)
require.True(t, changed, "clearing a NAT list that had an entry is a change")
}