Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move predicate err check earlier, inside RetryWithRelogin #13368

Merged
merged 12 commits into from
Jun 22, 2022
Merged

Move predicate err check earlier, inside RetryWithRelogin #13368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cfe3078
Move predicate err check earlier, inside RetryWithRelogin
kimlisa Jun 10, 2022
c01cc34
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 17, 2022
1992bc6
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 17, 2022
cb0f01b
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 18, 2022
ad09e60
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 21, 2022
18e16b0
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 21, 2022
9981faa
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 21, 2022
6d48e83
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 21, 2022
898ac3b
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 21, 2022
a6824b0
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 21, 2022
a174a17
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 22, 2022
d39a200
Merge branch 'master' into lisa/bug/resource-query-on-error
kimlisa Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/client/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,10 @@ func RetryWithRelogin(ctx context.Context, tc *TeleportClient, fn func() error)
return nil
}

if utils.IsPredicateError(err) {
return trace.Wrap(utils.PredicateError{Err: err})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's already a predicate error, why do you have to wrap it in another predicate error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if !IsErrorResolvableWithRelogin(err) {
return trace.Wrap(err)
}
Expand Down
3 changes: 0 additions & 3 deletions tool/tsh/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ func onListDatabases(cf *CLIConf) error {
return trace.Wrap(err)
})
if err != nil {
if utils.IsPredicateError(err) {
return trace.Wrap(utils.PredicateError{Err: err})
}
return trace.Wrap(err)
}

Expand Down
3 changes: 0 additions & 3 deletions tool/tsh/kube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,9 +963,6 @@ func fetchKubeClusters(ctx context.Context, tc *client.TeleportClient) (teleport
}
return nil
}
if utils.IsPredicateError(err) {
return trace.Wrap(utils.PredicateError{Err: err})
}
return trace.Wrap(err)
}

Expand Down
6 changes: 0 additions & 6 deletions tool/tsh/tsh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,9 +1646,6 @@ func onListNodes(cf *CLIConf) error {
return err
})
if err != nil {
if utils.IsPredicateError(err) {
return trace.Wrap(utils.PredicateError{Err: err})
}
return trace.Wrap(err)
}
sort.Slice(nodes, func(i, j int) bool {
Expand Down Expand Up @@ -3528,9 +3525,6 @@ func onApps(cf *CLIConf) error {
return err
})
if err != nil {
if utils.IsPredicateError(err) {
return trace.Wrap(utils.PredicateError{Err: err})
}
return trace.Wrap(err)
}

Expand Down