Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18192 )
Change subject: KUDU-3368 Encrypt file keys with server keys ...................................................................... Patch Set 5: Code-Review+1 (6 comments) overall looks good to me, just a few nits http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/fs/fs_manager.h@207 PS5, Line 207: server_key nit: it would be nice to document the behavior of this method to reflect the newly introduced 'server_key' parameter http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/fs/fs_manager.h@292 PS5, Line 292: const std::string& server_key() const; Would be nice to document the essence of this new method. What does it return if the FS isn't encrypted? http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/mini-cluster/internal_mini_cluster.h File src/kudu/mini-cluster/internal_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/mini-cluster/internal_mini_cluster.h@167 PS5, Line 167: Env* ts_env(int ts_idx) const override { nit: maybe, add a doc blurb to explain the essence of why the same default environment is used for any tablet server and master in a mini-cluster http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/tools/tool_action_fs.cc@88 PS5, Line 88: nit: remove the extra space BTW, is there any convention in which form the key is presented? PEM, just uuencoded/base64-encoded DER, etc.? http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env.h@80 PS5, Line 80: Unlike the default env, this is not owned by Kudu, and : // must be destroyed when not used anymore With this, maybe change the signature of this method to return std::unique_ptr<Env> instead? That way the expected lifecycle of the returned entity would be very straightforward. http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env_util.cc@243 PS5, Line 243: // Both the source and the destination files are treated as insensitive, : // because if they're encrypted, it would be unnecessary to decrypt and : // re-encrypt it. This way, we make a byte for byte copy of the file : // regardless if it's encrypted. Is this comment still valid after change at line 248? -- To view, visit http://gerrit.cloudera.org:8080/18192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1884f62fde3bb110291b1d01c5e68942c6071318 Gerrit-Change-Number: 18192 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 23 May 2022 01:21:25 +0000 Gerrit-HasComments: Yes
