[kudu-CR] KUDU-3413 server key update to tenant key

["Yuqi Du (Code Review)" <ge...@cloudera.org>]

Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19622 )

Change subject: KUDU-3413 server key update to tenant key
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@7
PS2, Line 7: server key update to tenant key
how about 'update server key to tenant key for multi-tenancy supported'

This patch is one of following patches. How about adding a tag for these patches. such as:

[multi-tenants] KUDU-3413 update server key to tenant key for multi-tenancy supported


http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@10
PS2, Line 10: we have to solve the
            : issue of version upgrade between the two features
Some description may add, such as:

Currently, there is no multi-tenants feature, in other words that is only one tenant exists and all tables belong to this tenant. So this patch will do some refactor to make this default tenant as a tenant of multi-tenants.


http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@14
PS2, Line 14: I want
            : to use this patch as the beginning of the multi-tenant
            : feature development.
What is your complete plan at this feature?
How many patches will be committed and what is the intention of every patch?

I have reviewed some of changes, and I think this patch is an advanced patch.
Before this patch, it seems tenants should be added first including manage tenants: CRUD a tenant.


http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:

http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.h@530
PS2, Line 530:   // Lock protecting the metadata below.
             :   mutable percpu_rwlock md_lock_;
nit: Protects 'metadata_'.

and how about metadata_rwlock_ ?


http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@915
PS2, Line 915:     InstanceMetadataPB::TenantMetadataPB* tenant_metadata = metadata->add_tenants();
             :     tenant_metadata->set_tenant_key_version(key_version);
             :     tenant_metadata->set_tenant_name(fs::kDefaultTenantName);
             :     tenant_metadata->set_tenant_key(key);
             :     tenant_metadata->set_tenant_key_iv(key_iv);
Whether simply convert the default tenant(the server tenant) as fs::kDefaultTenantName in memory, not add it into InstanceMetadataPB's 'repeated tenant_metadata'. If add it to InstanceMetadataPB::TenantMetadataPB it will be durable to 'instance' file and has  two tenants with the same name, one is the server tenant before and the other is durable by first startup after this feature.

Because fs::kDefaultTenantName is reserved for system kudu system to be compatible with the server before, we should not add another tenant named  fs::kDefaultTenantName. To reduce some complex problem, I think simply dealing with it is 
a considerable option.

What do you think this?


http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@968
PS2, Line 968: std::string
nit: string


http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@981
PS2, Line 981: std::string
nit: string


http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@986
PS2, Line 986: string
how about optional<string> ?


http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/key_provider.h
File src/kudu/fs/key_provider.h:

http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/key_provider.h@33
PS2, Line 33: DecryptTenantKey
IMO. 

Because your purpose is compatible with before, so it seems not necessary to change it for only the name changed. And the same advices for some other changes like this.



-- 
To view, visit http://gerrit.cloudera.org:8080/19622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e450d73940eb1dbaac6f905a46d6ccd084f15cf
Gerrit-Change-Number: 19622
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 01 Apr 2023 09:52:22 +0000
Gerrit-HasComments: Yes