*: avoid notify privilege update for all users #57042

tiancaiamao · 2024-10-31T10:37:57Z

What problem does this PR solve?

Issue Number: ref #55563

Problem Summary:

In the previous commit, I have maintained the active user lists, this commit intend to fix the notify part.
When privilege change, just notify the changed users and update data for them, instead of all the users.

What changed and how does it work?

The code changes including:

notify privilege update will encoding the changed user list into the etcd message
the domain privilege loop will decode the user list and update only the active users among them
fix bug on the privilege handle merge operation, the old "append diff + sort + dedup" can not handle the reovke privilege operation
the roles of a user is also consider as the privilege data and ensureActiveUse() need to load them
ensureActiveUser() should load the privilege data of the user, the roles of the data, plus the data of roles recursively
ensureActiveUser() is called more widely now to make the CI pass (we'd better optimize here later)

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-31T10:38:18Z

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-31T10:50:05Z

Codecov Report

Attention: Patch coverage is 82.82443% with 45 lines in your changes missing coverage. Please review.

Project coverage is 74.9233%. Comparing base (56e7093) to head (db9a92d).
Report is 56 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57042        +/-   ##
================================================
+ Coverage   72.9672%   74.9233%   +1.9561%     
================================================
  Files          1662       1717        +55     
  Lines        458718     479730     +21012     
================================================
+ Hits         334714     359430     +24716     
+ Misses       103425      98000      -5425     
- Partials      20579      22300      +1721

Flag	Coverage Δ
integration	`49.1373% <74.8091%> (?)`
unit	`72.2557% <81.2977%> (-0.0546%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`60.3631% <0.0000%> (+15.3172%)`	⬆️

ti-chi-bot · 2024-11-05T12:00:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bornchanger, d3hunter for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

tiancaiamao · 2024-11-05T13:11:10Z

/retest

tiprow · 2024-11-05T13:11:34Z

@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.

tiancaiamao · 2024-11-07T08:05:29Z

/test check-dev2

tiprow · 2024-11-07T08:05:51Z

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

In response to this:

/test check-dev2

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.

tiancaiamao · 2024-11-08T10:52:44Z

/test pull-br-integration-test

tiprow · 2024-11-08T10:53:06Z

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

In response to this:

/test pull-br-integration-test

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/domain/domain.go

pkg/executor/simple.go

tiancaiamao · 2024-11-11T08:13:45Z

/retest

tiprow · 2024-11-11T08:14:14Z

@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/domain/domain.go

tiancaiamao · 2024-11-18T07:00:39Z

/test unit-test

tiprow · 2024-11-18T07:01:01Z

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

In response to this:

/test unit-test

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.

lcwangchao · 2024-11-19T03:26:22Z

br/pkg/restore/snap_client/systable_restore.go

@@ -200,7 +200,7 @@ func (rc *SnapClient) afterSystemTablesReplaced(ctx context.Context, db string,
 	var err error
 	for _, table := range tables {
 		if table == "user" {
-			if serr := rc.dom.NotifyUpdatePrivilege(); serr != nil {
+			if serr := rc.dom.NotifyUpdatePrivilege(nil); serr != nil {


What does NotifyUpdatePrivilege actually do when input an empty user list?

lcwangchao · 2024-11-19T05:35:47Z

pkg/domain/domain.go

 			ok := true
 			select {
 			case <-do.exit:
 				return
-			case _, ok = <-watchCh:
+			case resp, ok = <-watchCh:
+				if ok {


Though it is not introduced in this PR, do we need to handle the ok == false here? If the watchCh closed unexpectedly for some reason, this loop would run frequent.

Just like this issue #49273

D3Hunter · 2024-11-19T05:45:21Z

pkg/privilege/privileges/cache.go

-func (h *Handle) Update() error {
-	var priv MySQLPrivilege
-	err := priv.LoadAll(h.sctx)
+func (h *Handle) Update(userList []string) error {


please split into UpdateAll and Update, a nil userList []string is not a clear parameter to mean we want to update all, it's more common to mean we want to update nothing

D3Hunter · 2024-11-19T05:54:04Z

pkg/domain/domain.go

+func encodeUserList(userList []string) string {
+	// use base64 encoding for the user and use ',' to separate them.
+	// Because the user name itself may contains special char like ','
+	encoded := make([]string, 0, len(userList))
+	for _, user := range userList {
+		encoded = append(encoded, base64.StdEncoding.EncodeToString([]byte(user)))
+	}
+	return strings.Join(encoded, ",")
+}
+
+func decodeUserList(userList []string, val string) []string {
+	users := strings.Split(val, ",")
+	for _, userRaw := range users {
+		user, err := base64.StdEncoding.DecodeString(userRaw)
+		if err == nil {
+			userList = append(userList, string(user))
+		}
+	}
+	return userList
+}


please use a json as the value of the etcd key, easy to en/decode and extend

also what's the size of the list, etcd is not fit to store large data, max 1.5m by default

*: avoid notify privilege update for all users

3569fb0

ti-chi-bot bot added 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 31, 2024

tiancaiamao added 3 commits November 1, 2024 09:33

fix build

4f3fcad

fix CI

dcd2095

append+sort+dedup does not work for delete record

529716a

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 Nov 4, 2024

tiancaiamao added 3 commits November 4, 2024 22:07

make lint happy

b545ab6

fix integration test

cf1c7b5

match resource group

ba6c578

tiancaiamao added 9 commits November 6, 2024 16:13

Merge branch 'master' into notify-update

de42e04

loadSomeUser now load the user's role first

e473466

make lint check happy

fbdc2e8

fix test case TestFailedLoginTrackingBasic

1b7ac08

make lint happy

363f6af

fix TestDefaultRoles

d54cd58

fix TestSandBoxMode

3c89902

fix build

20cbbcc

Merge branch 'master' into notify-update

8cc9d34

tiancaiamao added 2 commits November 7, 2024 20:43

fix TestAuthPlugin

a91270b

the role of an active user should also be consider active

e061798

tiancaiamao requested review from lance6716, Defined2014, Rustin170506 and D3Hunter and removed request for lance6716 and Defined2014 November 8, 2024 11:44

lance6716 reviewed Nov 11, 2024

View reviewed changes

pkg/domain/domain.go Outdated Show resolved Hide resolved

pkg/domain/domain.go Outdated Show resolved Hide resolved

pkg/executor/simple.go Show resolved Hide resolved

address comment

fc7589e

Merge branch 'master' into notify-update

9ff7c7a

tiancaiamao requested a review from lance6716 November 12, 2024 07:25

This was referenced Nov 12, 2024

*: move the 'user' field to the first one of primary key for mysql tables | tidb-test=pr/2425 #57320

Open

*: add context.Context to ensureActiveUser() #57351

Open

lance6716 reviewed Nov 14, 2024

View reviewed changes

pkg/domain/domain.go Outdated Show resolved Hide resolved

pkg/domain/domain.go Outdated Show resolved Hide resolved

pkg/domain/domain.go Show resolved Hide resolved

pkg/domain/domain.go Outdated Show resolved Hide resolved

tiancaiamao mentioned this pull request Nov 14, 2024

*: avoid some unnecessary call of ensureActiveUser() #57388

Open

13 tasks

tiancaiamao added 3 commits November 14, 2024 21:36

address comment

fe21eee

make fmt

66542b8

address comment

db9a92d

tiancaiamao requested a review from lance6716 November 18, 2024 06:06

lcwangchao reviewed Nov 19, 2024

View reviewed changes

D3Hunter reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: avoid notify privilege update for all users #57042

*: avoid notify privilege update for all users #57042

tiancaiamao commented Oct 31, 2024 •

edited

Loading

tiprow bot commented Oct 31, 2024

codecov bot commented Oct 31, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 5, 2024

tiancaiamao commented Nov 5, 2024

tiprow bot commented Nov 5, 2024

tiancaiamao commented Nov 7, 2024

tiprow bot commented Nov 7, 2024

tiancaiamao commented Nov 8, 2024

tiprow bot commented Nov 8, 2024

tiancaiamao commented Nov 11, 2024

tiprow bot commented Nov 11, 2024

tiancaiamao commented Nov 18, 2024

tiprow bot commented Nov 18, 2024

lcwangchao Nov 19, 2024

lcwangchao Nov 19, 2024

lcwangchao Nov 19, 2024

D3Hunter Nov 19, 2024 •

edited

Loading

D3Hunter Nov 19, 2024

*: avoid notify privilege update for all users #57042

Are you sure you want to change the base?

*: avoid notify privilege update for all users #57042

Conversation

tiancaiamao commented Oct 31, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 31, 2024

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Nov 5, 2024

tiancaiamao commented Nov 5, 2024

tiprow bot commented Nov 5, 2024

tiancaiamao commented Nov 7, 2024

tiprow bot commented Nov 7, 2024

tiancaiamao commented Nov 8, 2024

tiprow bot commented Nov 8, 2024

tiancaiamao commented Nov 11, 2024

tiprow bot commented Nov 11, 2024

tiancaiamao commented Nov 18, 2024

tiprow bot commented Nov 18, 2024

lcwangchao Nov 19, 2024

Choose a reason for hiding this comment

lcwangchao Nov 19, 2024

Choose a reason for hiding this comment

lcwangchao Nov 19, 2024

Choose a reason for hiding this comment

D3Hunter Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

D3Hunter Nov 19, 2024

Choose a reason for hiding this comment

tiancaiamao commented Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

D3Hunter Nov 19, 2024 •

edited

Loading