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