Move predicate err check earlier, inside RetryWithRelogin #13368
Conversation
kimlisa
requested review from
zmb3
and removed request for
alex-kovoy and
gzdunek
github-actions
bot
added
the
tsh
kimlisa
force-pushed
the
lisa/bug/resource-query-on-error
branch
from
f5bc9f1 to
cfe3078
Compare
| @@ -714,6 +714,10 @@ func RetryWithRelogin(ctx context.Context, tc *TeleportClient, fn func() error) | |||
| return nil | |||
| } | |||
|
|
|||
| if utils.IsPredicateError(err) { | |||
| return trace.Wrap(utils.PredicateError{Err: err}) | |||
There was a problem hiding this comment.
If it's already a predicate error, why do you have to wrap it in another predicate error?
There was a problem hiding this comment.
the IsPredicateError looks for keyword in the error to determine if its predicate, then we use a wrapper to add a link to the predicate documentation:
so if this is the error that returned:
failed to parse predicate expression: identifier "la" is not defined
it will get formatted to this:
ERROR: failed to parse predicate expression: identifier "la" is not defined
Check syntax at https://goteleport.com/docs/setup/reference/predicate-language/#resource-filtering
There was a problem hiding this comment.
It feels to me that RetryWithRelogin shouldn't be concerned with doing special stuff with errors that have nothing to do with auth. 🤔
Is there a single place which could wrap the predicate error before RetryWithRelogin catches it? Then we could change IsErrorResolvableWithRelogin to return false for predicate errors and that would be it.
If that's not possible, then perhaps another solution would be to make IsErrorResolvableWithRelogin return false for predicate errors and then wrap them in specific callsites, as it was done before this PR.
There was a problem hiding this comment.
Sorry for reviving this thread, I just noticed that this PR was made 2 weeks ago. Still, long term I think it'd be worthwhile to change it at some point. I think we would have to tackle this anyway when adding support for those queries to Connect.
Fixes a bug in tsh ls resources, where users were prompted to re-login when it was only a predicate query error. `RetryWithRelogin` now aborts the re-login attempt if the error is of type predicate.
… (#13747) Fixes a bug in tsh ls resources, where users were prompted to re-login when it was only a predicate query error. `RetryWithRelogin` now aborts the re-login attempt if the error is of type predicate.
fixes #13163
Description
In tsh, the list resources
nodes,apps,dbs, andkubesare wrapped inRetryWithRelogin, so before the predicate error reaches the predicate error check, it prompts user to relogin and then shows predicate error.RetryWithReloginnow aborts the relogin attempt if the error is of type predicate.