libroach: add basic file encryption stats. #26802

mberhault · 2018-06-18T18:45:04Z

Count the number of files and bytes reported by rocksdb.
Return totals for all files and for files using the active data key.

This is meant to be a rough indication of encryption progress when
changing key. Per-key ID stats will likely be necessary.

There is no synchronization between rocksdb and the file registry so
numbers may be off (eg: files deleted between calls). The active key may
also differ from the one contained in the encryption_status field.

Release note (general change): add encryption progress to stores debug page

cockroach-teamcity · 2018-06-18T18:45:10Z

This change is

mberhault

I still want to add a test that creates a full DB, checks the stats against the filesystem, restarts with different encryption settings, and checks again.

mberhault · 2018-06-18T18:45:23Z

c-deps/libroach/db.cc

@@ -43,7 +43,7 @@ namespace cockroach {
 // DBOpenHook in OSS mode only verifies that no extra options are specified.
 __attribute__((weak)) rocksdb::Status DBOpenHook(std::shared_ptr<rocksdb::Logger> info_log,
                                                 const std::string& db_dir, const DBOptions opts,
-                                                 EnvManager* env_ctx) {
+                                                 EnvManager* env_mgr) {


I forgot to rename this one with the rest.

mberhault · 2018-06-18T18:45:39Z

c-deps/libroach/file_registry.cc

@@ -13,8 +13,8 @@
 // permissions and limitations under the License.

 #include "file_registry.h"
-#include "../fmt.h"


not sure how I ended up doing that.

mberhault · 2018-06-19T12:44:53Z

Added a test that starts in plaintext, forces a compaction (to have a SST that remains in plaintext), re-opens with AES, and checks that the number of files using the active key is somewhere in [1, total-files).

bdarnell · 2018-06-20T22:29:01Z

Reviewed 16 of 17 files at r1, 3 of 3 files at r2, 4 of 4 files at r3.
Review status: complete! 0 of 0 LGTMs obtained

c-deps/libroach/engine.cc, line 214 at r3 (raw file):

  if (env_mgr->env_stats_handler == nullptr || env_mgr->file_registry == nullptr) {
    // We can't compute these if we don't have a file registry or stats handler.
    // This happens in OSS mode or when encryption has not been turned on.

Why isn't this code on the CCL side if it's never run in a pure-OSS build?

OTOH, the "total" stats at least seem like they could be computed in an OSS build.

c-deps/libroach/engine.cc, line 233 at r3 (raw file):

  // Only files accounted for by rocksdb are included. This will ignore files multiple types
  // of files:
  // - files that are no longer live (eg: older OPTIONS files, files pending deletion)

If you're rotating keys because of a breach, the relevant concern is not when all the live data is using the new key, but when the last file that was using the old key has been deleted (you may also want some sort of overwriting deletion). I don't know if we should worry about that here, though.

c-deps/libroach/engine.cc, line 242 at r3 (raw file):

  // do not call DisableFileDeletions. This means that some files may be listed by rocksdb but not
  // present in the file registry (deleted between calls). Such files will be considered
  // "plaintext".

This seems like a problem for any monitoring that would want to alert if there are any plaintext files.

c-deps/libroach/utils.cc, line 40 at r3 (raw file):

    return path1 + path2;
  }
  return path1 + '/' + path2;

Do we need a TODO about windows here?

c-deps/libroach/include/libroach.h, line 317 at r3 (raw file):

  uint64_t total_files;
  uint64_t total_bytes;
  // Files/bytes using the active data key.

Rename these to active_key_{bytes,files}. The context doesn't otherwise make it clear that this is encryption-related (and sounds like it could have something to do with live/dead files)

pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 74 at r3 (raw file):

    return [
      this.renderSimpleAlternateRow("Encryption Progress", "Fraction encrypted using the active data key"),

In the screenshot this looks like a key and value, not a subheading. Can we style this line differently (and make it a single cell)?

Comments from Reviewable

mberhault · 2018-06-21T12:37:43Z

Review status: complete! 0 of 0 LGTMs obtained

c-deps/libroach/engine.cc, line 214 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Why isn't this code on the CCL side if it's never run in a pure-OSS build?

OTOH, the "total" stats at least seem like they could be computed in an OSS build.

It's technically tied to the FileRegistry, with only a CCL hook to resolve key IDs. I think it'll be reasonable to compute the stats we can in OSS / no-file-registry mode, but I started out with the simple version.

c-deps/libroach/engine.cc, line 233 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

If you're rotating keys because of a breach, the relevant concern is not when all the live data is using the new key, but when the last file that was using the old key has been deleted (you may also want some sort of overwriting deletion). I don't know if we should worry about that here, though.

I think we'll need to worry about it before release. If the stats tell you 100% of the files are using the desired encryption settings, that needs to be true. This means:

disable deletions (easy and can be called multiple times: it's a counter. This means we don't have to worry about other callers). File creations should be fine as long as we scan the registry after the rocksdb calls.
we may want to traverse the file registry (easy) and maybe even the filesystem (tricky, there are too many random files that can show up in our base dir. eg: logs)
we may need to have a "unknown" category for files without an entry in the FileRegistry.
we should create a FileRegistry entry for plaintext files as well.

The combination of those would account for all files created when the FileRegistry is in use. The transition to the FileRegistry on-disk format would still leave files unaccounted for but would be shorter lived. If we recommend that people start their stores with encryption, the FileRegistry will be accurate.

I've added some todos for these proposals. I'll need to experiment more though.

c-deps/libroach/engine.cc, line 242 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This seems like a problem for any monitoring that would want to alert if there are any plaintext files.

Definitely. Even if deletions while we're scanning are rare, it just takes one false positive to seriously irritate admins. Accurate numbers are definitely required. I think the proposal above will get us most of the way there (format transitions are still a pain, but we can detect those).

c-deps/libroach/utils.cc, line 40 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Do we need a TODO about windows here?

Unsure. I still need to test all this on windows. But rocksdb itself hard-codes '/' all over the place (eg: the filename building utilities are all db_dir + "/" + etc...).

c-deps/libroach/include/libroach.h, line 317 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Rename these to active_key_{bytes,files}. The context doesn't otherwise make it clear that this is encryption-related (and sounds like it could have something to do with live/dead files)

Done.

pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 74 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

In the screenshot this looks like a key and value, not a subheading. Can we style this line differently (and make it a single cell)?

I've moved the UI change to a separate PR for now. But yes, I'll tweak that. I'll re-send that PR after some other UI changes I need to do on it (loading spinner, use of cached something-or-other).

Comments from Reviewable

mberhault · 2018-06-21T12:48:05Z

UI components are being moved to a separate PR: #26890

requires cockroachdb#26802 for new `StoresResponse` fields. This adds basic file stats to the stores report page. Also improves the styling: - show decoded key info protobuf fields rather than raw proto (eg: creation date rather than unix timestamp) - table styling moved to core style file - full-width cells to head different sections Release note (admin ui change): add encryption statistics on stores report page

bdarnell · 2018-06-25T22:09:38Z

Reviewed 9 of 13 files at r4.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

Comments from Reviewable

mberhault · 2018-06-28T06:39:15Z

bors r+

craig · 2018-06-28T08:02:20Z

Timed out

mberhault · 2018-06-28T08:46:04Z

bors r+

requires cockroachdb#26802 for new `StoresResponse` fields. This adds basic file stats to the stores report page. Also improves the styling: - show decoded key info protobuf fields rather than raw proto (eg: creation date rather than unix timestamp) - table styling moved to core style file - full-width cells to head different sections Release note (admin ui change): add encryption statistics on stores report page

craig · 2018-06-28T12:02:20Z

Timed out

mberhault · 2018-06-28T19:03:47Z

bors r+

TC is finally not taking forever.

craig · 2018-06-28T20:20:56Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2018-06-28T21:37:27Z

Build failed (retrying...)

GitHub CI (Cockroach)

mberhault · 2018-06-29T14:52:31Z

bors r+

craig · 2018-06-29T15:07:54Z

Build failed

GitHub CI (Cockroach)

Count the number of files and bytes reported by rocksdb. Return totals for all files and for files using the active data key. This is meant to be a rough indication of encryption progress when changing key. Per-key ID stats will likely be necessary. There is no synchronization between rocksdb and the file registry so numbers may be off (eg: files deleted between calls). The active key may also differ from the one contained in the encryption_status field. Release note (general change): add encryption progress to stores debug page

mberhault · 2018-06-29T15:53:58Z

bors r+

Now that TC is green, maybe bors-triggered TC will be as well?

26802: libroach: add basic file encryption stats. r=mberhault a=mberhault Count the number of files and bytes reported by rocksdb. Return totals for all files and for files using the active data key. This is meant to be a rough indication of encryption progress when changing key. Per-key ID stats will likely be necessary. There is no synchronization between rocksdb and the file registry so numbers may be off (eg: files deleted between calls). The active key may also differ from the one contained in the encryption_status field. Release note (general change): add encryption progress to stores debug page Co-authored-by: marc <[email protected]>

craig · 2018-06-29T16:15:29Z

Build succeeded

GitHub CI (Cockroach)

requires cockroachdb#26802 for new `StoresResponse` fields. This adds basic file stats to the stores report page. Also improves the styling: - show decoded key info protobuf fields rather than raw proto (eg: creation date rather than unix timestamp) - table styling moved to core style file - full-width cells to head different sections Release note (admin ui change): add encryption statistics on stores report page

Address TODOs from cockroachdb#26802 for more accurate file count/size calculation. Specifically: - plaintext files now have an entry in the file_registry - traverse file_registry when counting files - disable rocksdb deletions during the scan Still some TODOs to improve usability (eg: report "unknown" files, logic for older versions of files that stick around) but this should already drastically improve file stats reporting. Release note: None

27388: libroach: more accurate encryption file statistics. r=mberhault a=mberhault Address TODOs from #26802 for more accurate file count/size calculation. Specifically: - plaintext files now have an entry in the file_registry - traverse file_registry when counting files - disable rocksdb deletions during the scan Still some TODOs to improve usability (eg: report "unknown" files, logic for older versions of files that stick around) but this should already drastically improve file stats reporting. Release note: None Co-authored-by: marc <[email protected]>

Address TODOs from cockroachdb#26802 for more accurate file count/size calculation. Specifically: - plaintext files now have an entry in the file_registry - traverse file_registry when counting files - disable rocksdb deletions during the scan Still some TODOs to improve usability (eg: report "unknown" files, logic for older versions of files that stick around) but this should already drastically improve file stats reporting. Release note: None

requires cockroachdb#26802 for new `StoresResponse` fields. This adds basic file stats to the stores report page. Also improves the styling: - show decoded key info protobuf fields rather than raw proto (eg: creation date rather than unix timestamp) - table styling moved to core style file - full-width cells to head different sections Release note (admin ui change): add encryption statistics on stores report page

mberhault requested review from bdarnell and a team June 18, 2018 18:45

mberhault commented Jun 18, 2018

View reviewed changes

mberhault force-pushed the marc/basic_encryption_stats branch from ea8f83b to 8fb1048 Compare June 19, 2018 12:38

mberhault mentioned this pull request Jun 19, 2018

security: encryption at rest #19783

Closed

29 tasks

mberhault force-pushed the marc/basic_encryption_stats branch 2 times, most recently from 3a10225 to 78b07fc Compare June 19, 2018 17:53

mberhault requested a review from a team June 19, 2018 18:22

mberhault force-pushed the marc/basic_encryption_stats branch 2 times, most recently from 4399aaf to dc95a56 Compare June 19, 2018 19:42

mberhault force-pushed the marc/basic_encryption_stats branch from dc95a56 to 78b07fc Compare June 21, 2018 12:10

mberhault mentioned this pull request Jun 21, 2018

ui: encryption stats on stores report #26890

Merged

mberhault force-pushed the marc/basic_encryption_stats branch from 620689c to f239f8b Compare June 28, 2018 06:32

mberhault force-pushed the marc/basic_encryption_stats branch from f239f8b to 5a80329 Compare June 29, 2018 15:09

craig bot merged commit 5a80329 into cockroachdb:master Jun 29, 2018

mberhault deleted the marc/basic_encryption_stats branch June 29, 2018 17:47

mberhault mentioned this pull request Jul 11, 2018

libroach: more accurate encryption file statistics. #27388

Merged

jseldess mentioned this pull request Jul 31, 2018

libroach: add basic file encryption stats. cockroachdb/docs#3477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libroach: add basic file encryption stats. #26802

libroach: add basic file encryption stats. #26802

mberhault commented Jun 18, 2018 •

edited

Loading

cockroach-teamcity commented Jun 18, 2018

mberhault left a comment

mberhault Jun 18, 2018

mberhault Jun 18, 2018

mberhault commented Jun 19, 2018

bdarnell commented Jun 20, 2018

mberhault commented Jun 21, 2018

mberhault commented Jun 21, 2018

bdarnell commented Jun 25, 2018

mberhault commented Jun 28, 2018

craig bot commented Jun 28, 2018

mberhault commented Jun 28, 2018

craig bot commented Jun 28, 2018

mberhault commented Jun 28, 2018

craig bot commented Jun 28, 2018

craig bot commented Jun 28, 2018

mberhault commented Jun 29, 2018

craig bot commented Jun 29, 2018

mberhault commented Jun 29, 2018

craig bot commented Jun 29, 2018

libroach: add basic file encryption stats. #26802

libroach: add basic file encryption stats. #26802

Conversation

mberhault commented Jun 18, 2018 • edited Loading

cockroach-teamcity commented Jun 18, 2018

mberhault left a comment

Choose a reason for hiding this comment

mberhault Jun 18, 2018

Choose a reason for hiding this comment

mberhault Jun 18, 2018

Choose a reason for hiding this comment

mberhault commented Jun 19, 2018

bdarnell commented Jun 20, 2018

mberhault commented Jun 21, 2018

mberhault commented Jun 21, 2018

bdarnell commented Jun 25, 2018

mberhault commented Jun 28, 2018

craig bot commented Jun 28, 2018

Timed out

mberhault commented Jun 28, 2018

craig bot commented Jun 28, 2018

Timed out

mberhault commented Jun 28, 2018

craig bot commented Jun 28, 2018

Build failed (retrying...)

craig bot commented Jun 28, 2018

Build failed (retrying...)

mberhault commented Jun 29, 2018

craig bot commented Jun 29, 2018

Build failed

mberhault commented Jun 29, 2018

craig bot commented Jun 29, 2018

Build succeeded

mberhault commented Jun 18, 2018 •

edited

Loading