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

feat: object warehouse support rbac #17262

Merged
merged 7 commits into from
Feb 24, 2025
Merged

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Jan 13, 2025

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Warehouse support rbac.(Only support system cluster now)

Adds the following GRANT statements:

GRANT CREATE WAREHOUSE ON *.* TO ROLE <role_name>; -- This statement grants a role the permission to create warehouses .
GRANT OWNERSHIP ON WAREHOUSE <warehouse_name> TO ROLE <role_name>;  -- This statement allows a role to be granted ownership of a specific warehouse.

Warehouse object permission checks are only enforced when the query.resources_management configuration is set to type = "system_managed" with a node_group = "node_group". This conditional enforcement ensures that warehouse object permission checks are only actually applied when query.resources_management is configured.

e.g.

[query.resources_management]
type = "system_managed"
node_group = "node_group" 

Note:

If support_forward_warehouse_request=false, will not check warehouse privilege.

Now only SystemResourcesManagement will return true.

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@TCeason TCeason requested a review from drmingdrmer as a code owner January 13, 2025 08:11
@TCeason TCeason marked this pull request as draft January 13, 2025 08:11
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Jan 13, 2025
@TCeason TCeason force-pushed the warehouse_rbac branch 2 times, most recently from 61ebeb4 to f6f2812 Compare January 13, 2025 08:14
@TCeason
Copy link
Collaborator Author

TCeason commented Jan 13, 2025

Need to add compatibility tests.

@BohuTANG
Copy link
Member

Does this affect the cloud warehouse's operations?

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TCeason)

@TCeason
Copy link
Collaborator Author

TCeason commented Jan 13, 2025

Does this affect the cloud warehouse's operations?

No. Now it only control warehouse operator plan. Cloud warehouse need to support these plan.

@TCeason TCeason force-pushed the warehouse_rbac branch 3 times, most recently from 62afcd1 to 7640dfa Compare January 13, 2025 13:31
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TCeason)

@TCeason TCeason force-pushed the warehouse_rbac branch 4 times, most recently from af24458 to feee99b Compare January 15, 2025 03:31
@TCeason TCeason added the ci-cloud Build docker image for cloud test label Jan 15, 2025
Copy link
Contributor

Docker Image for PR

  • tag: pr-17262-d6d5d0a-1736917932

note: this image tag is only available for internal use,
please check the internal doc for more details.

@TCeason TCeason added ci-cloud Build docker image for cloud test and removed ci-cloud Build docker image for cloud test labels Feb 14, 2025
Copy link
Contributor

Docker Image for PR

  • tag: pr-17262-37f5dd5-1739503780

note: this image tag is only available for internal use,
please check the internal doc for more details.

@TCeason
Copy link
Collaborator Author

TCeason commented Feb 14, 2025

Test on local:

Test Steps:

1. Root User Operations:

-- Create a role named 'role_a'
create role role_a;

-- Grant the privilege to create warehouses to 'role_a' on all databases and schemas
grant create warehouse on *.* to role role_a;

-- Grant the privilege to create databases to 'role_a' on all databases and schemas
grant create database on *.* to role role_a;

-- Create user 'a' with password '123' and set the default role to 'role_a'
create user a identified by '123' with default_role='role_a';

-- Grant the role 'role_a' to user 'a'
grant role role_a to a;

-- Create user 'b' with password '123'
create user b identified by '123';

2. User 'a' Operations:

-- Create a database named 'a'
create database a;

-- Create a table 't' with an integer column 'id' in database 'a'
create table a.t(id int);

-- Insert a row with value 1 into table 'a.t'
insert into a.t values(1);

-- Select all data from table 'a.t'
select * from a.t;

-- Create a warehouse named 'w1' with warehouse size 1
create warehouse w1 with warehouse_size = 1;

-- Use the warehouse 'w1'
use warehouse w1;

-- Show grants on warehouse 'w1'
show grants on warehouse w1;

-- Rename warehouse 'w1' to 'w1_rename'
RENAME WAREHOUSE w1 TO w1_rename;

3. User 'a' Logout/Login Operations:

-- After Logout and Login as user 'a'

-- Show all available warehouses
show warehouses;

-- Show grants on the renamed warehouse 'w1_rename'
show grants on warehouse w1_rename;

-- Show grants for the current user
show grants;

-- Show grants for the role 'role_a'
show grants for role role_a;

4. User 'b' Operations:

-- As user 'b'

-- Show all available warehouses (expect empty result)
show warehouses; -- Expected: empty

-- Attempt to use warehouse 'w1' (expect failure with error code 2406)
use warehouse w1; -- Expected: failure (Error 2406)

-- Attempt to use the renamed warehouse 'w1_rename' (expect failure with error code 1063)
use warehouse w1_rename; -- Expected: failure (Error 1063)

Main Query Information:

  • The main version doesn't have create warehouse privilege, so show grants will display empty privileges.
-- As user 'a'
use warehouse w1_rename;
show grants;

Expected Output for show grants for role role_a:

┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ privileges │ object_name │     object_id    │ grant_to │  name  │             grants             │
│   String   │    String   │ Nullable(UInt64) │  String  │ String │        Nullable(String)        │
├────────────┼─────────────┼──────────────────┼──────────┼────────┼────────────────────────────────┤
│            │ *.*         │             NULL │ ROLE     │ role_a │ GRANT  ON *.* TO ROLE `role_a` │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘
1 row read in 0.021 sec. Processed 1 row, 99B (47.62 rows/s, 4.60 KiB/s)

5. Successful Operations for User 'a':

-- Ensure the following operations for user 'a' are successful

-- Use database 'a'
use a;

-- Select all data from table 'a.t'
select * from a.t;

-- Insert a new row with value 2 into table 'a.t'
insert into a.t values(2);

-- Create a database named 'b'
create database b;

-- Create a table 't' with an integer column 'id' in database 'b'
create table b.t(id int);

-- Insert a row with value 1 into table 'b.t'
insert into b.t values(1);

-- Select all data from table 'b.t'
select * from b.t;

@TCeason
Copy link
Collaborator Author

TCeason commented Feb 14, 2025

Cloud warehouse does not use same logic with databend-query? cc @flaneur2020 @everpcpc

@TCeason TCeason marked this pull request as ready for review February 14, 2025 05:19
@TCeason
Copy link
Collaborator Author

TCeason commented Feb 17, 2025

cc @zhang2014 @flaneur2020 Please review this pr.

note: only support_forward_warehouse_request will apply warehouse rbac. Now only support SystemResourcesManagement
2. grant ownership on warehouse object only supported for warehouses managed by the system
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

LGTM. some nits.

Reviewed 5 of 10 files at r3, 2 of 12 files at r4, 6 of 11 files at r5, 19 of 19 files at r6, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @everpcpc, @flaneur2020, and @zhang2014)


src/meta/proto-conv/src/ownership_from_to_protobuf_impl.rs line 129 at r6 (raw file):

                }),
            ),
            mt::principal::OwnershipObject::Warehouse { id: uid } => {

same here


src/meta/proto-conv/tests/it/v120_warehouse_ownershipobject.rs line 94 at r6 (raw file):

    common::test_pb_from_to(func_name!(), want())?;
    common::test_load_old(func_name!(), role_info_v120.as_slice(), 117, want())?;

117 should be 120?


src/meta/proto-conv/tests/it/v120_warehouse_ownershipobject.rs line 113 at r6 (raw file):

    };
    common::test_pb_from_to(func_name!(), want())?;
    common::test_load_old(func_name!(), ownership_info_v120.as_slice(), 117, want())?;

117 should be 120?


src/meta/app/src/principal/ownership_object.rs line 87 at r6 (raw file):

            OwnershipObject::UDF { name } => write!(f, "UDF {name}"),
            OwnershipObject::Stage { name } => write!(f, "STAGE {name}"),
            OwnershipObject::Warehouse { id: uid } => write!(f, "Warehouse {uid}"),

Suggestion:

            OwnershipObject::Warehouse { id } => write!(f, "Warehouse {id}"),

src/meta/app/src/principal/ownership_object.rs line 127 at r6 (raw file):

            OwnershipObject::Stage { name } => b.push_raw("stage-by-name").push_str(name),
            OwnershipObject::UDF { name } => b.push_raw("udf-by-name").push_str(name),
            OwnershipObject::Warehouse { id: uid } => b.push_raw("warehouse-by-uid").push_str(uid),

Suggestion:

            OwnershipObject::Warehouse { id } => b.push_raw("warehouse-by-uid").push_str(id),

src/meta/app/src/principal/ownership_object.rs line 176 at r6 (raw file):

            "warehouse-by-uid" => {
                let uid = p.next_str()?;
                Ok(OwnershipObject::Warehouse { id: uid })

same here

@TCeason TCeason requested a review from drmingdrmer February 21, 2025 01:09
Copy link
Member

@flaneur2020 flaneur2020 left a comment

Choose a reason for hiding this comment

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

LGTM

@TCeason
Copy link
Collaborator Author

TCeason commented Feb 21, 2025

cc @BohuTANG I think it's time to merge this pr.

@BohuTANG
Copy link
Member

@TCeason Please make the PR summary better and clearer. The usage examples in the comments are hard to follow.

@TCeason
Copy link
Collaborator Author

TCeason commented Feb 21, 2025

@TCeason Please make the PR summary better and clearer. The usage examples in the comments are hard to follow.

Done

@TCeason
Copy link
Collaborator Author

TCeason commented Feb 21, 2025

@TCeason Please make the PR summary better and clearer. The usage examples in the comments are hard to follow.

This comment describes the local compatibility testing process

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @everpcpc, @flaneur2020, @TCeason, and @Xuanwo)

@BohuTANG BohuTANG merged commit b0835cc into databendlabs:main Feb 24, 2025
79 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cloud Build docker image for cloud test pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support RBAC for warehouse
6 participants