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.

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

Slight improvement of the ACL error message #8805

Merged
merged 6 commits into from
Sep 22, 2021

Conversation

frouioui
Copy link
Member

@frouioui frouioui commented Sep 13, 2021

Description

This pull request improves the ACL error message. The original error message was constructed as follows:

"table acl error: <USERNAME> [<GROUPS>] cannot run <QUERY TYPE> on table <TABLE NAME>"

The first suggestion made by @deepthi, and pushed to this PR via d0db975, looks as follows:

"authorization error: user <USERNAME> is not authorized to run <QUERY TYPE> on table <TABLE NAME>"

Another suggestion which was to use MySQL's error message would look as follows:

"<QUERY TYPE> command denied to user '<USERNAME>' for table '<TABLE NAME>'"

This pull request is open to suggestions.

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

@frouioui frouioui added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving labels Sep 13, 2021
@mattlord
Copy link
Contributor

mattlord commented Sep 13, 2021

IMO, the more closely we mimic mysqld from the client's perspective the better -- so I'd vote for doing that here. So perhaps:

SELECT command denied to user 'myappusr' for table 'foo' (ACL check error)

That way we also note the internal vitess area in parenthesis.

Signed-off-by: Florent Poinsard <[email protected]>
@frouioui
Copy link
Member Author

@mattlord, that's a good idea. I've applied the suggestion through 498f1dd.

Minor difference with MySQL is that the plan name is printed in pascal case instead of upper case. This is meant to avoid long and odd strings like SHOWMIGRATIONLOGS, and instead, print ShowMigrationLogs.

@frouioui frouioui marked this pull request as ready for review September 14, 2021 11:01
@deepthi
Copy link
Member

deepthi commented Sep 20, 2021

@frouioui unit_race seems to be flaky. Can we move it to self-hosted runner?
cc @GuptaManan100

Copy link

@Phanatic Phanatic left a comment

Choose a reason for hiding this comment

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

Looks great! I left a comment about retaining Groups information in the error message.

go/vt/vttablet/tabletserver/query_executor.go Outdated Show resolved Hide resolved
@frouioui
Copy link
Member Author

@frouioui unit_race seems to be flaky. Can we move it to self-hosted runner?
cc @GuptaManan100

Created #8854 to migrate this test to our self-hosted runners.

@frouioui frouioui requested a review from deepthi September 22, 2021 05:42
@harshit-gangal harshit-gangal merged commit b6e33ff into vitessio:main Sep 22, 2021
@harshit-gangal harshit-gangal deleted the improve-acl-errors branch September 22, 2021 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants