Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libroach: add basic encryption stats. #25850

Merged
merged 1 commit into from
May 24, 2018

Conversation

mberhault
Copy link
Contributor

New endpoint: /_status/stores which for now only returns the store ID
and encryption stats.

The encryption stats is a serialized protobuf containing active key
information (but not the key itself).

This will be handled by CCL UI code since it needs access to CCL protos.

Release note: None

@mberhault mberhault requested review from bdarnell and a team May 23, 2018 15:08
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mberhault mberhault mentioned this pull request May 23, 2018
29 tasks
@petermattis
Copy link
Collaborator

Review status: 0 of 28 files reviewed at latest revision, all discussions resolved.


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

  std::string encryption_status;
  auto status = this->env_mgr->env_stats_handler->GetEncryptionStats(&encryption_status);

It feels a bit weird for GetEncryptionStats to pass back the stats in a string. If there is every more than one implementation of GetEncryptionStats, we'll need all of them to encode the same proto. That makes me think that GetEncryptionStats should populate a proto and this method should be where the proto is serialized to a string.

Note that in other places we pass stats from C++ to Go using C structs. For example, MVCCStatsResult. I'm curious why we're using a proto here.


c-deps/libroach/testutils.h, line 24 at r1 (raw file):

// Returns initialized DBOptions with reasonable values for unittests.
DBOptions defaultDBOptions() {

I think you need to mark this as inline. It's ok to place templated methods in a header, but for non-templated methods if you don't mark them as inline you can hit problems during linking with multiple definitions. This is dependent on the linker (i.e. it succeed on Linux but fail on Windows).


pkg/server/serverpb/status.proto, line 609 at r1 (raw file):

message StoresRequest {
  // TODO(tamird): use [(gogoproto.customname) = "NodeID"] below. Need to

I'm guessing tamird isn't going to get to this. Let's either remote the TODO or put someone else's name there.


pkg/storage/engine/engine.go, line 347 at r1 (raw file):

// EnvStats is a set of RocksDB env stats, including encryption status.
type EnvStats struct {
	// EncryptionStatus is a serialized	ccl/storageccl/engineccl/enginepbccl/stats.go::EncryptionStatus protobuf.

s/stats.go/stats.proto/g? Not sure if you need the full path above. It also makes the line very long.


Comments from Reviewable

@bdarnell
Copy link
Contributor

Reviewed 28 of 28 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 4 unresolved discussions.


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

Previously, petermattis (Peter Mattis) wrote…

It feels a bit weird for GetEncryptionStats to pass back the stats in a string. If there is every more than one implementation of GetEncryptionStats, we'll need all of them to encode the same proto. That makes me think that GetEncryptionStats should populate a proto and this method should be where the proto is serialized to a string.

Note that in other places we pass stats from C++ to Go using C structs. For example, MVCCStatsResult. I'm curious why we're using a proto here.

The problem here is that the OSS code doesn't have access to the proto, it's deeply CCL only. This serialized proto is generated in ccl/db.cc and will only be read by CCL code in the UI (and potentially CCL hooks into metrics). The OSS code just passes around an opaque blob.

This is an unfortunate thing we've had to do over and over again for encryption due to the many components that need to talk to each other through OSS.


c-deps/libroach/testutils.h, line 24 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think you need to mark this as inline. It's ok to place templated methods in a header, but for non-templated methods if you don't mark them as inline you can hit problems during linking with multiple definitions. This is dependent on the linker (i.e. it succeed on Linux but fail on Windows).

Done.


pkg/server/serverpb/status.proto, line 609 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm guessing tamird isn't going to get to this. Let's either remote the TODO or put someone else's name there.

Removed TODO.


pkg/storage/engine/engine.go, line 347 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/stats.go/stats.proto/g? Not sure if you need the full path above. It also makes the line very long.

Done. And shrunk to enginepbccl/stats.proto.


Comments from Reviewable

New endpoint: `/_status/stores` which for now only returns the store ID
and encryption stats.

The encryption stats is a serialized protobuf containing active key
information (but not the key itself).

This will be handled by CCL UI code since it needs access to CCL protos.

Release note: None
@mberhault mberhault force-pushed the marc/surface_encryption_status branch from 875600d to 8d2ae81 Compare May 23, 2018 19:48
@petermattis
Copy link
Collaborator

Can we add an end-to-end test in the CCL code that the new endpoint returns non-empty encryption status?


Review status: 23 of 28 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


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

Previously, mberhault (marc) wrote…

The problem here is that the OSS code doesn't have access to the proto, it's deeply CCL only. This serialized proto is generated in ccl/db.cc and will only be read by CCL code in the UI (and potentially CCL hooks into metrics). The OSS code just passes around an opaque blob.

This is an unfortunate thing we've had to do over and over again for encryption due to the many components that need to talk to each other through OSS.

Ack. It doesn't seem problematic for the OSS code to define an encryption stats struct that is used by CCL. If you leave the code as-is, at least add a comment alluding to the hoops which are causing this structure.

Update: I completely missed that the stats fields are KeyInfo proto. You could still use a struct, though I suppose that is more onerous that I was imagining when I suggested it.


c-deps/libroach/ccl/db.cc, line 42 at r2 (raw file):

    bool has_stats = false;
    if (this->store_key_manager_ != nullptr) {

Nit: no need for the explicit this->. We don't use this in other parts of our C++ code.


c-deps/libroach/ccl/db.cc, line 45 at r2 (raw file):

      has_stats = true;
      // Transfer ownership of new key info to status proto, this frees any previous value.
      enc_status.set_allocated_active_store_key(store_key_manager_->CurrentKeyInfo().release());

Transferring ownership is surprising. I'm also surprised you don't need any locking here. Seems like calling DBGetEnvStats concurrently is unsafe, and calling it consecutively will return different values.


pkg/storage/engine/engine.go, line 347 at r1 (raw file):

Previously, mberhault (marc) wrote…

Done. And shrunk to enginepbccl/stats.proto.

Nit: you've got some spurious spaces after the word "serialized".


Comments from Reviewable

@mberhault
Copy link
Contributor Author

bors r=bdarnell

craig bot pushed a commit that referenced this pull request May 24, 2018
25850: libroach: add basic encryption stats. r=bdarnell a=mberhault

New endpoint: `/_status/stores` which for now only returns the store ID
and encryption stats.

The encryption stats is a serialized protobuf containing active key
information (but not the key itself).

This will be handled by CCL UI code since it needs access to CCL protos.

Release note: None

Co-authored-by: marc <marc@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented May 24, 2018

Build succeeded

@craig craig bot merged commit 8d2ae81 into cockroachdb:master May 24, 2018
@mberhault mberhault deleted the marc/surface_encryption_status branch May 24, 2018 12:12
@petermattis
Copy link
Collaborator

Was this merged prematurely? I didn't see an LGTM or an approved. My comment below about locking needs an answer. Am I missing something?


Review status: 23 of 28 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

uh. oops. that was definitely premature. I'll address the nits in a followup PR. The locking isn't needed though.


Review status: 23 of 28 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


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

Previously, petermattis (Peter Mattis) wrote…

Ack. It doesn't seem problematic for the OSS code to define an encryption stats struct that is used by CCL. If you leave the code as-is, at least add a comment alluding to the hoops which are causing this structure.

Update: I completely missed that the stats fields are KeyInfo proto. You could still use a struct, though I suppose that is more onerous that I was imagining when I suggested it.

There's a struct for the returns from GetEnvStats into go code, but the encryption_status field is just that opaque blob as it reuses a lot of the CCL-only protos. Eventually other stats will be OSS (eg: file/size counts) since those use OSS fields and will be in that same struct. Since this is the only field touched by GetEncryptionStats, it doesn't need to get the outer DBEnvStatsResult.


c-deps/libroach/ccl/db.cc, line 42 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: no need for the explicit this->. We don't use this in other parts of our C++ code.

Good point. Removing in upcoming PR.


c-deps/libroach/ccl/db.cc, line 45 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Transferring ownership is surprising. I'm also surprised you don't need any locking here. Seems like calling DBGetEnvStats concurrently is unsafe, and calling it consecutively will return different values.

The object returned by CurrentKeyInfo is a unique_ptr of a copy of the current key, so it needs to be released to prevent deletion of the KeyInfo. The store_key_manager does not currently lock as it is write-once (at initialization time, locking is a TODO once we add post-initialization reload).


pkg/storage/engine/engine.go, line 347 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: you've got some spurious spaces after the word "serialized".

oops. fixing in upcoming PR.


Comments from Reviewable

mberhault pushed a commit to mberhault/cockroach that referenced this pull request May 24, 2018
Nothing major, followups to premature merge of cockroachdb#25850

Release note: None
craig bot pushed a commit that referenced this pull request May 24, 2018
26044: misc: fix encryption stats nits. r=mberhault a=mberhault

Nothing major, followups to premature merge of #25850

Release note: None

26048: tpccbench: fix existing dataset fast-path r=nvanbenschoten a=nvanbenschoten

This skewed with #24735, which changed the output of `information_schema`
when connecting to an implicit database.

Release note: None

Co-authored-by: marc <marc@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@petermattis
Copy link
Collaborator

Review status: 23 of 28 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


c-deps/libroach/ccl/db.cc, line 45 at r2 (raw file):

Previously, mberhault (marc) wrote…

The object returned by CurrentKeyInfo is a unique_ptr of a copy of the current key, so it needs to be released to prevent deletion of the KeyInfo. The store_key_manager does not currently lock as it is write-once (at initialization time, locking is a TODO once we add post-initialization reload).

Ah, the comment was confusing me. It sounds like you're transferring ownership from store_key_manager_, but really you're transferring ownership for the unique pointer. I think the comment should be adjusted.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants