Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17974 )
Change subject: [encryption] KUDU-3331 Encrypt file system ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/17974/2//COMMIT_MSG Commit Message: PS2: > It'd be nice to see some benchmarks for how this performs. Perhaps we could Thanks for the tips, I'll add some benchmarks (currently working on it). I wanted to push my changes so you can do another pass before I add the benchmark results to the commit message (and some relevant code changes). http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/consensus/consensus_meta-test.cc File src/kudu/consensus/consensus_meta-test.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/consensus/consensus_meta-test.cc@82 PS2, Line 82: EnableEncryption(bool enable) { > nit: why not just 'EnableEncryption(bool enable)' ? Done http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager-test.cc@229 PS2, Line 229: EnableEncryption(bool enable) { > nit: why not just 'EnableEncryption(bool enable)' ? Done http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager.cc@3005 PS2, Line 3005: string tmp_file_name; : RETURN_NOT_OK_LBM_DISK_FAILURE_PREPEND(env_->NewTempRWFile(RWFileOptions(), tmpl, : &tmp_file_name, &tmp_file), : "could not create temporary metadata file"); > Looked at this a bit, but could not understand why this change was necessar I guess this wasn't necessary at the end. I tried different approaches, it seems I left this here by mistake. http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/integration-tests/raft_consensus-itest.cc@828 PS2, Line 828: // process, as both of t > nit (here and below): could you add a comment explaining why adding flags t Done http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/tools/tool_action_pbc.cc File src/kudu/tools/tool_action_pbc.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/tools/tool_action_pbc.cc@105 PS2, Line 105: : bool IsFileEncrypted(Env* env, const std::string& fname) { : // TODO(abukor): replace with real encryption check. As LBM metadata files are : // the only PBC files that are encrypted right now, this check will suffice : > nit: maybe add a comment mentioning why this is the case? I don't quite fol thanks, added an explanation. http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env.h@383 PS2, Line 383: bool IsEncryptionE > Do you see it ever be requiring some non-const stuff in the implementation? Done http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env.h@421 PS2, Line 421: > +1, I found this this surprising. In general we've been security-disabled b I changed the behavior somewhat, mostly to require fewer changes in the code. This now means that the file is/should be encrypted, but it only takes effect when encryption is enabled. This way, when you create a file, you don't need to know if encryption is enabled. If encryption is disabled (which is the case by default), it doesn't matter what this flag is set to. When encryption is enabled, all files, except those where this is explicitly set to false, are encrypted. I added a note to the flag. http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env_posix.cc@203 PS2, Line 203: encrypt_da > nit: maybe, name this flag 'enable_encryption' ? How about encrypt_data_at_rest to make sure it's not going to be confused with TLS? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env_posix.cc@1997 PS2, Line 1997: const b > nit: drop 'virtual' since there is 'override' already Done -- To view, visit http://gerrit.cloudera.org:8080/17974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I909d0c4af0c1fca0d14c99a6627842dbe2ed7524 Gerrit-Change-Number: 17974 Gerrit-PatchSet: 3 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: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 27 Oct 2021 19:53:42 +0000 Gerrit-HasComments: Yes
