*: maintain the active users for privilege data #56709

tiancaiamao · 2024-10-17T13:15:10Z

What problem does this PR solve?

Issue Number: ref #55563

Problem Summary:

What changed and how does it work?

Add an activeUsers field to the privilege.Handler.
This is a step towrads supporting 2.5M users, later we'll only keep the active users' data in-memory.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2024-10-17T13:15:29Z

Hi @tiancaiamao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-10-17T13:26:29Z

Codecov Report

Attention: Patch coverage is 79.00763% with 55 lines in your changes missing coverage. Please review.

Project coverage is 57.4224%. Comparing base (5ab6738) to head (b8f6092).
Report is 43 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56709         +/-   ##
=================================================
- Coverage   73.3167%   57.4224%   -15.8944%     
=================================================
  Files          1636       1801        +165     
  Lines        453659     647223     +193564     
=================================================
+ Hits         332608     371651      +39043     
- Misses       100674     250234     +149560     
- Partials      20377      25338       +4961

Flag	Coverage Δ
integration	`39.5301% <78.2442%> (?)`
unit	`72.5787% <70.9923%> (+0.0534%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.1883% <ø> (+17.1766%)`	⬆️

tiancaiamao · 2024-10-28T10:18:26Z

/retest

tiprow · 2024-10-28T10:18:50Z

@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pkg/privilege/privileges/cache.go

lance6716 · 2024-10-29T02:48:41Z

pkg/privilege/privileges/privileges.go

 	u := user.Username
 	h := user.Hostname
 	if len(user.AuthUsername) > 0 && len(user.AuthHostname) > 0 {
 		u = user.AuthUsername
 		h = user.AuthHostname
 	}
+	if err := p.Handle.ensureActiveUser(u); err != nil {


I'm not sure about the future usage. Will something happen between ensureActiveUser and Get and evict the active user?

Currently there is no evict operation, evict make thing more complex.

pkg/privilege/privileges/cache.go

lance6716 · 2024-10-29T02:59:59Z

pkg/privilege/privileges/cache.go

+	})
+	ret.buildGlobalMap()
+
+	ret.RoleGraph = diff.RoleGraph


(I will check later if only User and GlobalPriv need dedup, and RoleGraph can be directly used)

RoleGraph is a full load, because I found that the table definitions differ from the others.
Other other tables has the User+Host column, while this table does not have it.

pkg/privilege/privileges/cache.go

pkg/privilege/privilege.go

lcwangchao · 2024-10-29T07:44:17Z

pkg/privilege/privileges/cache.go

@@ -1633,12 +1815,48 @@ func (p *MySQLPrivilege) getAllRoles(user, host string) []*auth.RoleIdentity {

 // Handle wraps MySQLPrivilege providing thread safe access.
 type Handle struct {
+	sctx sessionctx.Context


Use the type sqlexec.RestrictedSQLExecutor if we only use the context to execute some SQLs to reduce the dependency.

sqlexec.RestrictedSQLExecutor is not enough here.
We need something like a sqlexec.RestrictedSQLExecutor pool to avoid the single instance been used by multiple threads. @lcwangchao

If so, keeping a sessionctx.Context inside has the same problem.

pkg/privilege/privileges/tidb_auth_token_test.go

pkg/privilege/privileges/cache.go

lcwangchao · 2024-10-29T09:27:40Z

pkg/privilege/privileges/cache.go

+	columnsPriv  []columnsPrivRecord
+	defaultRoles []defaultRoleRecord
+
+	globalPriv  []globalPrivRecord


Why need to keep a immutable globalPriv and a mutable Global? Any problem to just keep one Global field?

Make the apply diff logic simpler if we're using one single slice representation...
we can copy and paste a lot code and just handle them the same way.

It's possible to keep different representation for different field, or even using Map for almost all field, but we need to add more code.

tiancaiamao · 2024-10-30T01:18:50Z

/retest

tiprow · 2024-10-30T01:19:13Z

@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pkg/privilege/privileges/cache.go

tiancaiamao · 2024-10-30T12:09:59Z

/retest

tiprow · 2024-10-30T12:10:23Z

@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lance6716 · 2024-10-30T12:38:34Z

pkg/privilege/privileges/cache.go

+		return cmp
+	}
+	switch {
+	case x.DB > y.DB:


nit: can use strings.Compare

lance6716 · 2024-10-30T12:41:33Z

pkg/privilege/privileges/cache.go

-	for i := 0; i < len(p.User); i++ {
-		record := &p.User[i]
+func (p *MySQLPrivilege) matchIdentity(sctx sqlexec.RestrictedSQLExecutor, user, host string, skipNameResolve bool) *UserRecord {
+	for i := 0; i < len(p.user); i++ {


nit: for i := range p.User

lance6716 · 2024-10-30T12:42:11Z

pkg/privilege/privileges/cache.go

@@ -1011,8 +1194,8 @@ func (p *MySQLPrivilege) matchIdentity(user, host string, skipNameResolve bool)

 // matchResoureGroup finds an identity to match resource group.
 func (p *MySQLPrivilege) matchResoureGroup(resourceGroupName string) *UserRecord {
-	for i := 0; i < len(p.User); i++ {
-		record := &p.User[i]
+	for i := 0; i < len(p.user); i++ {


lance6716 · 2024-10-30T12:42:16Z

pkg/privilege/privileges/cache.go

@@ -1090,8 +1273,8 @@ func (p *MySQLPrivilege) matchTables(user, host, db, table string) *tablesPrivRe
 }

 func (p *MySQLPrivilege) matchColumns(user, host, db, table, column string) *columnsPrivRecord {
-	for i := 0; i < len(p.ColumnsPriv); i++ {
-		record := &p.ColumnsPriv[i]
+	for i := 0; i < len(p.columnsPriv); i++ {


ti-chi-bot · 2024-10-30T12:43:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, lcwangchao

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lance6716,lcwangchao]
~~pkg/domain/OWNERS~~ [lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-10-30T12:43:57Z

[LGTM Timeline notifier]

Timeline:

2024-10-30 10:42:37.160397932 +0000 UTC m=+432869.999553478: ☑️ agreed by lcwangchao.
2024-10-30 12:43:56.172825283 +0000 UTC m=+440149.011980830: ☑️ agreed by lance6716.

lance6716 · 2024-10-30T12:44:27Z

It's OK to fix my comments in next PR or ignore them @tiancaiamao

tiancaiamao · 2024-11-01T02:27:44Z

This commit could make the login process slower, because now we need to load the users data.
And also some scenario like show grants for some other users.

The general DDL/DML cases should not be affected, this just affects a minority of privileges related.

tiancaiamao added 2 commits October 17, 2024 20:59

*: maintain the active users for privilege data

01376c9

fix lint

ca6f8c6

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2024

tiancaiamao added 4 commits October 17, 2024 22:54

make lint happy

6bbcdb6

fix a type

8f11cd3

handle duplicated columns priv record

c945725

handle role load

8fad2ff

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2024

tiancaiamao added 11 commits October 23, 2024 16:24

fix unit test TestForIssue23387

af91afc

load user for RequestVerificationForUser

1a7454f

fix build

dcc09b2

fix for mysql test t/grant_dynamic

d53bb0a

make lint happy

dd7a9fc

fix unit test TestIssue17794

114b319

make fmt

f0467d3

fix table global priv

11f4a64

fix mysql test t/role

38a62c9

avoid data race when loadTable

86364e1

Merge branch 'master' into active-user

bb4f876

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Oct 28, 2024

tiancaiamao requested review from lcwangchao and lance6716 October 28, 2024 11:02

lance6716 reviewed Oct 29, 2024

View reviewed changes

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 29, 2024

lcwangchao reviewed Oct 29, 2024

View reviewed changes

tiancaiamao added 4 commits October 29, 2024 17:37

address comment

a18e4ba

fix test

bbbea62

fix ci

8322557

revert unrelated changes

de29cfe

tiancaiamao requested review from lcwangchao and lance6716 October 30, 2024 02:19

lance6716 reviewed Oct 30, 2024

View reviewed changes

pkg/privilege/privileges/cache.go Outdated Show resolved Hide resolved

pkg/privilege/privileges/cache.go Outdated Show resolved Hide resolved

address comment

9f4e994

tiancaiamao requested a review from lance6716 October 30, 2024 09:50

address comment

843dbf8

lcwangchao approved these changes Oct 30, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 30, 2024

fix CI build

b8f6092

lance6716 approved these changes Oct 30, 2024

View reviewed changes

ti-chi-bot bot added lgtm approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 30, 2024

ti-chi-bot bot merged commit cf5a617 into pingcap:master Oct 30, 2024
25 checks passed

tiancaiamao added the type/performance label Nov 1, 2024

tiancaiamao deleted the active-user branch November 1, 2024 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: maintain the active users for privilege data #56709

*: maintain the active users for privilege data #56709

tiancaiamao commented Oct 17, 2024 •

edited by bb7133

Loading

tiprow bot commented Oct 17, 2024

codecov bot commented Oct 17, 2024 •

edited

Loading

tiancaiamao commented Oct 28, 2024

tiprow bot commented Oct 28, 2024

lance6716 Oct 29, 2024

tiancaiamao Oct 29, 2024

lance6716 Oct 29, 2024

tiancaiamao Oct 29, 2024

lcwangchao Oct 29, 2024

tiancaiamao Oct 29, 2024

lcwangchao Oct 30, 2024

lcwangchao Oct 29, 2024

tiancaiamao Oct 29, 2024

tiancaiamao commented Oct 30, 2024

tiprow bot commented Oct 30, 2024

tiancaiamao commented Oct 30, 2024

tiprow bot commented Oct 30, 2024

lance6716 Oct 30, 2024

lance6716 Oct 30, 2024

lance6716 Oct 30, 2024

lance6716 Oct 30, 2024

ti-chi-bot bot commented Oct 30, 2024

ti-chi-bot bot commented Oct 30, 2024

lance6716 commented Oct 30, 2024 •

edited

Loading

tiancaiamao commented Nov 1, 2024

*: maintain the active users for privilege data #56709

*: maintain the active users for privilege data #56709

Conversation

tiancaiamao commented Oct 17, 2024 • edited by bb7133 Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 17, 2024

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

tiancaiamao commented Oct 28, 2024

tiprow bot commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Oct 30, 2024

tiprow bot commented Oct 30, 2024

tiancaiamao commented Oct 30, 2024

tiprow bot commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 30, 2024

ti-chi-bot bot commented Oct 30, 2024

[LGTM Timeline notifier]

lance6716 commented Oct 30, 2024 • edited Loading

tiancaiamao commented Nov 1, 2024

tiancaiamao commented Oct 17, 2024 •

edited by bb7133

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

lance6716 commented Oct 30, 2024 •

edited

Loading