You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2021/01/15 03:36:13 UTC
[kudu-CR] allow customize sasl proto name instead of always using "kudu"
hongjizhang@ebay.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16955
Change subject: allow customize sasl_proto_name instead of always using "kudu"
......................................................................
allow customize sasl_proto_name instead of always using "kudu"
Change-Id: I79ccd0441f1d5eb087e2545e1ca8055078da5566
---
M src/kudu/rpc/messenger.cc
1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/16955/1
--
To view, visit http://gerrit.cloudera.org:8080/16955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I79ccd0441f1d5eb087e2545e1ca8055078da5566
Gerrit-Change-Number: 16955
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <ho...@ebay.com>
[kudu-CR] allow customize sasl proto name instead of always using "kudu"
Posted by "Hongjiang Zhang (Code Review)" <ge...@cloudera.org>.
Hongjiang Zhang has abandoned this change. ( http://gerrit.cloudera.org:8080/16955 )
Change subject: allow customize sasl_proto_name instead of always using "kudu"
......................................................................
Abandoned
This has been merged to other commit
--
To view, visit http://gerrit.cloudera.org:8080/16955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I79ccd0441f1d5eb087e2545e1ca8055078da5566
Gerrit-Change-Number: 16955
Gerrit-PatchSet: 2
Gerrit-Owner: Hongjiang Zhang <ho...@ebay.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hongjiang Zhang <ho...@ebay.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] allow customize sasl proto name instead of always using "kudu"
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16955 )
Change subject: allow customize sasl_proto_name instead of always using "kudu"
......................................................................
Patch Set 1:
(6 comments)
Thank you for the patch!
http://gerrit.cloudera.org:8080/#/c/16955/1//COMMIT_MSG
Commit Message:
PS1:
I think it would be great to have a test to make sure this works as expected and to catch further regressions in the code, if any.
You can check various tests under src/kudu/integration-tests (e.g. security-itest.cc, etc.) for the reference/example.
http://gerrit.cloudera.org:8080/#/c/16955/1//COMMIT_MSG@8
PS1, Line 8:
Please add a detailed description of this changelist: motivation, etc. How can people use this new functionality and what issue(s) it solves?
Is there corresponding JIRA already opened in this context? If yes, add information about that as well (you can see how it's done in other already submitted changelists in the repo).
This might be helpful as well: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines (that link can be found in https://kudu.apache.org/docs/contributing.html#_submitting_patches section).
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@58
PS1, Line 58: sasl_proto_name
I'm not sure this is the best name for the flag if thinking about end-users, not just readers of the code in this file.
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@59
PS1, Line 59: Default is kudu.
This is not needed: a help string for a gflag shows the default value automatically.
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@60
PS1, Line 60: sasl_proto_name
Can any string be supplied here and work just fine (an empty one, with spaces, etc.)? I think it's worth adding a validator for the flag to accept only strings which can be used by the SASL library at least.
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@81
PS1, Line 81: FLAGS_sasl_proto_name
Why not to use the same approach as for the rpc_authentication_ field, where the value for a particular Messenger instance is set via MessengerBuilder::set_sasl_proto_name() method in server_base.cc?
For the reference, see https://github.com/apache/kudu/blob/6f5f100c8443353c007ecf375b76ba940c687f98/src/kudu/server/server_base.cc#L512-L527
--
To view, visit http://gerrit.cloudera.org:8080/16955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ccd0441f1d5eb087e2545e1ca8055078da5566
Gerrit-Change-Number: 16955
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <ho...@ebay.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Jan 2021 05:04:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] allow customize sasl proto name instead of always using "kudu"
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
hongjizhang@ebay.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/16955 )
Change subject: allow customize sasl_proto_name instead of always using "kudu"
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@58
PS1, Line 58: sasl_proto_name
> I'm not sure this is the best name for the flag if thinking about end-users
According to the following docs:
https://www.cyrusimap.org/sasl/sasl/reference/manpages/library/sasl_server_new.html
https://opensource.apple.com/source/passwordserver_sasl/passwordserver_sasl-11/cyrus_sasl/doc/programming.html#sasl_server_new
This name is used to registered as a service, it suggests using protocol name, and is used by Kerberos,
So, what is about "sasl_service_name", "sasl_krb5_principal_name"?
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@59
PS1, Line 59: Default is kudu.
> This is not needed: a help string for a gflag shows the default value autom
Let me remove it
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@60
PS1, Line 60: sasl_proto_name
> Can any string be supplied here and work just fine (an empty one, with spac
Good suggestion. I'll add this.
http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@81
PS1, Line 81: FLAGS_sasl_proto_name
> Why not to use the same approach as for the rpc_authentication_ field, wher
If I set_sasl_proto_name() in server_base.cc, only kudu-master and kudu-tserver can use it. But when I run the kudu command line, for example, "kudu master list xxx", the code https://github.com/apache/kudu/blob/master/src/kudu/client/client.cc#L355 calls MessageBuilder without setting sasl_proto_name, as a result, "kudu" is still used.
So, it is better using a central place to set the proto name in MessengerBuilder.
--
To view, visit http://gerrit.cloudera.org:8080/16955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ccd0441f1d5eb087e2545e1ca8055078da5566
Gerrit-Change-Number: 16955
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <ho...@ebay.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ho...@ebay.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Jan 2021 06:22:13 +0000
Gerrit-HasComments: Yes