Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17974 )
Change subject: [encryption] KUDU-3331 Encrypt file system ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/17974/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17974/3//COMMIT_MSG@15 PS3, Line 15: I also changed the way the "encrypted" flag in *FileOptions works: nit: maybe it's worth calling this "is_sensitive" to avoid confusion http://gerrit.cloudera.org:8080/#/c/17974/3//COMMIT_MSG@19 PS3, Line 19: The default is : now true >From 'grep -r "FileOptions" . --exclude="*test*" --exclude="*env_posix*"', a >few stand out as things we should consider disabling encryption for (based on >this commit message, anyway): - fs/dir_manager.cc when copying instance metadata files - fs/dir_util.cc when creating instance metadata files - consensus/log_index.cc for the log index -- it doesn't store data, so do we actually care to encrypt it? - util/env_util.cc in OpenFileForWrite (without 'opts' input), though this is only used in tests right now In general I'm still not sold on changing the default, though I'd like to better understand the complications that you've run into. http://gerrit.cloudera.org:8080/#/c/17974/3//COMMIT_MSG@20 PS3, Line 20: not having to complicate : FileCache. Could you elaborate on this a bit more -- what would go wrong if we left the default as false and relied on callers manually request encryption? Wouldn't that just mean the FileCache::ReopenFileIfNecessary() needs to set the encryption option to true? http://gerrit.cloudera.org:8080/#/c/17974/3//COMMIT_MSG@28 PS3, Line 28: we can keep FileCache simpler contains block : location nit: I'm having trouble parsing this sentence. Mind rephrasing? I'm guessing the point is that it's simpler to implement when the entire FileCache is either encrypted or not. http://gerrit.cloudera.org:8080/#/c/17974/3//COMMIT_MSG@32 PS3, Line 32: and tablet metadata files. Why is this the case again? There's a lot you can infer from just a tablet metadata file alone (table schema, partitioning, data size, etc.). The design doc mentions both needing to be encrypted. I can see an argument for consensus metadata, but tablet metadata seems critical. -- 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 20:51:05 +0000 Gerrit-HasComments: Yes
