Todd Lipcon has posted comments on this change. Change subject: rpc: add basic service and method-level authorization ......................................................................
Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: PS1, Line 23: #include <boost/optional.hpp> : #include <ctype.h> : #include <glog/logging.h> : #include <google/protobuf/compiler/code_generator.h> : #include <google/protobuf/compiler/plugin.h> : #include <google/protobuf/descriptor.h> : #include <google/protobuf/descriptor.pb.h> : #include <google/protobuf/io/printer.h> : #include <google/protobuf/io/zero_copy_stream.h> : #include <google/protobuf/stubs/common.h> : #include <iostream> : #include <map> : #include <memory> : #include <sstream> : #include <string> > While you are at it, could you please re-organize those in accordance with Done http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 283: bool AuthorizeDisallowAlice(const google::protobuf::Message* req, > warning: parameter 'req' is unused [misc-unused-parameters] Done Line 284: google::protobuf::Message* resp, > warning: parameter 'resp' is unused [misc-unused-parameters] Done Line 293: bool AuthorizeDisallowBob(const google::protobuf::Message* req, > warning: parameter 'req' is unused [misc-unused-parameters] Done Line 294: google::protobuf::Message* resp, > warning: parameter 'resp' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: PS1, Line 262: // An option to set the authorization method for this particular : // RPC method. > Does it make sense to add a comment that if the authz method is not specifi Done http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: Line 117: // Fall out of the 'if' statement to the normal path. > Why are we checking the tracker result before checking authz? Seems like w hrm, I can see an argument either way. I think my thinking here is that, if for some reason the authorization changes (eg someone got revoked privileges), it would be incorrect to now return a "NotAuthorized" error when in fact the original call did succeed. I think we have to assume that request IDs are non-forgeable random UUIDs. I will make a note of that in the task list doc. PS1, Line 131: } > It's just a tiny stylistic nit, but do you think it would be better to prom Done http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.h File src/kudu/rpc/service_if.h: Line 99: bool AuthorizeAllowAll(const google::protobuf::Message* req, > warning: parameter 'req' is unused [misc-unused-parameters] Done Line 100: google::protobuf::Message* resp, > warning: parameter 'resp' is unused [misc-unused-parameters] Done Line 101: RpcContext* ctx) { > warning: parameter 'ctx' is unused [misc-unused-parameters] Done -- To view, visit http://gerrit.cloudera.org:8080/4897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
