sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-09-12 17:59:06 +02:00
Code Issues Packages Projects Releases Wiki Activity

[client] Treat an unset optional field as its default when diffing a config

Browse Source 
Seven Config fields mean "the effective default" when they hold no value:
the five SSH toggles, the SSH JWT cache TTL, and the network monitor. Every
consumer already reads a nil as that default, but apply() diffed them by
presence — `config.X == nil || *input.X != *config.X` — so an input restating
the default counted as a change.

That made the update-settings gate refuse `netbird up` outright. The CLI
sends every flag whose value came from an environment variable
(SetFlagsFromEnvVars goes through pflag's FlagSet.Set, which marks the flag
Changed), and the config a plain login writes leaves all seven unset, so a
container configured with, say, NB_ENABLE_SSH_ROOT=false restated a default
the file held as null on every start and was answered with
FailedPrecondition.

apply() now resolves the seven up front, the way it already did for
ServerSSHAllowed and RemoteJobsAllowed, which also repairs such a profile on
its next write. With the values named, the comparisons below diff values
instead of presence, so their nil branches are gone.

The network monitor keeps its platform default — on for windows and darwin —
and naming it as false elsewhere is what createEngineConfig already read a
nil to be. getJWTCacheTTL reaches the same 0 through its own default, and
Android's GetEnableSSH* getters already answered nil with false.
This commit is contained in:
riccardom
2026-09-07 16:57:34 +02:00
parent 0bfe2f6ead
commit aac936a810
2 changed files with 126 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/internal/profilemanager/config.go
+44 -16
View File
@@ -378,6 +378,43 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
}
// Fields whose nil means "the effective default" rather than "no opinion":
// every consumer already reads a nil as the value resolved here — the SSH
// toggles in engine_ssh.go, the network monitor in createEngineConfig — so
// naming it changes nothing about what runs.
//
// Resolving them up front is what lets the comparisons below diff values
// instead of presence. While they stayed nil, an input restating the
// default read as a change, and since the CLI sends every flag set through
// an environment variable on each `netbird up`, a client configured with
// NB_ENABLE_SSH_ROOT=false restated it every time and the update-settings
// gate refused the restatement.
for _, field := range []**bool{
&config.EnableSSHRoot,
&config.EnableSSHSFTP,
&config.EnableSSHLocalPortForwarding,
&config.EnableSSHRemotePortForwarding,
&config.DisableSSHAuth,
} {
if *field == nil {
*field = util.False()
updated = true
}
}
if config.SSHJWTCacheTTL == nil {
// A zero TTL disables the JWT cache, which is what no value meant.
config.SSHJWTCacheTTL = new(int)
updated = true
}
if config.NetworkMonitor == nil {
// network monitoring is on by default on windows and darwin clients
enabled := runtime.GOOS == "windows" || runtime.GOOS == "darwin"
config.NetworkMonitor = &enabled
updated = true
}
if config.ManagementURL == nil {
log.Infof("using default Management URL %s", DefaultManagementURL)
config.ManagementURL, err = parseURL("Management URL", DefaultManagementURL)
@@ -482,21 +519,12 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.NetworkMonitor != nil && (config.NetworkMonitor == nil || *input.NetworkMonitor != *config.NetworkMonitor) {
if input.NetworkMonitor != nil && *input.NetworkMonitor != *config.NetworkMonitor {
log.Infof("switching Network Monitor to %t", *input.NetworkMonitor)
config.NetworkMonitor = input.NetworkMonitor
updated = true
}
if config.NetworkMonitor == nil {
// enable network monitoring by default on windows and darwin clients
if runtime.GOOS == "windows" || runtime.GOOS == "darwin" {
enabled := true
config.NetworkMonitor = &enabled
updated = true
}
}
if input.CustomDNSAddress != nil && string(input.CustomDNSAddress) != config.CustomDNSAddress {
log.Infof("updating custom DNS address %#v (old value %#v)",
string(input.CustomDNSAddress), config.CustomDNSAddress)
@@ -565,7 +593,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.EnableSSHRoot != nil && (config.EnableSSHRoot == nil || *input.EnableSSHRoot != *config.EnableSSHRoot) {
if input.EnableSSHRoot != nil && *input.EnableSSHRoot != *config.EnableSSHRoot {
if *input.EnableSSHRoot {
log.Infof("enabling SSH root login")
} else {
@@ -575,7 +603,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.EnableSSHSFTP != nil && (config.EnableSSHSFTP == nil || *input.EnableSSHSFTP != *config.EnableSSHSFTP) {
if input.EnableSSHSFTP != nil && *input.EnableSSHSFTP != *config.EnableSSHSFTP {
if *input.EnableSSHSFTP {
log.Infof("enabling SSH SFTP subsystem")
} else {
@@ -585,7 +613,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.EnableSSHLocalPortForwarding != nil && (config.EnableSSHLocalPortForwarding == nil || *input.EnableSSHLocalPortForwarding != *config.EnableSSHLocalPortForwarding) {
if input.EnableSSHLocalPortForwarding != nil && *input.EnableSSHLocalPortForwarding != *config.EnableSSHLocalPortForwarding {
if *input.EnableSSHLocalPortForwarding {
log.Infof("enabling SSH local port forwarding")
} else {
@@ -595,7 +623,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.EnableSSHRemotePortForwarding != nil && (config.EnableSSHRemotePortForwarding == nil || *input.EnableSSHRemotePortForwarding != *config.EnableSSHRemotePortForwarding) {
if input.EnableSSHRemotePortForwarding != nil && *input.EnableSSHRemotePortForwarding != *config.EnableSSHRemotePortForwarding {
if *input.EnableSSHRemotePortForwarding {
log.Infof("enabling SSH remote port forwarding")
} else {
@@ -605,7 +633,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.DisableSSHAuth != nil && (config.DisableSSHAuth == nil || *input.DisableSSHAuth != *config.DisableSSHAuth) {
if input.DisableSSHAuth != nil && *input.DisableSSHAuth != *config.DisableSSHAuth {
if *input.DisableSSHAuth {
log.Infof("disabling SSH authentication")
} else {
@@ -615,7 +643,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.SSHJWTCacheTTL != nil && (config.SSHJWTCacheTTL == nil || *input.SSHJWTCacheTTL != *config.SSHJWTCacheTTL) {
if input.SSHJWTCacheTTL != nil && *input.SSHJWTCacheTTL != *config.SSHJWTCacheTTL {
log.Infof("updating SSH JWT cache TTL to %d seconds", *input.SSHJWTCacheTTL)
config.SSHJWTCacheTTL = input.SSHJWTCacheTTL
updated = true
client/internal/profilemanager/config_would_change_test.go
+82
View File
@@ -1,8 +1,10 @@
package profilemanager
import (
"encoding/json"
"os"
"path/filepath"
"runtime"
"testing"
"github.com/stretchr/testify/require"
@@ -26,6 +28,8 @@ func seededConfig(t *testing.T) *Config {
func strPointer(s string) *string { return &s }
func intPtr(i int) *int { return &i }
func TestWouldChange(t *testing.T) {
tests := []struct {
name string
@@ -366,6 +370,84 @@ func TestAdminURLPathIsPartOfTheIdentity(t *testing.T) {
require.Equal(t, "https://app.example.com:443/other", updated.AdminURL.String(), "the new panel path was not persisted")
}
// unsetOnDisk rewrites the stored config so the named fields carry a JSON null,
// which is how a profile that was never asked about them looks on disk.
func unsetOnDisk(t *testing.T, path string, fields ...string) {
t.Helper()
raw, err := os.ReadFile(path)
require.NoError(t, err)
var stored map[string]json.RawMessage
require.NoError(t, json.Unmarshal(raw, &stored))
for _, field := range fields {
_, present := stored[field]
require.True(t, present, "%s is not a field of the stored config", field)
stored[field] = json.RawMessage("null")
}
rewritten, err := json.Marshal(stored)
require.NoError(t, err)
require.NoError(t, os.WriteFile(path, rewritten, 0600))
}
// Seven fields mean "the effective default" when they hold no value, and the
// config a plain login writes leaves every one of them unset. Restating that
// default is asking for no change — and the CLI restates it on every `netbird
// up`, because a flag set through an environment variable is a flag pflag
// reports as Changed. Judging those restatements as changes made the
// update-settings gate refuse `netbird up` outright for a client configured
// through the environment, which is the shape of a Kubernetes deployment.
func TestWouldChangeIgnoresRestatedDefaultsOfUnsetFields(t *testing.T) {
networkMonitorDefault := runtime.GOOS == "windows" || runtime.GOOS == "darwin"
tests := []struct {
field string
theDefault ConfigInput
theOtherWay ConfigInput
}{
{"EnableSSHRoot",
ConfigInput{EnableSSHRoot: boolPtr(false)}, ConfigInput{EnableSSHRoot: boolPtr(true)}},
{"EnableSSHSFTP",
ConfigInput{EnableSSHSFTP: boolPtr(false)}, ConfigInput{EnableSSHSFTP: boolPtr(true)}},
{"EnableSSHLocalPortForwarding",
ConfigInput{EnableSSHLocalPortForwarding: boolPtr(false)}, ConfigInput{EnableSSHLocalPortForwarding: boolPtr(true)}},
{"EnableSSHRemotePortForwarding",
ConfigInput{EnableSSHRemotePortForwarding: boolPtr(false)}, ConfigInput{EnableSSHRemotePortForwarding: boolPtr(true)}},
{"DisableSSHAuth",
ConfigInput{DisableSSHAuth: boolPtr(false)}, ConfigInput{DisableSSHAuth: boolPtr(true)}},
{"SSHJWTCacheTTL",
ConfigInput{SSHJWTCacheTTL: intPtr(0)}, ConfigInput{SSHJWTCacheTTL: intPtr(300)}},
{"NetworkMonitor",
ConfigInput{NetworkMonitor: boolPtr(networkMonitorDefault)}, ConfigInput{NetworkMonitor: boolPtr(!networkMonitorDefault)}},
}
for _, tt := range tests {
t.Run(tt.field, func(t *testing.T) {
path := filepath.Join(t.TempDir(), "unset.json")
_, err := UpdateOrCreateConfig(ConfigInput{
ConfigPath: path,
ManagementURL: "https://api.netbird.io:443",
})
require.NoError(t, err)
unsetOnDisk(t, path, tt.field)
cfg, err := GetExistingConfig(path)
require.NoError(t, err)
changed, err := cfg.WouldChange(tt.theDefault)
require.NoError(t, err)
require.False(t, changed, "restating the default of an unset %s was judged a change", tt.field)
// The gate still has to refuse a request that does ask for something.
changed, err = cfg.WouldChange(tt.theOtherWay)
require.NoError(t, err)
require.True(t, changed, "asking for a non-default %s is a change", tt.field)
})
}
}
// A zero-padded port addresses the same port.
func TestServiceURLPortIsNormalizedNumerically(t *testing.T) {
padded, err := ParseServiceURL("padded", "https://mgmt.example.com:0443")