resource_control: introduce privilege RESOURCE_GROUP_USER to restrict the switch of resource group #53483

glorv · 2024-05-22T08:36:07Z

What problem does this PR solve?

Issue Number: close #53440

Problem Summary:

What changed and how does it work?

This PR introduces a new dynamic privilege RESOURCE_GROUP_USER to restrict switch current session or query's resource group.

Only user with RESOURCE_GROUP_USER or RESOURCE_GROUP_ADMIN can execute SET RESOURCE GROUP name statement.
Only user with RESOURCE_GROUP_USER or RESOURCE_GROUP_ADMIN can use optimizer hint RESOURCE_GROUP(name), this hint will be ignored and return a warning if the user doesn't has this two privilege.

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.

Introduce privilege `RESOURCE_GROUP_USER` to restrict the switch of resource group

…ource group

tiprow · 2024-05-22T08:36:23Z

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

glorv · 2024-05-22T08:39:04Z

/ok-to-test

codecov · 2024-05-22T09:14:15Z

Codecov Report

Attention: Patch coverage is 94.82759% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.1128%. Comparing base (f124165) to head (c0f7a52).
Report is 9 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #53483         +/-   ##
=================================================
- Coverage   74.5222%   56.1128%   -18.4095%     
=================================================
  Files          1508       1628        +120     
  Lines        358470     605138     +246668     
=================================================
+ Hits         267140     339560      +72420     
- Misses        71953     242498     +170545     
- Partials      19377      23080       +3703

Flag	Coverage Δ
integration	`37.6310% <60.3448%> (?)`
unit	`71.7811% <89.6551%> (-1.6323%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (-2.2339%)`	⬇️
parser	`∅ <ø> (∅)`
br	`50.6773% <ø> (+6.7757%)`	⬆️

glorv · 2024-05-23T09:43:36Z

/retest

glorv · 2024-05-30T05:46:55Z

@tiancaiamao @nolouch PTAL

nolouch

RESOURCE_GROUP_ADMIN: CREATE/DROP/ALTER/SET/HINT resource group, CALIBRATE resources.

RESOURCE_GROUP_USER: SET/HINT resource group

am I right?

pkg/planner/core/planbuilder.go

nolouch · 2024-05-30T09:43:38Z

pkg/executor/simple.go

- hasSystemUserPriv := checker.RequestDynamicVerification(activeRoles, "SYSTEM_USER", false)
- hasRestrictedUserPriv := checker.RequestDynamicVerification(activeRoles, "RESTRICTED_USER_ADMIN", false)
+ hasSystemUserPriv := checker.RequestDynamicVerification(activeRoles, []string{"SYSTEM_USER"}, false)
+ hasRestrictedUserPriv := checker.RequestDynamicVerification(activeRoles, []string{"RESTRICTED_USER_ADMIN"}, false)


It seems that we can also use the old interface to check a list of privileges? like here

Yes, it doable, but not very convenient in some cases. While except for ResourceGroup there is only 1 other cases that need to pass 2 permissions. I'm also not sure if there are other cases in the future.

cc @tiancaiamao Do you think this modification is okay?

Maybe change the interface is unnecessary.
We do not have much cases relying on the new interface.

glorv · 2024-05-30T10:30:31Z

RESOURCE_GROUP_ADMIN: CREATE/DROP/ALTER/SET/HINT resource group, CALIBRATE resources.

RESOURCE_GROUP_USER: SET/HINT resource group

am I right?

Yes

glorv · 2024-06-06T10:11:05Z

@tiancaiamao PTAL, thanks~

tiancaiamao · 2024-06-13T08:02:04Z

pkg/planner/core/logical_plan_builder.go

 return append(vi, visitInfo{
 privilege: mysql.ExtendedPriv,
- dynamicPriv:  priv,
+ dynamicPrivs: privs,


Is this change necessary? Maybe you can 'git checkout master xxx.go' for most of the changes. @glorv

Yes, I think this change is necessary to express that user should have at least one of the privileges. I added a comment for the field dynamicPrivs

ti-chi-bot · 2024-06-13T09:22:39Z

[LGTM Timeline notifier]

Timeline:

2024-06-13 08:28:30.918776906 +0000 UTC m=+626064.972088831: ☑️ agreed by tiancaiamao.
2024-06-13 09:22:38.640574052 +0000 UTC m=+629312.693885977: ☑️ agreed by nolouch.

ti-chi-bot · 2024-06-14T06:34:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nolouch, qw4990, tiancaiamao

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~~ [qw4990,tiancaiamao]
~~pkg/planner/OWNERS~~ [qw4990]

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

add a new privilege RESOURCE_GROUP_USER to restrict the switch of res…

e09078a

…ource group

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels May 22, 2024

glorv requested review from tiancaiamao and nolouch May 22, 2024 08:36

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label May 22, 2024

fix test

8a1bfce

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2024

glorv added 7 commits May 22, 2024 19:08

remove unused import

6c1d843

replace reflect.DeepEqual

be2fe0c

fix test

f483518

fix

cefa8bc

fix integration test

8ecb767

fix test again

dafb560

remove unstable show warnings

855bca0

nolouch reviewed May 30, 2024

View reviewed changes

glorv added 2 commits June 13, 2024 15:27

do not change interface RequestDynamicVerification

81dd28f

Merge branch 'master' of ssh://github.com/pingcap/tidb into rg-user-priv

cb0baf2

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 13, 2024

break early

ba20d3b

fix test

8ae1440

tiancaiamao reviewed Jun 13, 2024

View reviewed changes

add comment

c0f7a52

tiancaiamao approved these changes Jun 13, 2024

View reviewed changes

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

nolouch approved these changes Jun 13, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 13, 2024

qw4990 approved these changes Jun 14, 2024

View reviewed changes

ti-chi-bot bot added the approved label Jun 14, 2024

ti-chi-bot bot merged commit 29fc940 into pingcap:master Jun 14, 2024
23 checks passed

glorv deleted the rg-user-priv branch June 14, 2024 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_control: introduce privilege RESOURCE_GROUP_USER to restrict the switch of resource group #53483

resource_control: introduce privilege RESOURCE_GROUP_USER to restrict the switch of resource group #53483

glorv commented May 22, 2024 •

edited

Loading

tiprow bot commented May 22, 2024

glorv commented May 22, 2024

codecov bot commented May 22, 2024 •

edited

Loading

glorv commented May 23, 2024

glorv commented May 30, 2024

nolouch left a comment

nolouch May 30, 2024

glorv May 30, 2024

nolouch Jun 4, 2024

tiancaiamao Jun 13, 2024

glorv commented May 30, 2024

glorv commented Jun 6, 2024

tiancaiamao Jun 13, 2024

glorv Jun 13, 2024

ti-chi-bot bot commented Jun 13, 2024

ti-chi-bot bot commented Jun 14, 2024

resource_control: introduce privilege RESOURCE_GROUP_USER to restrict the switch of resource group #53483

resource_control: introduce privilege RESOURCE_GROUP_USER to restrict the switch of resource group #53483

Conversation

glorv commented May 22, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented May 22, 2024

glorv commented May 22, 2024

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

glorv commented May 23, 2024

glorv commented May 30, 2024

nolouch left a comment

Choose a reason for hiding this comment

nolouch May 30, 2024

Choose a reason for hiding this comment

glorv May 30, 2024

Choose a reason for hiding this comment

nolouch Jun 4, 2024

Choose a reason for hiding this comment

tiancaiamao Jun 13, 2024

Choose a reason for hiding this comment

glorv commented May 30, 2024

glorv commented Jun 6, 2024

tiancaiamao Jun 13, 2024

Choose a reason for hiding this comment

glorv Jun 13, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 13, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Jun 14, 2024

glorv commented May 22, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading