mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-24 23:59:08 +02:00
[client] Address the remaining bot findings on PR #7398
- Login logged the active-profile-state error and returned the same cause; the repo's guidelines call for one or the other, and the wrapped error is the one that carries context. (CodeRabbit) - `netbird up` reported a codes.Unavailable SetConfig failure as "the daemon refused the settings update", but that code also covers a daemon that became unreachable. It now reports what the daemon said without asserting why. (cubic-dev-ai) - TestLogin_ChangingTheManagementURLIsRefused asserted the error and nothing else, while "refused before it can touch daemon state" is the contract. It now checks the stored management URL, the in-progress login and the active profile, matching its SetConfig counterpart. (cubic-dev-ai)
This commit is contained in:
+5
-4
@@ -353,10 +353,11 @@ func runInDaemonMode(ctx context.Context, cmd *cobra.Command, pm *profilemanager
|
|||||||
req := setupSetConfigReq(customDNSAddressConverted, cmd, activeProf.ID.String(), username.Username)
|
req := setupSetConfigReq(customDNSAddressConverted, cmd, activeProf.ID.String(), username.Username)
|
||||||
if _, err := client.SetConfig(ctx, req); err != nil {
|
if _, err := client.SetConfig(ctx, req); err != nil {
|
||||||
if st, ok := gstatus.FromError(err); ok && st.Code() == codes.Unavailable {
|
if st, ok := gstatus.FromError(err); ok && st.Code() == codes.Unavailable {
|
||||||
// The daemon refused the settings update, it did not lack the
|
// Report what the daemon said rather than asserting why: this code
|
||||||
// method: reporting the latter sent people looking for a version
|
// covers both a refused update and a daemon that became
|
||||||
// mismatch that was not there.
|
// unreachable. Claiming the method was missing, as this used to,
|
||||||
log.Warnf("the daemon refused the settings update: %s", st.Message())
|
// 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 {
|
} else {
|
||||||
return daemonCallError("call service setConfig method", err)
|
return daemonCallError("call service setConfig method", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -621,7 +621,6 @@ func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest) (profile
|
|||||||
func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*proto.LoginResponse, error) {
|
func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*proto.LoginResponse, error) {
|
||||||
activeProf, err := s.profileManager.GetActiveProfileState()
|
activeProf, err := s.profileManager.GetActiveProfileState()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
log.Errorf("failed to get active profile state: %v", err)
|
|
||||||
return nil, fmt.Errorf("failed to get active profile state: %w", err)
|
return nil, fmt.Errorf("failed to get active profile state: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -107,16 +107,30 @@ func TestSetConfig_ChangeAllowedWhenTheSwitchIsOff(t *testing.T) {
|
|||||||
// way: a login that would move a protected setting is refused before it can
|
// way: a login that would move a protected setting is refused before it can
|
||||||
// touch daemon state.
|
// touch daemon state.
|
||||||
func TestLogin_ChangingTheManagementURLIsRefused(t *testing.T) {
|
func TestLogin_ChangingTheManagementURLIsRefused(t *testing.T) {
|
||||||
s, _, _, username, _ := setupServerWithProfile(t)
|
s, _, profName, username, cfgPath := setupServerWithProfile(t)
|
||||||
s.updateSettingsDisabled = true
|
s.updateSettingsDisabled = true
|
||||||
s.rootCtx = internal.CtxInitState(context.Background())
|
s.rootCtx = internal.CtxInitState(context.Background())
|
||||||
|
|
||||||
|
cancelled := false
|
||||||
|
s.actCancel = func() { cancelled = true }
|
||||||
|
|
||||||
_, err := s.Login(userCtx(), &proto.LoginRequest{
|
_, err := s.Login(userCtx(), &proto.LoginRequest{
|
||||||
Username: &username,
|
Username: &username,
|
||||||
ManagementUrl: "https://mgmt.elsewhere.example:443",
|
ManagementUrl: "https://mgmt.elsewhere.example:443",
|
||||||
})
|
})
|
||||||
require.Error(t, err, "moving the management URL through Login is a settings change")
|
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.Unavailable, 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.
|
||||||
|
cfg, err := profilemanager.GetExistingConfig(cfgPath)
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.Equal(t, storedManagementURL, cfg.ManagementURL.String(), "the refused login moved the management URL")
|
||||||
|
require.False(t, cancelled, "the refused login cancelled the login already in progress")
|
||||||
|
|
||||||
|
active, err := s.profileManager.GetActiveProfileState()
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.Equal(t, profilemanager.ID(profName), active.ID, "the refused login switched the active profile")
|
||||||
}
|
}
|
||||||
|
|
||||||
// seedProfileConfig writes a profile config carrying the given management URL
|
// seedProfileConfig writes a profile config carrying the given management URL
|
||||||
|
|||||||
Reference in New Issue
Block a user