diff --git a/client/cmd/login.go b/client/cmd/login.go index 2a4172cb6..a41d5c51a 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -9,8 +9,6 @@ import ( log "github.com/sirupsen/logrus" "github.com/spf13/cobra" "golang.org/x/term" - "google.golang.org/grpc/codes" - gstatus "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/internal/auth" @@ -144,10 +142,7 @@ func doDaemonLogin(ctx context.Context, cmd *cobra.Command, providedSetupKey str err = WithBackOff(func() error { var backOffErr error loginResp, backOffErr = client.Login(ctx, &loginRequest) - if s, ok := gstatus.FromError(backOffErr); ok && (s.Code() == codes.InvalidArgument || - s.Code() == codes.PermissionDenied || - s.Code() == codes.NotFound || - s.Code() == codes.Unimplemented) { + if terminalLoginError(backOffErr) { loginErr = backOffErr return nil } diff --git a/client/cmd/root.go b/client/cmd/root.go index be6479440..b599ca859 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -20,6 +20,8 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/anonymize" daddr "github.com/netbirdio/netbird/client/internal/daemonaddr" @@ -285,6 +287,35 @@ func DialClientGRPCServer(ctx context.Context, addr string) (*grpc.ClientConn, e return grpc.DialContext(ctx, target, opts...) } +// terminalLoginError reports whether a Login failure is final, so the backoff +// cycle stops and the caller is told what the daemon said instead of "login +// backoff cycle failed" thirty seconds later. Retrying cannot change any of +// these answers: the request is malformed, the caller is not allowed, the +// target does not exist, a precondition on the daemon refuses it (the +// update-settings kill switch, an MDM-managed field), or the method is not +// implemented. +// +// Both `netbird up` and `netbird login` run Login through the backoff, and +// they each carried their own copy of this list — which is how one of them +// ended up retrying a refusal the other treated as final. +func terminalLoginError(err error) bool { + s, ok := gstatus.FromError(err) + if !ok { + return false + } + + switch s.Code() { + case codes.InvalidArgument, + codes.PermissionDenied, + codes.NotFound, + codes.FailedPrecondition, + codes.Unimplemented: + return true + default: + return false + } +} + // WithBackOff execute function in backoff cycle. func WithBackOff(bf func() error) error { return backoff.RetryNotify(bf, CLIBackOffSettings, func(err error, duration time.Duration) { diff --git a/client/cmd/up.go b/client/cmd/up.go index e0dde3863..d821af8bb 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -403,11 +403,7 @@ func doDaemonUp(ctx context.Context, cmd *cobra.Command, client proto.DaemonServ err = WithBackOff(func() error { var backOffErr error loginResp, backOffErr = client.Login(ctx, loginRequest) - 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) { + if terminalLoginError(backOffErr) { loginErr = backOffErr return nil } diff --git a/client/cmd/up_setconfig_refusal_test.go b/client/cmd/up_setconfig_refusal_test.go index 38914f816..74623e434 100644 --- a/client/cmd/up_setconfig_refusal_test.go +++ b/client/cmd/up_setconfig_refusal_test.go @@ -55,3 +55,30 @@ func TestRefusedSettingsUpdate(t *testing.T) { }) } } + +// Both `netbird up` and `netbird login` drive Login through the backoff cycle, +// and a final answer has to stop it: retrying a refusal only replaces the +// daemon's reason with "login backoff cycle failed" thirty seconds later. +func TestTerminalLoginError(t *testing.T) { + tests := []struct { + name string + err error + wantTerminal bool + }{ + {name: "settings refused by the kill switch", err: gstatus.Errorf(codes.FailedPrecondition, "update settings are disabled"), wantTerminal: true}, + {name: "field managed by MDM", err: gstatus.Errorf(codes.FailedPrecondition, "fields managed by MDM policy: managementURL"), wantTerminal: true}, + {name: "caller not allowed", err: gstatus.Errorf(codes.PermissionDenied, "nope"), wantTerminal: true}, + {name: "malformed request", err: gstatus.Errorf(codes.InvalidArgument, "nope"), wantTerminal: true}, + {name: "profile not found", err: gstatus.Errorf(codes.NotFound, "nope"), wantTerminal: true}, + {name: "method missing on an older daemon", err: gstatus.Errorf(codes.Unimplemented, "nope"), wantTerminal: true}, + {name: "daemon unreachable, worth retrying", err: gstatus.Errorf(codes.Unavailable, "connection refused"), wantTerminal: false}, + {name: "transient internal failure", err: gstatus.Errorf(codes.Internal, "boom"), wantTerminal: false}, + {name: "not a status error", err: errors.New("boom"), wantTerminal: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.wantTerminal, terminalLoginError(tt.err)) + }) + } +}