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] Gather the optional-field defaults into one function

Browse Source 
Resolving an unset optional field was spread over five places: the two
values newConfigSkeleton pre-sets, the block this branch added for the SSH
toggles, the network monitor's own if, the `else if` tails of
ServerSSHAllowed and RemoteJobsAllowed, and a trailing if for
DisableNotifications several hundred lines further down. Reading apply() left
no single answer to "what does this field default to, and who decides".

They now live in Config.resolveUnsetDefaults, which apply() calls before it
compares anything — the ordering being the point, since it is what lets
every comparison below diff values instead of presence. The comparisons for
ServerSSHAllowed, RemoteJobsAllowed and DisableNotifications lose their
`config.X == nil ||` clauses accordingly, as the other six already had.

newConfigSkeleton keeps its two, and that is the one asymmetry worth naming:
ServerSSHAllowed defaults to false for a new profile and to true for a
legacy one, and it only works because the skeleton runs first. The doc
comment says so, where before it was implied by the order of two distant
blocks.

Pure refactor. Verified as one: for the four fields whose branches moved,
plus two that did not and the JWT TTL, all 63 combinations of stored value
(nil/false/true) against input value (absent/false/true) produce byte-
identical resolved values and `updated` verdicts before and after.
This commit is contained in:
riccardom
2026-09-08 09:30:46 +02:00
parent 9ff4e6ce70
commit ea8e64e6ef
1 changed files with 83 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/internal/profilemanager/config.go
+83 -59
View File
@@ -307,6 +307,83 @@ func newConfigSkeleton() *Config {
}
}
// resolveUnsetDefaults is the single place where an optional field that carries
// no value gets one, and the only place that states what each of those defaults
// is. apply() runs it before it compares anything, and that ordering is the
// point: with the values named, every comparison below it diffs values instead
// of presence.
//
// Presence-based comparison is what broke `netbird up` for a client configured
// through the environment. These fields mean "the effective default" when they
// hold nothing — every consumer already reads a nil as the value resolved here,
// the SSH toggles in engine_ssh.go and the network monitor in
// createEngineConfig — so naming them changes nothing about what runs. But
// while they stayed nil, an input restating the default read as a change, and
// since the CLI sends every flag whose value came from an environment variable
// on each `netbird up`, a client with NB_ENABLE_SSH_ROOT=false restated it
// every time and the update-settings gate refused it.
//
// Filling a field in is not a settings change, so a caller measuring change
// must not read the returned bool as one: see WouldChange, which runs a pass
// for this and discards its verdict.
//
// ServerSSHAllowed is the one field whose default depends on the config's age.
// A brand-new profile gets false from newConfigSkeleton, which runs before
// this, so what is resolved here is only the legacy case: a config written by a
// version that had no such field keeps SSH on, for backwards compatibility.
func (config *Config) resolveUnsetDefaults() (updated bool) {
// Fields that default to false on every platform.
for _, field := range []**bool{
&config.EnableSSHRoot,
&config.EnableSSHSFTP,
&config.EnableSSHLocalPortForwarding,
&config.EnableSSHRemotePortForwarding,
&config.DisableSSHAuth,
// Remote jobs are an explicit opt-in: unlike SSH, a pre-existing config
// with no value defaults to disabled rather than being turned on.
&config.RemoteJobsAllowed,
} {
if *field == nil {
*field = util.False()
updated = true
}
}
if config.DisableNotifications == nil {
log.Infof("setting notifications to disabled by default")
config.DisableNotifications = util.True()
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.ServerSSHAllowed == nil {
if runtime.GOOS == "android" {
// default to disabled SSH on Android for security
log.Infof("setting SSH server to false by default on Android")
config.ServerSSHAllowed = util.False()
} else {
// enables SSH for configs from old versions to preserve backwards compatibility
log.Infof("falling back to enabled SSH server for pre-existing configuration")
config.ServerSSHAllowed = util.True()
}
updated = true
}
return updated
}
// createNewConfig resolves a new config in memory, with no identity: whoever
// needs the peer's keys calls EnsureIdentity and persists the result, so a read
// that lands on a missing file cannot hand back a config carrying keys that
@@ -379,42 +456,12 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
}
}
// 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)
// Every optional field gets its value here, before anything below compares
// one. See resolveUnsetDefaults for why that ordering is the point.
if config.resolveUnsetDefaults() {
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)
@@ -557,7 +604,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.ServerSSHAllowed != nil && (config.ServerSSHAllowed == nil || *input.ServerSSHAllowed != *config.ServerSSHAllowed) {
if input.ServerSSHAllowed != nil && *input.ServerSSHAllowed != *config.ServerSSHAllowed {
if *input.ServerSSHAllowed {
log.Infof("enabling SSH server")
} else {
@@ -565,20 +612,9 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
}
config.ServerSSHAllowed = input.ServerSSHAllowed
updated = true
} else if config.ServerSSHAllowed == nil {
if runtime.GOOS == "android" {
// default to disabled SSH on Android for security
log.Infof("setting SSH server to false by default on Android")
config.ServerSSHAllowed = util.False()
} else {
// enables SSH for configs from old versions to preserve backwards compatibility
log.Infof("falling back to enabled SSH server for pre-existing configuration")
config.ServerSSHAllowed = util.True()
}
updated = true
}
if input.RemoteJobsAllowed != nil && (config.RemoteJobsAllowed == nil || *input.RemoteJobsAllowed != *config.RemoteJobsAllowed) {
if input.RemoteJobsAllowed != nil && *input.RemoteJobsAllowed != *config.RemoteJobsAllowed {
if *input.RemoteJobsAllowed {
log.Infof("enabling remote jobs")
} else {
@@ -586,11 +622,6 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
}
config.RemoteJobsAllowed = input.RemoteJobsAllowed
updated = true
} else if config.RemoteJobsAllowed == nil {
// Remote jobs are an explicit opt-in: unlike SSH, a pre-existing config
// with no value defaults to disabled rather than being turned on.
config.RemoteJobsAllowed = util.False()
updated = true
}
if input.EnableSSHRoot != nil && *input.EnableSSHRoot != *config.EnableSSHRoot {
@@ -735,7 +766,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if input.DisableNotifications != nil && (config.DisableNotifications == nil || *input.DisableNotifications != *config.DisableNotifications) {
if input.DisableNotifications != nil && *input.DisableNotifications != *config.DisableNotifications {
if *input.DisableNotifications {
log.Infof("disabling notifications")
} else {
@@ -745,13 +776,6 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}
if config.DisableNotifications == nil {
disabled := true
config.DisableNotifications = &disabled
log.Infof("setting notifications to disabled by default")
updated = true
}
// Compared, not just assigned: restating the path a config already holds
// changes nothing, and reporting it as an update makes a caller that
// re-sends its own configuration look like one asking to change it.