diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 9218a3bfc..ac1b90a62 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -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, " ")) diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index 7a83dc2fe..6b140030f 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -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") +}