From 682b2de549c1844a4e9b3edf19b9730eb7053a51 Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 15:56:32 +0200 Subject: [PATCH] [client] Fail `netbird up` when the daemon refuses the settings update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the update-settings kill switch on, `netbird up --enable-rosenpass` connected and said almost nothing: SetConfig refused the change, the CLI downgraded that to a warning, and Login carries no rosenpass field to apply, so the flag was silently dropped. The setting stayed disabled, which is the point of the switch, but the caller was never told their request had been ignored. The refusal now travels as codes.FailedPrecondition instead of codes.Unavailable, and the CLI fails on it. Unavailable means "the daemon cannot serve this call", which is why the CLI downgraded it and why client/ui/services reads it as an unreachable daemon — both wrong for a daemon that answered and refused. FailedPrecondition also matches what the MDM gate already returns for a managed field, so both refusals are now one class of error, and it is added to the login backoff's early-exit codes so a refused login stops instead of retrying for 30s. This does not put the container back in the deadlock: with the value-aware gate, a client restating its own configuration is not refused at all, so nothing reaches this path unless a real change was asked for. --- client/cmd/up.go | 35 ++++++++++--- client/cmd/up_setconfig_refusal_test.go | 57 ++++++++++++++++++++++ client/server/server.go | 11 +++-- client/server/update_settings_gate_test.go | 12 ++--- 4 files changed, 99 insertions(+), 16 deletions(-) create mode 100644 client/cmd/up_setconfig_refusal_test.go diff --git a/client/cmd/up.go b/client/cmd/up.go index 6812cf88b..e0dde3863 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -352,13 +352,17 @@ func runInDaemonMode(ctx context.Context, cmd *cobra.Command, pm *profilemanager // set the new config req := setupSetConfigReq(customDNSAddressConverted, cmd, activeProf.ID.String(), username.Username) if _, err := client.SetConfig(ctx, req); err != nil { - if st, ok := gstatus.FromError(err); ok && st.Code() == codes.Unavailable { - // Report what the daemon said rather than asserting why: this code - // covers both a refused update and a daemon that became - // unreachable. Claiming the method was missing, as this used to, - // sent people looking for a version mismatch that was not there. - log.Warnf("the daemon did not apply the settings update: %s", st.Message()) - } else { + switch reason, refused := refusedSettingsUpdate(err); { + case refused: + // Failing here is the point: carrying on would connect while + // silently dropping the settings the caller asked for, since + // nothing further down the line applies them. + return fmt.Errorf("the daemon refused the settings update: %s", reason) + case gstatus.Code(err) == codes.Unavailable: + // The daemon cannot serve the method at all, which is what this + // code means; an older daemon without it lands here. + log.Warnf("the daemon did not apply the settings update: %s", gstatus.Convert(err).Message()) + default: return daemonCallError("call service setConfig method", err) } } @@ -402,6 +406,7 @@ func doDaemonUp(ctx context.Context, cmd *cobra.Command, client proto.DaemonServ if s, ok := gstatus.FromError(backOffErr); ok && (s.Code() == codes.InvalidArgument || s.Code() == codes.PermissionDenied || s.Code() == codes.NotFound || + s.Code() == codes.FailedPrecondition || s.Code() == codes.Unimplemented) { loginErr = backOffErr return nil @@ -471,6 +476,22 @@ func setSSHSetConfigFields(req *proto.SetConfigRequest, cmd *cobra.Command) { } } +// refusedSettingsUpdate reports whether err is the daemon refusing the settings +// a request carried — the update-settings kill switch, or a field an MDM policy +// manages — and returns the reason it gave. +// +// The distinction that matters is against codes.Unavailable, which means the +// daemon cannot serve the call: that one is worth a warning, because an older +// daemon without the method lands there and the rest of `netbird up` still +// works. A refusal is not, because the settings would be silently dropped. +func refusedSettingsUpdate(err error) (string, bool) { + st, ok := gstatus.FromError(err) + if !ok || st.Code() != codes.FailedPrecondition { + return "", false + } + return st.Message(), true +} + func setupSetConfigReq(customDNSAddressConverted []byte, cmd *cobra.Command, profileName, username string) *proto.SetConfigRequest { var req proto.SetConfigRequest req.ProfileName = profileName diff --git a/client/cmd/up_setconfig_refusal_test.go b/client/cmd/up_setconfig_refusal_test.go new file mode 100644 index 000000000..38914f816 --- /dev/null +++ b/client/cmd/up_setconfig_refusal_test.go @@ -0,0 +1,57 @@ +package cmd + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" +) + +// A refused settings update has to fail `netbird up`, or a caller that asked +// for a setting the daemon will not apply connects as if it had been applied. +// The daemon being unable to serve the call is the case that stays a warning. +func TestRefusedSettingsUpdate(t *testing.T) { + tests := []struct { + name string + err error + wantRefused bool + }{ + { + name: "the kill switch refused the change", + err: gstatus.Errorf(codes.FailedPrecondition, "update settings are disabled, you cannot use this feature without update settings enabled"), + wantRefused: true, + }, + { + name: "an MDM policy manages the field", + err: gstatus.Errorf(codes.FailedPrecondition, "fields managed by MDM policy: managementURL"), + wantRefused: true, + }, + { + name: "the daemon cannot serve the call", + err: gstatus.Errorf(codes.Unavailable, "connection refused"), + wantRefused: false, + }, + { + name: "any other RPC failure", + err: gstatus.Errorf(codes.Internal, "boom"), + wantRefused: false, + }, + { + name: "not a status error at all", + err: errors.New("boom"), + wantRefused: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reason, refused := refusedSettingsUpdate(tt.err) + require.Equal(t, tt.wantRefused, refused) + if tt.wantRefused { + require.Equal(t, gstatus.Convert(tt.err).Message(), reason, "the daemon's reason must reach the caller") + } + }) + } +} diff --git a/client/server/server.go b/client/server/server.go index 5807cff64..cee1e7815 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -59,6 +59,11 @@ const ( errRestoreResidualState = "failed to restore residual state: %v" errProfilesDisabled = "profiles are disabled, you cannot use this feature without profiles enabled" + // errUpdateSettingsDisabled is returned with codes.FailedPrecondition, not + // codes.Unavailable: the daemon answered, and it refused. Unavailable means + // "the daemon cannot serve this", which is why the CLI downgrades it to a + // warning and the GUI reads it as an unreachable daemon — both wrong for a + // refusal the caller has to act on. errUpdateSettingsDisabled = "update settings are disabled, you cannot use this feature without update settings enabled" errNetworksDisabled = "network selection is disabled by the administrator" ) @@ -488,7 +493,7 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques // their login too, which left a client configured by environment // (NB_MANAGEMENT_URL and friends) unable to come up at all. if s.checkUpdateSettingsDisabled() && configChangeRequested(stored, config) { - return nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) + return nil, gstatus.Errorf(codes.FailedPrecondition, errUpdateSettingsDisabled) } // MDM gate: refuse the whole request if any of its fields is enforced @@ -648,7 +653,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro // that is what keeps a re-login, or a container restart carrying // NB_MANAGEMENT_URL, working with the kill switch on. if s.checkUpdateSettingsDisabled() && configChangeRequested(stored, loginOverridesInput(msg)) { - return nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) + return nil, gstatus.Errorf(codes.FailedPrecondition, errUpdateSettingsDisabled) } policy := loadMDMPolicy() @@ -2663,7 +2668,7 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. // authoritative check, and it is the last read before persistLoginOverrides // writes. if s.checkUpdateSettingsDisabled() && configChangeRequested(stored, loginOverridesInput(msg)) { - return nil, nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) + return nil, nil, gstatus.Errorf(codes.FailedPrecondition, errUpdateSettingsDisabled) } s.mutex.Lock() diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go index e0ea8bcf9..812ef3554 100644 --- a/client/server/update_settings_gate_test.go +++ b/client/server/update_settings_gate_test.go @@ -62,7 +62,7 @@ func TestSetConfig_ChangingASettingIsRefused(t *testing.T) { ManagementUrl: "https://mgmt.elsewhere.example:443", }) require.Error(t, err, "moving the management URL is a settings change") - require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + require.Equal(t, codes.FailedPrecondition, gstatus.Code(err), "want the update-settings refusal, got %v", err) cfg, err := profilemanager.GetExistingConfig(cfgPath) require.NoError(t, err) @@ -83,7 +83,7 @@ func TestSetConfig_SingleDivergingFieldIsRefused(t *testing.T) { RosenpassEnabled: &rosenpass, }) require.Error(t, err, "enabling Rosenpass is a settings change") - require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + require.Equal(t, codes.FailedPrecondition, gstatus.Code(err), "want the update-settings refusal, got %v", err) } // With the switch off, the same diverging request goes through: the gate must @@ -119,7 +119,7 @@ func TestLogin_ChangingTheManagementURLIsRefused(t *testing.T) { ManagementUrl: "https://mgmt.elsewhere.example:443", }) require.Error(t, err, "moving the management URL through Login is a settings change") - require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + require.Equal(t, codes.FailedPrecondition, gstatus.Code(err), "want the update-settings refusal, got %v", err) // "Refused before it can touch daemon state" is the contract, so check the // state as well as the error. @@ -252,7 +252,7 @@ func TestSetConfig_RefusedRequestLeavesTheConfigFileUntouched(t *testing.T) { ManagementUrl: "https://mgmt.elsewhere.example:443", }) require.Error(t, err) - require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + require.Equal(t, codes.FailedPrecondition, gstatus.Code(err), "want the update-settings refusal, got %v", err) after, err := os.ReadFile(cfgPath) require.NoError(t, err) @@ -305,7 +305,7 @@ func TestLogin_RestatingTheStoredConfigPassesTheGate(t *testing.T) { ManagementUrl: storedManagementURL, }) if err != nil { - require.NotEqual(t, codes.Unavailable, gstatus.Code(err), + require.NotEqual(t, codes.FailedPrecondition, gstatus.Code(err), "the gate refused a login that changes nothing: %v", err) require.NotContains(t, err.Error(), "update settings are disabled", "the gate refused a login that changes nothing: %v", err) @@ -352,7 +352,7 @@ func TestLogin_ChangeThatAppearsMidRequestIsRefused(t *testing.T) { ManagementUrl: storedManagementURL, }) require.Error(t, err, "the login became a settings change before it was written") - require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + require.Equal(t, codes.FailedPrecondition, gstatus.Code(err), "want the update-settings refusal, got %v", err) require.False(t, cancelled, "the refused login cancelled the login already in progress") stored, err := profilemanager.GetExistingConfig(targetPath)