Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )
Change subject: KUDU-3373 Key provider interface ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@195 PS4, Line 195: key_provider_(new DefaultKeyProvider()) BTW, is there a case when 'key_provider_' would be a nullptr wrapper? In other words, why to have 'key_provider_' with semantics of a pointer instead of simply having DefaultKeyProvider instance? http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@691 PS4, Line 691: clear_server_key nit: maybe, plain_server_key would be a better name here (not sure I understand what 'clear' stands for here -- maybe I'm missing something) http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@693 PS4, Line 693: key_provider_->EncryptServerKey(clear_server_key, metadata->mutable_server_key()); Should this be wrapped into RETURN_NOT_OK() as well? http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/default_key_provider.h File src/kudu/server/default_key_provider.h: http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/default_key_provider.h@38 PS4, Line 38: memfrob(encrypted_server_key->data(), server_key.length()); Does this compile on macOS? http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h File src/kudu/server/key_provider.h: http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h@32 PS4, Line 32: // Decrypts the server key. : virtual Status DecryptServerKey(const std::string& encrypted_server_key, : std::string* server_key) = 0; : : // Encrypts the server key. : virtual Status EncryptServerKey(const std::string& server_key, : std::string* encrypted_server_key) = 0; naming nit: maybe, drop 'Server' from the names? It would be just 'DecryptKey()' and 'EncryptKey()' then. Or there is something substantial behind having 'Server' as the part of method names? http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h@394 PS4, Line 394: size_t key_size, const uint8_t* key nit: it's not exactly part of this changelist, but maybe it's not too late to change the order of the parameters to swap them? Usually, the data blob comes first and then comes the size in signatures like this. -- To view, visit http://gerrit.cloudera.org:8080/18568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac Gerrit-Change-Number: 18568 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 31 May 2022 22:19:54 +0000 Gerrit-HasComments: Yes
