Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/19622 )
Change subject: KUDU-3413 [multi-tenancy] update server key for multi-tenancy
......................................................................
Patch Set 11:
(10 comments)
Thank you for working on the multi-tenant feature.
I apologize for the very late review.
I think it's would be great to
* move simple renamings into a separate changelist
* move the upgrade procedure into a new kudu CLI tool since that way it would
be
** more controllable w.r.t. upgrading the data and handle possible failures
** more practical from keeping the run-time of the tablet servers free of
the code of 'single-use'
Also, please document (at least in the changelist's description) what's the
behaviour of a tablet server of prior version when it run on the data
directories 'converted' into multi-tenant ones.
http://gerrit.cloudera.org:8080/#/c/19622/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19622/11//COMMIT_MSG@16
PS11, Line 16: If the flag is enabled, a default tenant will be
: generated using the existing server key info during bootstrap,
the key
: information of both will be exactly the same. At the same time,
: the information of the server key will be preserved.
What happens when a tablet server of prior versions or a tablet server of newer
version but with --enable_multi_tenancy=false is run against such 'upgraded'
data? Will it silently corrupt the data?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs.proto
File src/kudu/fs/fs.proto:
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs.proto@47
PS11, Line 47: we have to be compatible with the upgrade of the old
: // version
Please rephrase this. The compatibility isn't something that affects 'us' and
'them', but rather filesystem metadata files between different versions, etc.
Essentially, it would be great to layout the exact terms of what
'compatibility' means here.
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
PS11:
Why to have all this code in the runtime of a tablet server when it's is
supposed to be used only once and in rare case if somebody starts using the
multi-tenancy feature?
Wouldn't it be more practical to have the upgrade procedure as a new kudu CLI
tool?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs_manager.cc@513
PS11, Line 513: CHECK(FLAGS_enable_multi_tenancy);
Why to crash here if FLAGS_enable_multi_tenancy is set 'false' even in RELEASE
build? Why not just return Status::IllegalState(), exiting gracefully from
such a situation?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/key_provider.h
File src/kudu/fs/key_provider.h:
PS11:
Could you please separate this and other simple renaming changes into its own
changelist (before introducing those multi-tenancy extras)?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/external_mini_cluster.cc@1374
PS11, Line 1374: tenant_name
Can a valid 'tenant_name' be empty?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/external_mini_cluster.cc@1380
PS11, Line 1380: if (!instance.server_key().empty()) {
:
RETURN_NOT_OK(key_provider_->DecryptEncryptedKey(instance.server_key(),
:
instance.server_key_iv(),
:
instance.server_key_version(),
:
&decrypted_key));
: } else {
: const InstanceMetadataPB::TenantMetadataPB* tenant =
nullptr;
: for (const auto& tdata : instance.tenants()) {
: if (tdata.tenant_name() == tenant_name) {
: tenant = &tdata;
: break;
: }
: }
: if (tenant && !tenant->tenant_key().empty()) {
:
RETURN_NOT_OK(key_provider_->DecryptEncryptedKey(tenant->tenant_key(),
:
tenant->tenant_key_iv(),
:
tenant->tenant_key_version(),
:
&decrypted_key));
: }
: }
I'm not sure this reflects the priority of tenant keys described in the commit
description:
> The priority of tenant key is higher than that of server key, which
> means that if both server key info and tenants info exist in the
> metadata, we will use the tenants.
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/internal_mini_cluster.cc
File src/kudu/mini-cluster/internal_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/internal_mini_cluster.cc@228
PS11, Line 228: string tenant_name;
: string tenant_id;
: KuduTest::GetEncryptionKey(&tenant_name,
: &tenant_id,
These two arguments aren't used in this code. Maybe, it makes sense to allow
for nullptr for first two parameters for the KuduTest::GetEncryptionKey()
method?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/util/env_posix.cc@216
PS11, Line 216: TAG_FLAG(enable_multi_tenancy
Shouldn't this flag be marked as 'experimental' at this point?
http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/util/env_posix.cc@221
PS11, Line 221: "We should make --enable_multi_tenancy set with
--encrypt_data_at_rest"
: "at the same time
It's not what the condition for the if() actually checks for (otherwise, it
should have had (!FLAGS_enable_multi_tenancy && FLAGS_encrypt_data_at_rest) as
well).
How about:
--enable_multi_tenancy can be set 'true' only when --encrypt_data_at_rest is
set 'true'.
--
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: 11
Gerrit-Owner: KeDeng <kdeng...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <ale...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abu...@apache.org>
Gerrit-Reviewer: KeDeng <kdeng...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com>
Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org>
Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com>
Gerrit-Comment-Date: Wed, 17 May 2023 05:12:11 +0000
Gerrit-HasComments: Yes
