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 2018/01/18 07:05:43 UTC
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9052
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
WIP [master] no half-baked responses on ConnectoToMaster
Do not send half-baked responses to clients from master that declares
itself a leader when its catalog manager is not yet in proper state.
WIP: already existing AuthTokenIssuingTest.ChannelConfidentiality test
provides some coverage for the new code, but maybe a dedicated
test is needed?
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/master_service.cc
2 files changed, 14 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/1
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when its catalog
: // manager is not in proper state yet.
> Are you sure that's not by design? Todd added ConnectToMaster and I think t
I'm not sure this is by design. My point is that in case if a leader is not in OK state, it's not usable as a leader anyway. Why to report such server as a leader master if it cannot be used as a leader master in the sense that client assumes it to be the case if it gets success response marked "I'm the leader master"?
The only useful info from this method in that case seems to be the set of other master servers' end-points. Do you mean that information should always be included into the response? But then I don't understand why we don't include that in the case if code at line 456 fires.
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 'role' is LEADER but
> This comment seems relevant.
That's about some different case, actually -- when leader_status.ok() but no CA cert or TSK has been generated yet.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
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...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:19:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> yea, in that case I think we should send an error back to the client anyway
It's a good idea, indeed.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 03:22:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#5).
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectoToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority/TokenSigner are not
in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 238 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/5
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@477
PS5, Line 477: if (cert_authority->is_initialized()) {
isn't this guaranteed? leader_status() only is OK in the case that this node is the leader and that PrepareForLeadershipTask() has completed. That function unconditionally inits the CA.
I tried running your tests with a CHECK(cert_authority->is_initialized()) here and it still passed. So perhaps that portion of this patch can be removed and the original call can be left in place, which assumes ca_cert_der() is always available.
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
can we just simplify this to the approach outlined in the JIRA? ie:
// Rather than consulting the current consensus role, instead
// base it on the catalog manager's view. This prevents us
// from advertising LEADER until we have taken over all the
// associated responsibilities.
resp->set_role(l.leader_status().ok() ? LEADER : FOLLOWER);
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 00:02:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#2).
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
WIP [master] no half-baked responses on ConnectoToMaster
Do not send half-baked responses to clients from master that declares
itself a leader when its catalog manager is not yet in proper state.
WIP: already existing AuthTokenIssuingTest.ChannelConfidentiality test
provides some coverage for the new code, but maybe a dedicated
test is needed?
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/master_service.cc
2 files changed, 15 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/2
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/catalog_manager.cc@224
PS8, Line 224: during
> s/during loading/while reading
Done
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/catalog_manager.cc@236
PS8, Line 236:
> extra line
Done
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/master_cert_authority.h@57
PS8, Line 57: Does not require Init() to be called.
: // Calling this method does not have side-effects on the instance, i.e.
: // even this method has been called on object, it's still necessary
: // to call Init() prior to calling SignServerCSR() method.
> I think this info about Init can be omitted now since it's static.
Ah, right. Good catch.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 8
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jan 2018 01:36:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/catalog_manager.cc@224
PS8, Line 224: during
s/during loading/while reading
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/catalog_manager.cc@236
PS8, Line 236:
extra line
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
http://gerrit.cloudera.org:8080/#/c/9052/8/src/kudu/master/master_cert_authority.h@57
PS8, Line 57: Does not require Init() to be called.
: // Calling this method does not have side-effects on the instance, i.e.
: // even this method has been called on object, it's still necessary
: // to call Init() prior to calling SignServerCSR() method.
I think this info about Init can be omitted now since it's static.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 8
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 23:32:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#6).
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority
are not in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 184 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/6
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 6
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#9).
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority/TokenSigner
are not in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 199 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/9
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 9
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/7/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/7/src/kudu/master/master_service.cc@484
PS7, Line 484: KLOG_EVERY_N_SECS(WARNING, 60) << "no username for authn token from "
should we reject this at the RPC authn layer? Dan what do you think?
or at least return an actual error back to the client here instead of letting them connect
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 7
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 02:50:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#4).
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectoToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority/TokenSigner are not
in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 237 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/4
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 4
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> Right, we don't need to issue a token to a client if authentication is disa
I think we should treat token generation as a guaranteed success -- if we can't generate one, we are broken and we should probably FATAL so a different leader who is not broken can take over.
Am I missing some case where we might fail to generate one?
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 02:17:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 2: Verified+1
Unrelated flaek in BlockManagerType/TsRecoveryITest.TestRestartWithPendingCommitFromFailedOp/1
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 18 Jan 2018 20:17:42 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#7).
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority/TokenSigner
are not in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 191 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/7
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 7
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> Isn't it necessary for it to do so in the case that encryption is disabled?
Right, we don't need to issue a token to a client if authentication is disabled (I think the encryption part is already handled by the 'rpc->is_confidential()' part). But our code does this anyway up to the current version anyway. We can update that behavior accordingly.
I think my question here is: what should be the behavior of the master in this method if authentication is enabled (i.e. --rpc_authentication={optional,required}) and there was an error while generating the authn token? Should client still receive such a response as from leader master, or not?
Probably, that's a theoretical question right now, I'd like to know what we do expect from our masters in that case.
If you don't like the idea of updating the role of the master in response due to that failure, would it be OK just to add CHECK() or LOG(FATAL) instead of KLOG_EVERY_N_SECS(WARNING, 1) at line 494?
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:45:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#3).
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectoToMaster
Do not send half-baked responses to clients from master that declares
itself a leader when its catalog manager or CA authority or TokenSigner
are not yet in proper state.
Also, added a dedicated integration test. In addition, already existing
AuthTokenIssuingTest.ChannelConfidentiality test provides some coverage
for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 225 insertions(+), 66 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/3
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 3
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...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9052/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9052/3//COMMIT_MSG@7
PS3, Line 7: KUDU-1927: no half-baked responses on ConnectoToMaster
> I'm a bit confused on this -- the JIRA linked here is marked as "wont fix"
Original intention was to fix not exactly KUDU-1927, but some other (at least by understanding) issue. However, during the review, Adar raised a question about KUDU-1927, so I decided to address that as well.
I spotted that issue while working on AuthTokenIssuingTest.ChannelConfidentiality. The issue I was trying to fix was: the master has its role as leader according to the consensus information, but CatalogManager::ScopedLeaderSharedLock::leader_status().ok() returning 'false' as at https://github.com/apache/kudu/blob/master/src/kudu/master/master_service.cc#L473
By my understanding, KUDU-1927 was more about CatalogManager::ScopedLeaderSharedLock::leader_status().ok() returning 'true', but the CA cert hasn't been generated yet or the token signer doesn't have valid signing key. That's why I didn't re-open the JIRA issue.
I'll do it now, though.
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc
File src/kudu/integration-tests/master_cert_authority-itest.cc:
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc@416
PS3, Line 416: // Add master-only flags.
: const vector<string> master_flags = {
: Substitute("--catalog_manager_inject_latency_load_ca_info_ms=$0", latency_ms_),
: };
: copy(master_flags.begin(), master_flags.end(),
: back_inserter(cluster_opts_.extra_master_flags));
> isn't this simpler to just do cluster_opts_.extra_master_flags.push_back(..
Yes, I think it. It seems the current code just came from ConnectToClusterBaseTest's constructor.
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc@438
PS3, Line 438: const int hb_interval_ms = 50;
> does this always eventually converge to elect even in tsan?
It does not, and that's the reason behind flakiness in Jenkins pre-commit builds.
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@52
PS3, Line 52: parallel other
> "parallel with other"
Ah, it seems I started some modifications on the class, and then I rolled them back and updated the comment. But I should have moved it back. Done.
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@82
PS3, Line 82: Status SignServerCSR(const std::string& csr_der, const rpc::RemoteUser& caller,
> warning: function 'kudu::master::MasterCertAuthority::SignServerCSR' has a
Done
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@94
PS3, Line 94: // Access the CA certificate object.
> doesn't really add much
That's fair: I removed the comment.
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@97
PS3, Line 97: // Whether the object is initialized.
> worth noting here something like "requires external synchronization against
I thought it would be enough to have that NOTE in the class comment.
However, I think it's nice to have a comment as you suggested: done.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 3
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Jan 2018 01:58:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority/TokenSigner
are not in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Reviewed-on: http://gerrit.cloudera.org:8080/9052
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@cloudera.com>
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 199 insertions(+), 65 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 10
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when its catalog
: // manager is not in proper state yet.
Are you sure that's not by design? Todd added ConnectToMaster and I think the intent was for it to always "succeed" and supply enough information to explain the state the master was in.
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 'role' is LEADER but
This comment seems relevant.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
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: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 18 Jan 2018 21:24:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9052
to look at the new patch set (#8).
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
KUDU-1927: no half-baked responses on ConnectToMaster
Do not send half-baked responses to clients from a master that declares
itself a leader if its catalog manager/CA authority/TokenSigner
are not in proper state.
While working on AuthTokenIssuingTest.ChannelConfidentiality test,
I noticed that implementation of MasterServiceImpl::ConnectToMaster()
allowed for getting a success response without proper authn/security
information in case if the master hasn't been established as a leader
yet. By examining the code further I found that could happen not
only at the very first startup of a master, but also during master
re-elections in case of multi-master setup.
If a legit client connects to master but it does not get CA cert and
authn token, it might be a situation when it works flawlessly with
masters and tablet servers using its Kerberos credentials, but the
exported authentication credentials contain neither CA cert nor authn
token. The latter is very surprising in cases when the credentials
are later imported by other Kudu client applications that do not have
Kerberos credentials in their environment. If so, the client is not
able to connect to a secured Kudu cluster at all.
Added a dedicated integration test to verify that the new implementation
does not allow for such a behavior. In addition, updated the already
existing AuthTokenIssuingTest.ChannelConfidentiality test so now it
provides some coverage for the new code as well.
Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/client/client.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
7 files changed, 200 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/8
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 8
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@477
PS5, Line 477: if (cert_authority->is_initialized()) {
> It seems to be true, and it seems the original version has never aborted be
well, it adds complexity to the MasterCertAuthority class, right?
Also it seems generally useful to say that the response to this method indicates whether the master is a _ready_ master, not just a master according to Consensus. The cert authority issue here is one particular result of a non-ready master reporting itself as ready, but in the future seems plausible there would be others, no?
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
In KUDU-1927 Adar wrote:
> If !leader_status.ok() but Role() is LEADER, it's indeed a sign that the consensus state machine has elected this master as leader, but the leader is still becoming active and setting up in-memory state. In that case, we should enforce that resp->role() is not set to LEADER (i.e. just overwrite it with UNKNOWN_ROLE, FOLLOWER, or whatever).
which is what I'm suggesting here.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:01:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 9
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Jan 2018 19:31:19 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> There is one funny case: when the username is empty. Others seems to be ca
yea, in that case I think we should send an error back to the client anyway that its request was invalid.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 02:48:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@477
PS5, Line 477: if (cert_authority->is_initialized()) {
> well, it adds complexity to the MasterCertAuthority class, right?
Yep, that adds additional boolean variable and is_initialized() method: basically, an explicit way to detect whether it's safe to call other methods of MasterCertAuthority without risking abort(). However, if we don't need that in any of the use cases, we can keep the original version.
Yes, there might be more sub-components there with their own statuses. If we want to keep that under the blanket of l.leader_status(), all right then.
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> In KUDU-1927 Adar wrote:
All right, but do we want to make sure we were able to issue an authn token as the response, if needed?
In other words, is it be OK for a master to report itself as leader in the response if it failed to issue an authn token for the connecting client?
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:26:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9052/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9052/3//COMMIT_MSG@7
PS3, Line 7: KUDU-1927: no half-baked responses on ConnectoToMaster
I'm a bit confused on this -- the JIRA linked here is marked as "wont fix" back in June, but here you're patching it. Can you explain in the commit message why it is we need this patch? And perhaps we need to re-open the JIRA explaining why our previous "wont fix" logic was not right?
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc
File src/kudu/integration-tests/master_cert_authority-itest.cc:
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc@416
PS3, Line 416: // Add master-only flags.
: const vector<string> master_flags = {
: Substitute("--catalog_manager_inject_latency_load_ca_info_ms=$0", latency_ms_),
: };
: copy(master_flags.begin(), master_flags.end(),
: back_inserter(cluster_opts_.extra_master_flags));
isn't this simpler to just do cluster_opts_.extra_master_flags.push_back(...) instead of making and copying a vector?
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc@438
PS3, Line 438: const int hb_interval_ms = 50;
does this always eventually converge to elect even in tsan?
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@52
PS3, Line 52: parallel other
"parallel with other"
Why did you move this away from the "Init()" method itself? It seemed to make more sense there instead of at the class-level.
I think it's also better to keep the comment scoped to this own class's behavior rather than talking about the overall design of what might use the class. In other words, it's enough to just say "NOTE: Init() is not thread-safe must be called before any other methods."
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@94
PS3, Line 94: // Access the CA certificate object.
doesn't really add much
http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@97
PS3, Line 97: // Whether the object is initialized.
worth noting here something like "requires external synchronization against calls of Init()" or something
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 3
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Jan 2018 00:16:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 9
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jan 2018 02:06:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@477
PS5, Line 477: if (cert_authority->is_initialized()) {
> isn't this guaranteed? leader_status() only is OK in the case that this nod
It seems to be true, and it seems the original version has never aborted because of that even if similar CHECK() was in MasterCertAuthority::ca_cert_der() method.
But it seemed a little bit brittle to rely on that. What's wrong with explicit check of MasterCertAuthority::is_initialized()? With that, it's not required to remember whether initialized CA authority is guaranteed if l.leader_status().ok().
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> can we just simplify this to the approach outlined in the JIRA? ie:
The crucial point here was also to make sure the authn token is issued, if needed. Don't we need that assurance?
What JIRA is that, BTW? I could not find that piece in KUDU-1927 (nor in KUDU-1924).
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 00:55:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> I think we should treat token generation as a guaranteed success -- if we c
There is one funny case: when the username is empty. Others seems to be candidate for LOG(FATAL), indeed.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 02:29:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when its catalog
: // manager is not in proper state yet.
> So it seems like we thought we had a world with the two states { FOLLOWER,
I think that makes sense, and some time ago there was a patch which did something similar:
https://gerrit.cloudera.org/#/c/6356/
I think I'll do like you suggested. I think that's a good idea especially given the fact that the clients are supposed to handle such cases. Thanks!
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 'role' is LEADER but
> Hmm, but the message is consistent with the bug you're addressing ("a short
Sorry about being fuzzy. Agreed, that's confusing. Maybe, I should handle KUDU-1924 in this commit as well. Basically, I think we can apply your idea here as well: send the role of the server as FOLLOWER in the response if this current leader doesn't have proper CA cert or TSK yet.
BTW, the correct code will make the process crash if leader_status.ok() but the CA cert hasn't been set yet. That's because of CHECK() in the MasterCertAuthority::ca_cert_der() method.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
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...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 23:18:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectoToMaster
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/5/src/kudu/master/master_service.cc@507
PS5, Line 507: (!has_cert || (!has_authn_token && needs_authn_token))) {
> All right, but do we want to make sure we were able to issue an authn token
Isn't it necessary for it to do so in the case that encryption is disabled? in that case the client still needs to know who is the leader, and it should still indicate it is the leader even if it didn't issue a token, right?
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 5
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:35:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 'role' is LEADER but
> Sorry about being fuzzy. Agreed, that's confusing. Maybe, I should handle
It should have been '... current code will make ...' instead of '... correct code will make ...'.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
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...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 23:22:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: WIP [master] no half-baked responses on ConnectoToMaster
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when its catalog
: // manager is not in proper state yet.
> I'm not sure this is by design. My point is that in case if a leader is no
So it seems like we thought we had a world with the two states { FOLLOWER, LEADER } but we actually have a world with the three states { FOLLOWER, FOLLOWER_TRANSITIONING_TO_LEADER, LEADER }. This patch identifies the new state, but what I don't like about it is that it returns an error while in that state when no error is returned while in the other two states.
Instead of doing this, what if we treated role == LEADER && !leader_status.ok() the same as role == FOLLOWER? That is, do resp->set_role(FOLLOWER), do resp->add_master_addrs() as before, and skip the KUDU-1924 stuff below? It's not "wrong" per se; it just means this master will be considered a FOLLOWER by the client during the (short) transition to LEADER. Clients should already handle the scenario when all masters are FOLLOWER because they're doing a leader election; this just prolongs that state slightly.
http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 'role' is LEADER but
> That's about some different case, actually -- when leader_status.ok() but n
Hmm, but the message is consistent with the bug you're addressing ("a short window wherein the master is LEADER but isn't done initializing").
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
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...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:32:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1927: no half-baked responses on ConnectToMaster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectToMaster
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9052/7/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/9052/7/src/kudu/master/master_service.cc@484
PS7, Line 484: KLOG_EVERY_N_SECS(WARNING, 60) << "no username for authn token from "
> should we reject this at the RPC authn layer? Dan what do you think?
I think we can do both, at least it's a good idea to send back an error response at this level anyway. That will make the semantics of this methods more consistent.
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 7
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...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 03:07:37 +0000
Gerrit-HasComments: Yes