You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/01/25 09:12:32 UTC
[kudu-CR] WIP [master] store CA information in the system table
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5793
Change subject: WIP [master] store CA information in the system table
......................................................................
WIP [master] store CA information in the system table
Use CatalogManager::SetCAInfo() to set CA private key and certificate.
TBD: unit tests
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 436 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/1
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#5).
Change subject: WIP [master] store CA information in the system table
......................................................................
WIP [master] store CA information in the system table
Use CatalogManager::SetCAInfo() to set CA private key and certificate.
TBD: intergration tests with multi-master setup
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
11 files changed, 620 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/5
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#8).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/mini_master.h
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
15 files changed, 947 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/8
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS11, Line 179: // Private key body in PEM format.
: required bytes private_key = 1 [(kudu.REDACT) = true];
: // Certificate body in PEM format.
:
> why PEM instead of DER? we use DER elsewhere for our PBs, and most of the d
I thought it might be better because that's the format to store the info. Also, I thought it might be better for troubleshooting.
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
Line 40: class MasterCertAuthorityTest;
> don't think this forward decl is necessary
But otherwise the compiler spits an error for the 'friend' statement below:
kudu/src/kudu/master/master_cert_authority.h:72:32: error:
no class named 'MasterCertAuthorityTest' in namespace 'kudu::master'
friend class ::kudu::master::MasterCertAuthorityTest;
Line 55: // bound to the aggregated server UUID. Does not require Init() to be called.
> should note whether this also initializes the instance using the given cert
I thought 'const' specified would be enough, but having this explicitly in the doc is better, I agree.
PS11, Line 77: // Mutex to protect access to variables below.
: Mutex lock_;
:
> I think you need to rebase -- the class no longer has a cert-signer member,
But object of this class is used to sign incoming CSRs in master_service.cc. That means we don't want to have the private key and the cert to be updated while SignServerCSR() is in the middle of operation, right?
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
PS11, Line 92: 00000000000000000000000000000001
> hm, I dont think there's any need to make this look like a tablet id. How a
Done
Line 516: *entry_id = str_id;
> can you avoid a copy here by making 'str_id' non-const and std::move?
Done
Line 583: ProcessEntries<SysCertAuthorityEntryPB, CERT_AUTHORITY_INFO>(processor);
> missing a RETURN_NOT_OK
Good catch. That was those issues with pre-processor parsing. I put extra braces and now it's fine.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#15).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 538 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/15
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(16 comments)
Thank you for review. I spot some extra things myself and decided to update the patch, thinking you haven't started reviewing it yet.
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 680: Status CatalogManager::CheckInitCertAuthority() {
> BTW, shouldn't there be a short circuit path here for users who aren't usin
Yep, calls to this method should not be done from the upper level if some security-related flag is not set (or, vice versa, is set). There is corresponding TODO at the call site.
I think we can add in a separate changelist -- that flag should disable security features for all the components.
PS10, Line 684: shared_ptr<security::PrivateKey> ca_private_key(
: std::make_shared<security::PrivateKey>());
: shared_ptr<security::Cert> ca_cert(std::make_shared<security::Cert>());
> Why do these need shared ownership?
Because the MasterCertAuthority has such interface. Or it's no longer true?
Line 688: auto info(make_shared<SysCertAuthorityEntryPB>());
> Why does this need shared ownership?
good point: this can be just an object on stack.
Line 697: if (PREDICT_TRUE(found_ca_info)) {
> Maybe just if s.ok() here? I don't think we're doing anything with found_ca
Done
Line 712: RETURN_NOT_OK(sys_catalog_->AddCertAuthorityEntry(*info));
> I don't think we should do this with lock_ held, since it requires disk and
Good point -- I already removed the lock. Thanks!
Line 715: }
> What would be the motivation for moving it?
I was not happy with it in here because MasterCertAuthority might have some logic which would be brittle to re-initting the object in the middle. But it's OK in its current implementation -- I'll remove this TODO.
Line 770: LOG_SLOW_EXECUTION(WARNING, 1000, LogPrefix() + "Loading CA info into memory") {
> This still needs to be refactored with VisitTablesAndTablets() so that lead
I thought the idea was not to do that twice only in this part of the code -- there were a couple methods acquiring the lock. Will fix.
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 689: const Status s = sys_catalog_->GetCertAuthorityEntry(info.get());
> So we're still going to need to cache this in memory so that we're not read
I'm not sure I understand. This method (i.e. CatalogManager::CheckInitCertAuthority) is called every time a master server becomes a leader. I thought it's done only when leadership role transfers from one server to another.
As for the usage of the certificate authority information, it's needed for the MasterCertAuthority -- that component signs tablet server's certificate requests every heartbeat. Once MasterCertAuthority::Init() has been called, the master will have the necessary data to sign those certificate requests.
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 34: #include "kudu/master/master_cert_authority.h"
> Don't need?
Done
PS10, Line 57: namespace master {
:
: class CatalogManagerBgTasks;
: class Master;
> Don't need these anymore?
Done
PS10, Line 529: // becomes the leader of a consensus configuration. Executes VisitTablesAndTabletsTask
: // via 'worker_pool_'.
> These no longer exist, right?
Done
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master.proto
File src/kudu/master/master.proto:
Line 182: required bytes certificate = 2;
> Talk to Dan about this; he suggested we may want to redact this too, not fo
Dan is not currently available in Slack channel, but I'll update this as soon as we make a decision.
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/sys_catalog-test.cc
File src/kudu/master/sys_catalog-test.cc:
Line 45: using std::unique_ptr;
> warning: using decl 'unique_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
Line 24: #include <vector>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS11, Line 40: template<typename T>
: class TupleInfo;
> Unused.
Done
Line 134: Status GetCertAuthorityEntry(SysCertAuthorityEntryPB* entry);
> Should be a unique_ptr<>, since the caller takes ownership, right?
The caller provides a placeholder for the result. I don't think we want unique_ptr here. But if you feel strong about having unique_ptr<> here, I will change it.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
PS11, Line 77: // Mutex to protect access to variables below.
: Mutex lock_;
:
> But object of this class is used to sign incoming CSRs in master_service.cc
But master_service shouldn't be trying to sign anyone's CSRs until after this thing is fully initted. And once initted, the value never changes, right? i.e this should be "externally synchronized". If you put the mutex here, it would hide the TSAN warnings that might tell us if we're messing up there and starting to sign certs before it's fully ready to go.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 694: // Block new catalog operations, and wait for existing operations to finish.
> This comment is probably no longer necessary. Certainly doesn't apply to th
Good catch!
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#16).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 547 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/16
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP [master] store CA information in the system table
......................................................................
Patch Set 2:
(3 comments)
Thank you for the review of this WIP-ish changelist. I'll continue tailoring this draft up to our needs.
http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 272: // The generic wrapper around {id, metadata} tuple from the system table's row.
> hrm, does this buy us much? I think we'll need sequence numbers on our keys
That's just set of fields necessary to plug those into the existing model of the catalog_manager.
The sequence numbers and other I thought to add into the corresponding PB structures/messages and expose via the wrappers as well (the wrappers might start diverging).
At this point it a lot of WIP-ish things.
http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS2, Line 181: // The identifier of the entry in the system table for the certificate's
: // private key. If empty, then the private key is stored along with the
: // certificate data itself.
: optional string private_key_id = 3;
> what's the purpose of the flexibility here? Aren't we always going to be st
Current code assumes we always store the key separately. The coment was about generic semantics of this field: I didn't want to make it a required field and this comment says what it would be meaning if the field were absent.
It turned to be a misleading comment since that functionality is not present now (we always store those separately).
Line 190: message SysCAPrivateKeyEntryPB {
> see above, not sure why it has to be stored separately?
The idea was to support the case when we have multiple CA certs based on the same private key. And I liked the idea to store separate entities in different records.
Say, for the certificate entry we could add additional sequence/serial numbers, expose validity dates via additional fields, etc. For private keys there might be different set of properties/fields we want to store along.
Do you think it's not worth it?
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
PS11, Line 77: // Mutex to protect access to variables below.
: Mutex lock_;
:
> But master_service shouldn't be trying to sign anyone's CSRs until after th
Sounds good -- will remove the mutex.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 680: Status CatalogManager::CheckInitCertAuthority() {
> Yep, calls to this method should not be done from the upper level if some s
Sounds good.
PS10, Line 684: shared_ptr<security::PrivateKey> ca_private_key(
: std::make_shared<security::PrivateKey>());
: shared_ptr<security::Cert> ca_cert(std::make_shared<security::Cert>());
> Because the MasterCertAuthority has such interface. Or it's no longer true
Oh okay, makes sense.
Line 715: }
> I was not happy with it in here because MasterCertAuthority might have some
I don't have an opinion one way or the other, was just wondering what yours was.
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 689: const Status s = sys_catalog_->GetCertAuthorityEntry(info.get());
> I'm not sure I understand. This method (i.e. CatalogManager::CheckInitCert
Oh, okay. I was confused; I thought the CatalogManager needed to maintain the cert/key in-memory to use when servicing heartbeats. If I'm understanding you correctly, you're saying that that in-memory representation already exists in MasterCertAuthority. Great.
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
Line 134: Status GetCertAuthorityEntry(SysCertAuthorityEntryPB* entry);
> The caller provides a placeholder for the result. I don't think we want un
Ah, right. If we were actually passing an allocated object back, this would have been a double pointer. As-is, it's an overwrite to a (probably) stack allocated object, and any nested heap pointers will be freed by the caller when that object goes out of scope.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS11, Line 179: // Private key body in PEM format.
: required bytes private_key = 1 [(kudu.REDACT) = true];
: // Certificate body in PEM format.
:
why PEM instead of DER? we use DER elsewhere for our PBs, and most of the data in the systable is already binary
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
Line 40: class MasterCertAuthorityTest;
don't think this forward decl is necessary
Line 55: // bound to the aggregated server UUID. Does not require Init() to be called.
should note whether this also initializes the instance using the given certs, or if the user should then call Init()
PS11, Line 77: // Mutex to protect access to variables below.
: Mutex lock_;
:
I think you need to rebase -- the class no longer has a cert-signer member, and I don't think we need shared_ptrs anymore.
Regarding thread-safety, I think given the intended use case is that the setup will be single-threaded, and that the SignServerCSR() call won't mutate any class state, a lock isn't necessary, right?
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
PS11, Line 92: 00000000000000000000000000000001
hm, I dont think there's any need to make this look like a tablet id. How about some string like 'root-ca-info' or somesuch?
Line 516: *entry_id = str_id;
can you avoid a copy here by making 'str_id' non-const and std::move?
Line 583: ProcessEntries<SysCertAuthorityEntryPB, CERT_AUTHORITY_INFO>(processor);
missing a RETURN_NOT_OK
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#9).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/mini_master.h
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
15 files changed, 948 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/9
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/15/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 199: using std::make_shared;
> warning: using decl 'make_shared' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 10:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:
Line 73: ADD_KUDU_TEST(master_replication-itest RESOURCE_LOCK "master-rpc-ports")
> Nit: sort alphabetically.
Done
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/integration-tests/master_cert_authority-itest.cc
File src/kudu/integration-tests/master_cert_authority-itest.cc:
PS9, Line 55: aserbin):
> Heh, this should be you now.
I thought I should not declare somebody's else thought as mine :)
Done.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS9, Line 337: void Wake() {
: MutexLock lock(lock_);
: pending_updates_ = true;
> If we're redacting the data (and we should; is it marked as secure in maste
Oh, I see. I forgot about the redaction feature -- will update the master.proto accordingly.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 288:
> Nit: terminate with period.
Done
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
PS9, Line 37: #include "kudu/rpc/messenger.h"
: #include "kudu/server/rpc_server.h"
> Are these used?
woops. No, they are not -- these are remnants from the tests I started here and then moved elsewhere.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master.cc
File src/kudu/master/master.cc:
Line 113: google::FlushLogFiles(google::INFO); // Flush the startup messages.
> Nit: remove this?
Done
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
Line 156: // TODO(aserbin): 7. Send any active CA certs which the TS doesn't have.
> This is TODO, presumably?
I think so: will add TODO.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog-test.cc
File src/kudu/master/sys_catalog-test.cc:
PS9, Line 110: essage& a,
> Not for ever?
As per result of our discussion in Slack, it seems it will be as you said.
Line 456
> Probably don't need this test anymore if we're not yet going to support DEL
Done
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
Line 538: RETURN_NOT_OK(iter->Init(&spec));
> You introduced these nice generic methods but then aren't using them in Vis
Good catch. Probably, I just forgot to re-factor that part. Will do in a separate changelist, thanks for the reminder.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS9, Line 138: Status AddCertAuthorityEntry(const SysCertAuthorityEntryPB& entry);
:
: private:
: FRIEND_TEST(MasterTest, TestMasterMetadataConsistentDespiteFailures);
: DISALLOW_COPY_AND_ASSIGN(SysCatalogTable);
> I don't think these should be here. All table/tablet changes are here so th
That's a good point. I think we want to write both the CA root key and corresponding certificate in a single transaction anyway, but yes -- it seems to be better if it were done in a simplified manner.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/12/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
Line 77: Mutex lock_;
ah, I see that you just rebased and got the shared_ptr thing. But I still am not sure the lock is necessary (and may be problematic during startup of a large cluster)
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(14 comments)
PS11 showed up as I was reviewing PS10, so some of my comments may no longer be relevant.
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 680: Status CatalogManager::CheckInitCertAuthority() {
BTW, shouldn't there be a short circuit path here for users who aren't using security at all?
PS10, Line 684: shared_ptr<security::PrivateKey> ca_private_key(
: std::make_shared<security::PrivateKey>());
: shared_ptr<security::Cert> ca_cert(std::make_shared<security::Cert>());
Why do these need shared ownership?
Line 688: auto info(make_shared<SysCertAuthorityEntryPB>());
> error: no template named 'make_shared'; did you mean 'std::make_shared'? [c
Why does this need shared ownership?
Line 697: if (PREDICT_TRUE(found_ca_info)) {
Maybe just if s.ok() here? I don't think we're doing anything with found_ca_info except that, and the indirection means one more piece of state to follow.
Line 712: RETURN_NOT_OK(sys_catalog_->AddCertAuthorityEntry(*info));
I don't think we should do this with lock_ held, since it requires disk and network IO.
Line 715: }
What would be the motivation for moving it?
Line 770: LOG_SLOW_EXECUTION(WARNING, 1000, LogPrefix() + "Loading CA info into memory") {
This still needs to be refactored with VisitTablesAndTablets() so that leader_lock_ is only acquired once, not twice.
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 689: const Status s = sys_catalog_->GetCertAuthorityEntry(info.get());
So we're still going to need to cache this in memory so that we're not reading from sys_catalog with every tserver heartbeat, right? Are you thinking of doing that in a separate patch?
http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 34: #include "kudu/master/master_cert_authority.h"
Don't need?
PS10, Line 57: namespace master {
:
: class CatalogManagerBgTasks;
: class Master;
Don't need these anymore?
PS10, Line 529: // becomes the leader of a consensus configuration. Executes VisitTablesAndTabletsTask
: // via 'worker_pool_'.
These no longer exist, right?
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master.proto
File src/kudu/master/master.proto:
Line 182: required bytes certificate = 2;
Talk to Dan about this; he suggested we may want to redact this too, not for security reasons, but just because it's not an interesting thing to log.
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS11, Line 40: template<typename T>
: class TupleInfo;
Unused.
Line 134: Status GetCertAuthorityEntry(SysCertAuthorityEntryPB* entry);
Should be a unique_ptr<>, since the caller takes ownership, right?
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#2).
Change subject: WIP [master] store CA information in the system table
......................................................................
WIP [master] store CA information in the system table
Use CatalogManager::SetCAInfo() to set CA private key and certificate.
TBD: unit tests
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 440 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/2
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#13).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 544 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/13
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#10).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 626 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/10
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#11).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 602 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/11
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP [master] store CA information in the system table
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS2, Line 181: // The identifier of the entry in the system table for the certificate's
: // private key. If empty, then the private key is stored along with the
: // certificate data itself.
: optional string private_key_id = 3;
> Current code assumes we always store the key separately. The coment was ab
Yea, I think given these are just internal-facing PBs, we shouldn't document "what-ifs" if they are things that don't actually happen.
Line 190: message SysCAPrivateKeyEntryPB {
> The idea was to support the case when we have multiple CA certs based on th
Yea, that makes sense, I forgot about other metadata that you might want to store with a key such as the timestamps, etc, and the idea that the cert and key may rotate at different frequencies.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#14).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 542 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/14
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#4).
Change subject: WIP [master] store CA information in the system table
......................................................................
WIP [master] store CA information in the system table
Use CatalogManager::SetCAInfo() to set CA private key and certificate.
TBD: intergration tests with multi-master setup
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
11 files changed, 652 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/4
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP [master] store CA information in the system table
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 272: // The generic wrapper around {id, metadata} tuple from the system table's row.
hrm, does this buy us much? I think we'll need sequence numbers on our keys and such anyway (which we'll expose out to clients), so even if it's slightly redundant, maybe we can just derive the table row from the key's sequence number, and then just pass around the PBs?
http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS2, Line 181: // The identifier of the entry in the system table for the certificate's
: // private key. If empty, then the private key is stored along with the
: // certificate data itself.
: optional string private_key_id = 3;
what's the purpose of the flexibility here? Aren't we always going to be storing the cert along with its key rather than separately?
Line 190: message SysCAPrivateKeyEntryPB {
see above, not sure why it has to be stored separately?
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#3).
Change subject: WIP [master] store CA information in the system table
......................................................................
WIP [master] store CA information in the system table
Use CatalogManager::SetCAInfo() to set CA private key and certificate.
TBD: unit tests
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 440 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/3
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#6).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
Use CatalogManager::SetCAInfo() to set CA private key and certificate.
Intergration tests with multi-master setup are coming in a separate
changelist.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
11 files changed, 658 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/6
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#12).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 537 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/12
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 16: Verified+1
I think the test failure in KerberosOnAndOff/ExternalMiniClusterTest.TestBasicOperation/0 is either unrelated or can be ignored since we have other patches in pipeline which are based on this patch.
Verified +1
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [master] store CA information in the system table
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 9:
(20 comments)
I didn't review the changes to master_cert_authority since I have no context for that stuff.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:
Line 73: ADD_KUDU_TEST(master_cert_authority-itest RESOURCE_LOCK "master-rpc-ports")
Nit: sort alphabetically.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/integration-tests/master_cert_authority-itest.cc
File src/kudu/integration-tests/master_cert_authority-itest.cc:
PS9, Line 55: afeinberg
Heh, this should be you now.
PS9, Line 164: *has_signed_certificate = true;
: *signed_certificate = ts_cert_str;
Maybe combine using a boost::optional<> or unique_ptr<> ?
Line 241: ASSERT_OK(WaitForLeader());
We didn't bring down any master, so haven't we already waited for the leader as part of SetUp()?
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS9, Line 337: // Not logging due to security reasons?
: //VLOG(1) << "Root CA private key " << id
: // << ": " << SecureShortDebugString(data);
If we're redacting the data (and we should; is it marked as secure in master.proto?) then this would be fine.
Line 785: CHECK_OK(VisitCertAuthorityInfo());
Could you restructure this code such that leader_lock_ is taken only once and held for the entire duration of these operations? Releasing/reacquiring leader_lock_ a couple times probably doesn't matter since the write to leader_ready_term_ has yet to happen, but I think we should treat all of this work as atomic w.r.t. leader elections, and holding leader_lock_ for the entire duration would be safer.
Line 1328: shared_ptr<CACertInfo> CatalogManager::CreateCACertInfo(
Could be static. Better yet, could be a factory function in CACertInfo.
Same for all of these, I think.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 288: // As in 'metadata' column of the system table
Nit: terminate with period.
Line 606:
Could you document these? I'm especially curious about 'id' vs. 'key_id'.
PS9, Line 787: Protected by lock_ and the special
: // protocol should be used to update both values
Here you talk about a special protocol to update both values, but steps 1 to 5 below only update ca_cert_, not ca_private_key_info_.
PS9, Line 794: the certificate
Nit: ca_cert_ to be more clear.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
PS9, Line 37: #include "kudu/security/openssl_util.h"
: #include "kudu/security/ca/cert_management.h"
Are these used?
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master.cc
File src/kudu/master/master.cc:
Line 113:
Nit: remove this?
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
Line 156: // 7. Send any active CA certs which the TS doesn't have.
This is TODO, presumably?
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog-test.cc
File src/kudu/master/sys_catalog-test.cc:
PS9, Line 110: As for now,
Not for ever?
Line 456: TEST_F(SysCatalogTest, UpdateCertAuthorityInfo) {
Probably don't need this test anymore if we're not yet going to support DELETE.
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
Line 538: Status SysCatalogTable::VisitEntries(V* visitor) const {
You introduced these nice generic methods but then aren't using them in VisitTables()/VisitTablets(). Why not?
http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS9, Line 64: class CACertVisitor {
: public:
: virtual Status Visit(const std::string& id,
: const SysCACertEntryPB& data) = 0;
: };
:
: class CAPrivateKeyVisitor {
: public:
: virtual Status Visit(const std::string& id,
: const SysCAPrivateKeyEntryPB& data) = 0;
: };
Given that there's either zero or one of these rows, I don't think a visitor pattern makes sense. Perhaps provide a higher level function like this:
Status GetCACertAndKey(unique_ptr<pair<SysCACertentryPB, SysCAPrivateKeyEntryPB>>* out) const;
'out' could be null if we've yet to write a CA key/cert.
This function could also enforce that there aren't multiple entries in the tablet, returning a bad status (or even a CHECK()) if that's the case.
PS9, Line 92: static const char* const kSysCaPrivateKeyId;
: static const char* const kSysCaCertId;
I think this implies that there's only one root CA private key and certificate at any given time in the tablet, but that (very important) semantic isn't stated anywhere in this change AFAICT. It probably belongs in several places: both master.proto and somewhere here or in catalog_manager.
PS9, Line 138: CACertInfo* ca_cert_to_add;
: CACertInfo* ca_cert_to_delete;
:
: CAPrivateKeyInfo* ca_private_key_to_add;
: CAPrivateKeyInfo* ca_private_key_to_delete;
I don't think these should be here. All table/tablet changes are here so that they can all be encapsulated in a single transaction, but that's not a requirement for CA cert/key operations.
Instead, let's have SysCatalogTable provide functions that mirror the desired use cases of the data. We clearly need a ProcessTableActions() that takes a struct Actions and makes some table/tablet changes in a single transaction.
Then, for the root CA cert/key data, we should provide just one function that INSERTs a pair, since that's the only functionality we need right now. That doesn't even need to be encapsulated in a struct; they could just be floating arguments to a InsertCACertAndKey() function (or some such).
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 15: Code-Review+2
Looks good to me, leaving open for Todd though.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 689: const Status s = sys_catalog_->GetCertAuthorityEntry(info.get());
> Oh, okay. I was confused; I thought the CatalogManager needed to maintain t
Yep, the MasterCertAuthority holds that data in the memory and uses appropriately when a heartbeat with CSR arrives from a tablet server. So, here we just want to load that data and pass it to MasterCertAuthority which takes care of the rest.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [master] store CA information in the system table
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [master] store CA information in the system table
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5793/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 694: // Block new catalog operations, and wait for existing operations to finish.
This comment is probably no longer necessary. Certainly doesn't apply to the next line.
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5793
to look at the new patch set (#7).
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/mini_master.h
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
15 files changed, 945 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/7
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] store CA information in the system table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [master] store CA information in the system table
......................................................................
[master] store CA information in the system table
The certificate authority information (private key and corresponding
CA certificate) is generated upon first start of Kudu cluster and
persistently stored in the system table.
Added simple unit tests for the corresponding entries in the
catalog table. Also, added integration tests to verify the operation
in case of multi-master clusters.
The next step would be adding a way to re-generate CA information.
Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Reviewed-on: http://gerrit.cloudera.org:8080/5793
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 547 insertions(+), 49 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>