*: 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 75.27675% with 67 lines in your changes missing coverage. Please review.

Project coverage is 74.8968%. Comparing base (ad5ca42) to head (99cd464).
Report is 27 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57042        +/-   ##
================================================
+ Coverage   72.8329%   74.8968%   +2.0639%     
================================================
  Files          1676       1721        +45     
  Lines        463539     482927     +19388     
================================================
+ Hits         337609     361697     +24088     
+ Misses       105125      98841      -6284     
- Partials      20805      22389      +1584

Flag	Coverage Δ
integration	`49.2693% <72.3247%> (?)`
unit	`72.1978% <73.4317%> (-0.0215%)`	⬇️

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

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

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

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/privilege/privileges/cache.go

tiancaiamao · 2024-11-20T09:31:14Z

/retest

tiprow · 2024-11-20T09:31:35Z

@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-21T07:50:18Z

/retest

tiprow · 2024-11-21T07:50:40Z

@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-21T08:08:56Z

/test unit-test

tiprow · 2024-11-21T08:09:17Z

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

D3Hunter

rest lgtm

D3Hunter · 2024-11-21T07:19:50Z

pkg/domain/domain.go

+			if len(val) > 0 {
+				err := json.Unmarshal(val, &msg)
+				if err == nil {
+					break
+				}
+				logutil.BgLogger().Warn("decodePrivilegeEvent unmarshal fail", zap.Error(err))
+			}
+		}
+	}
+	// In case something is wrong, for example, old version tidb mixed with newer, the unmarshal would fail.
+	// Then we fallback to the old way: reload all the users.
+	if len(msg.UserList) == 0 {
+		msg.All = true
+	}


Suggested change

if len(val) > 0 {

err := json.Unmarshal(val, &msg)

if err == nil {

break

}

logutil.BgLogger().Warn("decodePrivilegeEvent unmarshal fail", zap.Error(err))

}

}

}

// In case something is wrong, for example, old version tidb mixed with newer, the unmarshal would fail.

// Then we fallback to the old way: reload all the users.

if len(msg.UserList) == 0 {

msg.All = true

}

if len(val) > 0 {

err := json.Unmarshal(val, &msg)

if err == nil {

break

}

logutil.BgLogger().Warn("decodePrivilegeEvent unmarshal fail", zap.Error(err))

} else {

// In case old version triggers the event, the event value is empty,

// Then we fall back to the old way: reload all the users.

if len(msg.UserList) == 0 {

msg.All = true

}

}

}

}

This differs from the old logic. @D3Hunter
What happen if something is wrong? if val does have data, but the data is not what we expected?
Neither the UserList contains data nor the ALL flag is set.
Our expected case is reload all on error to tolerance the cases, but the suggested change here is ignore on error.

What happen if something is wrong? if val does have data, but the data is not what we expected?

if no third-part involved, older version tidb write a empty value, new version write a valid json, so when will the data be invalid? If it's caused by bug, then let's fix it, else I think the event consumer should do as what the producer told it to do. and maybe you can add a intest.Assert to check that either All=true or the len(userList) > 0, the producer side should make sure no such invalid data are sent to ETCD

and as you have mentioned, the event is not that critical, i think it's acceptable

What happen if something is wrong? if val does have data, but the data is not what we expected?

if no third-part involved, older version tidb write a empty value, new version write a valid json, so when will the data be invalid? If it's caused by bug, then let's fix it, else I think the event consumer should do as what the producer told it to do. and maybe you can add a intest.Assert to check that either All=true or the len(userList) > 0, the producer side should make sure no such invalid data are sent to ETCD

and as you have mentioned, the event is not that critical, i think it's acceptable

I agree that we should ignore the message and skip to update privilege cache when failed to decode the json. We only need the handle to case when val == "" to be compatible with old versions (please add some clear comments to explain why we have this logic). And because we have a loop to guarantee the cache should be updated in a certain time, it is not a big deal.

We can also generate some warnings logs when meet a invalid json.

pkg/domain/domain.go

pkg/privilege/privileges/cache.go

Co-authored-by: D3Hunter <jujj603@gmail.com>

tiancaiamao · 2024-11-21T11:41:03Z

/test unit-test

tiprow · 2024-11-21T11:41:25Z

@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

LGTM except the disussion: https://github.com/pingcap/tidb/pull/57042/files#r1853232658

ti-chi-bot · 2024-11-22T03:22:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lcwangchao
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:

~~OWNERS~~ [lcwangchao]
br/OWNERS
pkg/ddl/OWNERS
pkg/domain/OWNERS

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-11-22T03:22:10Z

[LGTM Timeline notifier]

Timeline:

2024-11-22 03:22:10.09138556 +0000 UTC m=+174717.711040075: ☑️ agreed by lcwangchao.

*: 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

lcwangchao reviewed Nov 20, 2024

View reviewed changes

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

tiancaiamao added 6 commits November 20, 2024 14:17

address comment

f0ee13a

make bazel_prepare

54c2e1a

make lint happy

2ea8cdd

make lint happy

60bc7f6

make lint happy

13b5a91

Merge branch 'master' into notify-update

badb6ab

D3Hunter reviewed Nov 20, 2024

View reviewed changes

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

fix CI

5b0db78

lcwangchao reviewed Nov 21, 2024

View reviewed changes

pkg/domain/domain.go Show resolved Hide resolved

pkg/domain/domain.go Show resolved Hide resolved

tiancaiamao added 2 commits November 21, 2024 15:09

address comment

57a9275

address comment

6558e49

tiancaiamao requested review from D3Hunter and lcwangchao November 21, 2024 07:13

D3Hunter reviewed Nov 21, 2024

View reviewed changes

tiancaiamao and others added 2 commits November 21, 2024 17:43

Update pkg/privilege/privileges/cache.go

a187c8a

Co-authored-by: D3Hunter <jujj603@gmail.com>

address comment

99cd464

tiancaiamao requested a review from D3Hunter November 22, 2024 02:50

lcwangchao approved these changes Nov 22, 2024

View reviewed changes

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

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

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 20, 2024

tiprow bot commented Nov 20, 2024

tiancaiamao commented Nov 21, 2024

tiprow bot commented Nov 21, 2024

tiancaiamao commented Nov 21, 2024

tiprow bot commented Nov 21, 2024

D3Hunter left a comment

D3Hunter Nov 21, 2024

tiancaiamao Nov 21, 2024

D3Hunter Nov 22, 2024

lcwangchao Nov 22, 2024 •

edited

Loading

tiancaiamao commented Nov 21, 2024

tiprow bot commented Nov 21, 2024

lcwangchao left a comment

ti-chi-bot bot commented Nov 22, 2024

ti-chi-bot bot commented Nov 22, 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

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 20, 2024

tiprow bot commented Nov 20, 2024

tiancaiamao commented Nov 21, 2024

tiprow bot commented Nov 21, 2024

tiancaiamao commented Nov 21, 2024

tiprow bot commented Nov 21, 2024

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Nov 21, 2024

Choose a reason for hiding this comment

tiancaiamao Nov 21, 2024

Choose a reason for hiding this comment

D3Hunter Nov 22, 2024

Choose a reason for hiding this comment

lcwangchao Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

tiancaiamao commented Nov 21, 2024

tiprow bot commented Nov 21, 2024

lcwangchao left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 22, 2024

ti-chi-bot bot commented Nov 22, 2024

[LGTM Timeline notifier]

tiancaiamao commented Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

lcwangchao Nov 22, 2024 •

edited

Loading