Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18469 )
Change subject: rpc: plumb JWTs into the RPC layer ...................................................................... Patch Set 15: (8 comments) http://gerrit.cloudera.org:8080/#/c/18469/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18469/13//COMMIT_MSG@10 PS13, Line 10: It is limited in the sense that JWTs can be sent over encrypted : channels > ok Updated the commit message to reflect the actual behaviour. http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/client_negotiation.h@191 PS13, Line 191: * > style nit: stick the asterisk to the type, not to the parameter/variable na Done http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/messenger.cc@324 PS13, Line 324: jwt_verifier_(nullptr), > Why to set a non-functional verifier? Is it possible to keep 'jwt_verifier As jwt_verifier_ is declared as a shared_ptr it's not possible to assign std::nullopt to it. I think there are two possible choices here, either switch jwt_verifier_ to optional from shared_ptr, or leave it as is and assign nullptr. I went with the second option, but if you think we should switch to optional, that's possible too. Although it wouldn't necessarily be consistent accross the class members of Messenger. http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/negotiation.cc@178 PS13, Line 178: ClientNegotiation client_negotiati > nit: remove this variable and use messenger->jwt() in-place in the ClientNe Done http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/server_negotiation.cc@717 PS13, Line 717: Negoti > nit: indent Done http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/server_negotiation.cc@729 PS13, Line 729: std::mo > nit: could be std::move(subject) ? Done http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.h@97 PS13, Line 97: jwks_uri_(std::move(jwks_uri)), : is_local_file_(is_local_file) > nit: the indent is off Done http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.cc@929 PS13, Line 929: return jwt_->Init(jwks_uri_, is_local_file_); : } > Why not just Done -- To view, visit http://gerrit.cloudera.org:8080/18469 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I252f1e597d9df4408379c3b695f266dbd7f48dcc Gerrit-Change-Number: 18469 Gerrit-PatchSet: 15 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 06 Dec 2022 11:53:01 +0000 Gerrit-HasComments: Yes
