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

Basic CREATE USER implementation #704

Merged
merged 2 commits into from
Jan 18, 2022
Merged

Basic CREATE USER implementation #704

merged 2 commits into from
Jan 18, 2022

Conversation

Hydrocharged
Copy link
Contributor

@Hydrocharged Hydrocharged commented Jan 3, 2022

This adds the minimum for the CREATE USER statement to work, in that we have an in-memory representation of the grant tables, which may be accessed either directly (by the mysql database) or through the user statements (of which we only have CREATE USER for now).

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

This seems like a wrong turn, but I could be convinced otherwise.

There's no need to keep all physical grant / user rows in memory all the time. What you need is

  1. A Database interface extension for CheckPrivileges (this is what your analyzer function will call into)
  2. A DatabaseProvider interface extension for GetUsers / GetGrants etc., not sure exactly what's needed there

The table representation is only really relevant when querying the user / grant tables directly. And I don't know if we will ever care if that's fast, I'm totally fine constructing them from the primitives in 2) at runtime as needed. The thing that needs to be fast is auth during analysis, and for that you should provide custom hooks for integrators that they can make as fast as necessary.

This is the same pattern we have followed for all the other mysql system tables: we build them at query time from the information provided by integrators through the interfaces they implement. I don't see a good reason to deviate from that pattern for this.

Let me know if I have the wrong idea here.

sql/in_mem_table/inmem_table_data.go Show resolved Hide resolved
@Hydrocharged Hydrocharged changed the title In-memory table data for Grant Tables Basic CREATE USER implementation Jan 14, 2022
@Hydrocharged Hydrocharged force-pushed the daylon/users-2 branch 2 times, most recently from 8eb702b to 45d9350 Compare January 14, 2022 15:37
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Seems generally fine, hard to give more concrete feedback until I see how it ties into an integrator

sql/core.go Outdated Show resolved Hide resolved
sql/grant_tables/user.go Outdated Show resolved Hide resolved
@Hydrocharged Hydrocharged merged commit 95025d3 into main Jan 18, 2022
@Hydrocharged Hydrocharged deleted the daylon/users-2 branch January 18, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants