You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/12/07 11:30:13 UTC
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8789
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/server_base.cc
M src/kudu/util/flags.h
15 files changed, 442 insertions(+), 267 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/1
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/8/src/kudu/rpc/sasl_common.h
File src/kudu/rpc/sasl_common.h:
http://gerrit.cloudera.org:8080/#/c/8789/8/src/kudu/rpc/sasl_common.h@66
PS8, Line 66: Status SaslInit(bool kerberos_keytab_provided = false) WARN_UNUSED_RESULT;
> warning: function 'kudu::rpc::SaslInit' has a definition with different par
Done
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 13 Dec 2017 16:23:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7:
(3 comments)
overall lgtm, just a couple of tiny nits.
Also, it seems the tests fail.
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h
File src/kudu/security/security_flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@32
PS7, Line 32: class
nit: maybe, switch to struct since this does not have anything private/protected?
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@34
PS7, Line 34: const char*
nit: const char* const ?
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@35
PS7, Line 35: const char*
ditto
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 09 Dec 2017 00:55:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc@272
PS7, Line 272: DCHECK_EQ(kerberos_enabled, is_kerberos_enabled);
Nearly all the external mini cluster kerberos tests hit this DCHECK. I'm not exactly sure how this code works, but it looks like the Messenger created here doesn't turn on Kerberos even if 'enable_kerberos' is true for the tests:
https://github.com/apache/kudu/blob/master/src/kudu/mini-cluster/external_mini_cluster.cc#L154-L158
It wouldn't be turned on even before this patch because FLAGS_keytab_file for the test binary is an empty string.
Does this look like an existing bug? Removing this DCHECK makes all the tests pass.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 09 Dec 2017 06:49:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Reviewed-on: http://gerrit.cloudera.org:8080/8789
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.cc
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
25 files changed, 580 insertions(+), 334 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@168
PS3, Line 168: private:
> Probably best to set these defaults in the constructor to keep things consi
Done
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@176
PS3, Line 176: int64_t rpc_negotiation_timeout_ms_;
> Is there any reason why FLAGS_rpc_tls_ciphers and FLAGS_rpc_tls_min_protoco
Good catch, looks like I forgot to add them. I added them as options in the latest patch set.
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@74
PS3, Line 74:
> nit: no need to comment this (it's obvious from context).
Done
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@124
PS3, Line 124:
> Seems more consistent to copy here too like other functions unless there is
Done
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h
File src/kudu/rpc/negotiation.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h@33
PS3, Line 33:
> This doesn't appear to be used.
I think I added it by mistake. It's not needed.
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@564
PS3, Line 564: void StartTestServer(Sockaddr *server_addr,
> Wrap this like you did with 'CreateMessenger'
Done
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@605
PS3, Line 605: template<class ServiceClass>
> ditto
Done
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc@269
PS3, Line 269:
> I think it may be cleaner to pass in the kerberos_enabled flag into the Onc
Ah, thanks, I was trying to do that initially but couldn't because of GoogleOnceInit only allowing functions with no args. I changed it to std::call_once.
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc@78
PS4, Line 78: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:"
: "ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:"
: "ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:"
: "ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:"
: "ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:"
: "AES256-GCM-SHA384:AES128-GCM-SHA256:"
: "AES256-SHA256:AES128-SHA256:"
: "AES256-SHA:AES128-SHA"
I'm not thrilled about duplicating the defaults here and in MessengerBuilder, but if someone prefers a different way of doing this, I'm open to suggestions.
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc@52
PS3, Line 52:
> this comment isn't necessary, we have a tool that checks headers.
Done
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/util/flags.h@85
PS3, Line 85: Status ParseTriState(const char* flag_name, const std::string& flag_value,
: TriStateFlag* tri_state);
:
: } // namespace kudu
: #endif /* KUDU_UTIL_FLAGS_H */
:
:
:
:
:
:
:
:
:
:
> Is there any particular reason making this function template? Why not to d
Good point. I'm not sure why it was a template. I had just moved this function from messenger.cc. In any case, it doesn't need to be a template, I made it non-templatized and moved it to flags.cc now.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 10:10:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#5).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
18 files changed, 563 insertions(+), 321 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/5
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 18 Dec 2017 20:58:49 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc@78
PS4, Line 78: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:"
: "ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:"
: "ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:"
: "ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:"
: "ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:"
: "AES256-GCM-SHA384:AES128-GCM-SHA256:"
: "AES256-SHA256:AES128-SHA256:"
: "AES256-SHA:AES128-SHA"
> I'm not thrilled about duplicating the defaults here and in MessengerBuilde
Can think of two possible solutions:
- Get rid of the no-arg constructor, effectively making the cipher/tls version a required arg.
- Make this string a static const in security, and refer to it as the default here, as well as in server_base. It's also been referenced now in our squeasel config setup, so maybe it could DRY up that as well (webserver_options.cc).
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 18:54:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/messenger.cc@85
PS6, Line 85: keytab_file_(""),
This can be omitted since it's the default, and it's not a primitive type.
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/sasl_common.cc@271
PS6, Line 271: std::call_once(once, DoSaslInit, kerberos_enabled);
Could you add a line after this call once:
DCHECK_EQ(kerberos_enabled, is_kerberos_enabled);
I just want to make sure we don't accidently start calling this with different values for kerberos_enabled.
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h
File src/kudu/security/security_flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h@38
PS6, Line 38: static const std::string kDefaultTlsCiphers =
I think this needs to be a const char*, and the value definition moved into the .cc. I can't remember the exact rules, but I recall having a static std::string is a no-no.
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h@48
PS6, Line 48: static const std::string kDefaultTlsMinVersion = "TLSv1";
ditto
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/server/server_base.cc@168
PS6, Line 168: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:"
Can these flags use the constant?
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 22:19:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/messenger.cc@85
PS6, Line 85: enable_inbound_tls_(false) {
> This can be omitted since it's the default, and it's not a primitive type.
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/sasl_common.cc@271
PS6, Line 271: std::call_once(once, DoSaslInit, kerberos_enabled);
> Could you add a line after this call once:
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h
File src/kudu/security/security_flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h@38
PS6, Line 38: } // namespace security
> I think this needs to be a const char*, and the value definition moved into
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h@48
PS6, Line 48:
> ditto
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/server/server_base.cc@168
PS6, Line 168: DEFINE_string(rpc_tls_min_protocol,
> Can these flags use the constant?
Yup it can, I forgot to make the change earlier. Done.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 23:06:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#6).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
19 files changed, 561 insertions(+), 330 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/6
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#8).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/CMakeLists.txt
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.cc
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
23 files changed, 580 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/8
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 3:
The last failure looks like a flaky test in:
MultiThreadedHybridClockTabletTest/2.UpdateNoMergeCompaction
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 15:53:30 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#7).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.cc
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
21 files changed, 581 insertions(+), 330 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/7
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7: -Code-Review
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 00:53:23 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@124
PS3, Line 124: std::string sasl_proto_name
Seems more consistent to copy here too like other functions unless there is a reason not to.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 08:42:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc@272
PS7, Line 272: DCHECK_EQ(kerberos_enabled, is_kerberos_enabled);
> I looked into this, turns out it's caused by SaslInit() calls from client_n
Thanks. I made both the above changes and the failing tests seem to pass now.
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h
File src/kudu/security/security_flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@32
PS7, Line 32: class
> nit: maybe, switch to struct since this does not have anything private/prot
Done
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@34
PS7, Line 34: const char*
> nit: const char* const ?
Done
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@35
PS7, Line 35: const char*
> ditto
Done
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 13 Dec 2017 15:40:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#10).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.cc
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
25 files changed, 580 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/10
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#9).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/CMakeLists.txt
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.cc
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
23 files changed, 580 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/9
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/util/flags.h@85
PS3, Line 85: template <typename T>
: inline Status ParseTriState(const char* flag_name, const std::string& flag_value, T* tri_state) {
: if (boost::iequals(flag_value, "required")) {
: *tri_state = T::REQUIRED;
: } else if (boost::iequals(flag_value, "optional")) {
: *tri_state = T::OPTIONAL;
: } else if (boost::iequals(flag_value, "disabled")) {
: *tri_state = T::DISABLED;
: } else {
: return Status::InvalidArgument(strings::Substitute(
: "$0 flag must be one of 'required', 'optional', or 'disabled'",
: flag_name));
: }
: return Status::OK();
: }
Is there any particular reason making this function template? Why not to declare it as non-template and move the implementation into the .cc file?
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 17:34:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc@78
PS4, Line 78: ert_(false) {
: security::InitializeOpenSSL();
: }
:
: TlsContext::TlsContext(std::string tls_ciphers, std::string tls_min_protocol)
: : tls_ciphers_(std::move(tls_ciphers)),
: tls_min_protocol_(std::move(tls_min_protocol)),
: lock_(RWMutex::Priority::PREFER_READ
> Can think of two possible solutions:
Thanks for the suggestions. I went with the second option by creating the defaults as static consts in security/security_flags.h
I also used these default members it in webserver_options.cc
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 21:54:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#4).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_tls_ciphers
FLAGS_rpc_tls_min_protocol
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
18 files changed, 565 insertions(+), 320 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/4
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 2:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@434
PS1, Line 434: const std::string& rpc_certificate_file = "",
> warning: the parameter 'rpc_certificate_file' is copied for each invocation
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@435
PS1, Line 435: const std::string& rpc_private_key_file = "",
> warning: the parameter 'rpc_private_key_file' is copied for each invocation
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@436
PS1, Line 436: const std::string& rpc_ca_certificate_file = "",
> warning: the parameter 'rpc_ca_certificate_file' is copied for each invocat
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@437
PS1, Line 437: const std::string& rpc_private_key_password_cmd = "") {
> warning: the parameter 'rpc_private_key_password_cmd' is copied for each in
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@569
PS1, Line 569: DoStartTestServer<GenericCalculatorService>(server_addr, enable_ssl, rpc_certificate_file,
> warning: parameter 'rpc_certificate_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@570
PS1, Line 570: rpc_private_key_file, rpc_ca_certificate_file, rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@570
PS1, Line 570: rpc_private_key_file, rpc_ca_certificate_file, rpc_private_key_password_cmd);
> warning: parameter 'rpc_ca_certificate_file' is passed by value and only co
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@570
PS1, Line 570: rpc_private_key_file, rpc_ca_certificate_file, rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_password_cmd' is passed by value and on
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@613
PS1, Line 613: CreateMessenger("TestServer", n_server_reactor_threads_, enable_ssl, rpc_certificate_file,
> warning: parameter 'rpc_certificate_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@614
PS1, Line 614: rpc_private_key_file, rpc_ca_certificate_file, rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@614
PS1, Line 614: rpc_private_key_file, rpc_ca_certificate_file, rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_password_cmd' is passed by value and on
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@614
PS1, Line 614: rpc_private_key_file, rpc_ca_certificate_file, rpc_private_key_password_cmd);
> warning: parameter 'rpc_ca_certificate_file' is passed by value and only co
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/server/server_base.cc@228
PS1, Line 228: } // namespace
> warning: anonymous namespace not terminated with a closing comment [google-
Done
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 14:34:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@176
PS3, Line 176: std::string keytab_file_ = "";
Is there any reason why FLAGS_rpc_tls_ciphers and FLAGS_rpc_tls_min_protocol and possibly other flags used in TlsContext::Init() are not converted to options configurable in messenger builder ?
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 08:53:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#3).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/server_base.cc
M src/kudu/util/flags.h
15 files changed, 443 insertions(+), 279 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/3
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc@272
PS7, Line 272: DCHECK_EQ(kerberos_enabled, is_kerberos_enabled);
> Nearly all the external mini cluster kerberos tests hit this DCHECK. I'm no
I looked into this, turns out it's caused by SaslInit() calls from client_negotiation.cc and server_negotiation.cc which didn't pass kerberos_enabled (and thus inherited the default false). Since those calls necessarily come after MessengerBuilder::Build, they can safely be removed. This patch got all the tests passing: https://github.com/danburkert/kudu/commit/959054e6f9a1a00341c6cf82644003ee6beeb75a .
As part of that, I realized that 'kerberos_enabled' isn't a good name for this flag - what we're really interested in is whether the process has a keytab (and thus a background thread renewing credentials from the keytab). For Impala this is equivalent to whether kerberos is enabled, but for Kudu it's frequently different. So, I think it'd be best to rename 'kerberos_enabled' and similar variables to 'has_kerberos_keytab' or something like that (very open to suggestions).
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 00:53:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 7: Code-Review+2
LGTM. Will leave open in case Alexey wants to do another pass.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 23:13:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@168
PS3, Line 168: int64_t rpc_negotiation_timeout_ms_ = 3000;
Probably best to set these defaults in the constructor to keep things consistent.
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@74
PS3, Line 74: // Default of 65 seconds.
nit: no need to comment this (it's obvious from context).
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h
File src/kudu/rpc/negotiation.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h@33
PS3, Line 33: enum class TriStateFlag;
This doesn't appear to be used.
Edit: or is it necessary because RpcAuthentication and RpcEncryption are typedefs?
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@564
PS3, Line 564: void StartTestServer(Sockaddr *server_addr, bool enable_ssl = false,
Wrap this like you did with 'CreateMessenger'
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@605
PS3, Line 605: void DoStartTestServer(Sockaddr *server_addr, bool enable_ssl = false,
ditto
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc@269
PS3, Line 269: is_kerberos_enabled.Store(kerberos_enabled);
I think it may be cleaner to pass in the kerberos_enabled flag into the Once, and set it there. That will make it threadsafe as just a normal (non-atomic) static. Unfortunately it doesn't look like the Google flavor of once supports flags to the init function, but the standard library does, so you'd have to switch it to use that.
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc@52
PS3, Line 52: // for RpcAuthentication, RpcEncryption
this comment isn't necessary, we have a tool that checks headers.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 22:02:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8789
to look at the new patch set (#2).
Change subject: KUDU-2228: Make Messenger options configurable
......................................................................
KUDU-2228: Make Messenger options configurable
Currently, the RPC layer accesses many gflags directly to take
certain decisions, eg. whether to turn on encryption,
authentication, etc.
Since the RPC layer is to be used more like a library, these should
be configurable options that are passed to the Messenger
(which is the API endpoint for the application using the RPC layer),
instead of the RPC layer itself directly accessing these flags.
This patch converts the following flags to Messenger options and moves
the flag definitions to server_base.cc which is the "application" in
Kudu that uses the Messenger:
FLAGS_rpc_default_keepalive_time_ms
FLAGS_rpc_negotiation_timeout_ms
FLAGS_rpc_authentication
FLAGS_rpc_encryption
FLAGS_rpc_certificate_file
FLAGS_rpc_private_key_file
FLAGS_rpc_ca_certificate_file
FLAGS_rpc_private_key_password_cmd
FLAGS_keytab_file
Most of the remaining flags are test or benchmark related flags. There
may be a few more flags that can be moved out and converted to options,
but we can leave that as future work if we decide to move them.
Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
A src/kudu/security/security_flags.h
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/server_base.cc
M src/kudu/util/flags.h
15 files changed, 443 insertions(+), 267 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/2
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot