Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys ...................................................................... Patch Set 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@662 PS11, Line 662: Should only be called : // when a block is fully written, nit: mind updating this? http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@749 PS11, Line 749: uint8_t nit: auto or size_t? http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@750 PS11, Line 750: if (encryption_header_size > 0) { Please add a comment describing why this is here. Would be also good to mention in the commit message that there are some special considerations for LBM container files, since this patch is quite opinionated about how we handle them: each container "loses" its first FS-block-size-worth of data, right? http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@937 PS11, Line 937: a nit: const? http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1095 PS11, Line 1095: record->length() > 0 I'm curious how this changed? What in this patch results in 0-length records? http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1318 PS11, Line 1318: if (block_start_offset < FLAGS_log_container_preallocate_bytes) { Can you add a comment for this too? http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc@243 PS11, Line 243: AES128ECB = 0xFD, : AES192ECB = 0XFE, : AES256ECB = 0xFF, What's the rationale behind these values, vs just continuing at 0x03? http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc File src/kudu/util/file_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103 PS8, Line 103: unique_ptr<RWFile> f; : RWFileOptions opts; : opts.is_sensitive = true; : RETURN_NOT_OK(env_->NewRWFile(opts, name, &f)); : RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data)); > Well, it's not exactly the same, but your proposed approach and PS3 share t Looking through this again, it seems like whenever we deal with Read() and Write() calls, we are usually supplying logical offsets. In that regard, it seems for those particular APIs we might be able to get by with using logical size. Re: WALs, perhaps we could extend the file API to have some OffsetFromEnd(int bytes_from_end) that gives a byte offset usable via Read/Write calls. LBM though, I agree, seems like a different beast entirely, because we persist the physical offsets in log block metadata, and thus cannot use a logical-offset-based scheme. I do think the comments in this patch don't thoroughly explain this. It'd certainly help readers present and future better understand how and why it isn't so straightforward to ubiquitously use logical offsets. -- To view, visit http://gerrit.cloudera.org:8080/18025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd Gerrit-Change-Number: 18025 Gerrit-PatchSet: 11 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: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 25 Jan 2022 02:26:51 +0000 Gerrit-HasComments: Yes
