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)