Move security credentials, connectors, and auth context to C++ #17291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
soheilhy
requested review from
yashykt,
ncteisen,
vjpai,
markdroth and
jiangtaoli2016
soheilhy
added
the
release notes: no
|
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
8cbc2ba
to
bf314c8
Compare
|
|
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
bf314c8
to
996cb31
Compare
|
|
|
|
|
|
|
|
|
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
996cb31
to
4adacc3
Compare
markdroth
requested changes
Nov 26, 2018
There was a problem hiding this comment.
Thanks for doing this!
My main structural comment is that I think we can actually use RefCountedPtr by having a thin C wrapper around the C++ implementation for the public API. The C++ implementation would use RefCountedPtr while the C wrapper would not expose that fact.
This definitely needs review from @jiangtaoli2016 and/or @yihuazhang, since their team owns the security code.
Please let me know if you have any questions or concerns. Thanks!
Reviewed 46 of 46 files at r1.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @soheilhy, @jiangtaoli2016, @AspirinSJL, @yashykt, @ncteisen, @vjpai, and @apolcyn)
src/core/lib/security/context/security_context.h, line 64 at r1 (raw file):
// This type is forward declared as a C struct and we cannot define it as a // class. Otherwise, compiler will complain about type mismatch due to
I think a better way to address this would be to have the real implementation be a full-fledged C++ class called GprcAuthContext, and then have a thin C wrapper called grpc_auth_context. This would allow us to use RefCountedPtr.
Same thing for most of the classes in this PR.
src/core/lib/security/context/security_context.h, line 69 at r1 (raw file):
public: explicit grpc_auth_context(grpc_auth_context* chained = nullptr) : chained_(chained ? GRPC_AUTH_CONTEXT_REF(chained, "chained")
Nit: Prefer != nullptr to make it clear to the reader that it's a pointer and not a bool.
src/core/lib/security/context/security_context.h, line 127 at r1 (raw file):
inline grpc_auth_context* grpc_auth_context_ref(grpc_auth_context* policy) { return policy ? policy->Ref() : nullptr;
!= nullptr
src/core/lib/security/context/security_context.h, line 130 at r1 (raw file):
} inline void grpc_auth_context_unref(grpc_auth_context* policy) { if (policy) policy->Unref();
!= nullptr
src/core/lib/security/context/security_context.h, line 148 at r1 (raw file):
struct grpc_client_security_context { grpc_client_security_context(grpc_call_credentials* creds)
explicit
src/core/lib/security/context/security_context.h, line 149 at r1 (raw file):
struct grpc_client_security_context { grpc_client_security_context(grpc_call_credentials* creds) : creds(creds ? creds->Ref() : nullptr) {}
creds != nullptr
src/core/lib/security/context/security_context.h, line 153 at r1 (raw file):
void set_creds(grpc_call_credentials* creds) { if (this->creds) this->creds->Unref();
!= nullptr
src/core/lib/security/context/security_context.h, line 154 at r1 (raw file):
void set_creds(grpc_call_credentials* creds) { if (this->creds) this->creds->Unref(); this->creds = creds ? creds->Ref() : nullptr;
creds != nullptr
src/core/lib/security/context/security_context.cc, line 83 at r1 (raw file):
/* --- grpc_client_security_context --- */ grpc_client_security_context::~grpc_client_security_context() { if (creds) creds->Unref();
!= nullptr
I'll stop noting these in each individual place, but please fix throughout this PR.
src/core/lib/security/credentials/credentials.h, line 99 at r1 (raw file):
struct grpc_channel_credentials { public: grpc_channel_credentials(const char* type) : type_(type) {}
explicit
src/core/lib/security/credentials/credentials.h, line 174 at r1 (raw file):
struct grpc_call_credentials { public: grpc_call_credentials(const char* type) : type_(type) {}
explicit
src/core/lib/security/credentials/credentials.h, line 245 at r1 (raw file):
const char* type() const { return type_; } const grpc_auth_metadata_processor& processor() const { return processor_; }
Please call these methods auth_metadata_processor() and set_auth_metadata_processor().
src/core/lib/security/credentials/credentials.h, line 273 at r1 (raw file):
struct grpc_credentials_metadata_request { grpc_credentials_metadata_request(grpc_call_credentials* creds)
explicit
src/core/lib/security/credentials/alts/alts_credentials.h, line 30 at r1 (raw file):
/* Main struct for grpc ALTS channel credential. */ class grpc_alts_credentials final : public grpc_channel_credentials {
Please use C++ style for C++ classes (e.g., class names and method names shoulld be MethodName instead of method_name). Also, please move them into the grpc_core namespace.
Same thing throughout.
src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 345 at r1 (raw file):
// // static grpc_call_credentials_vtable compute_engine_vtable = {
I assume this can be removed.
src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 497 at r1 (raw file):
} // static grpc_call_credentials_vtable access_token_vtable = {
Please remove.
src/core/lib/security/credentials/plugin/plugin_credentials.h, line 30 at r1 (raw file):
struct grpc_plugin_credentials; struct grpc_plugin_credentials_pending_request {
Perhaps this should be nested inside of grpc_plugin_credentials?
src/core/lib/security/security_connector/security_connector.h, line 166 at r1 (raw file):
}; /// A helper function for use in grpc_security_connector_cmp() implementations.
I think this should be the cmp() method (overriding the one from the base class) instead of being a standalone function. Subclasses can override and call the method from the grpc_channel_security_connector class if needed.
src/core/lib/security/security_connector/security_connector.h, line 195 at r1 (raw file):
/// A helper function for use in grpc_security_connector_cmp() implementations. int grpc_server_security_connector_cmp(
Same comment as above.
src/core/lib/security/security_connector/security_connector.cc, line 73 at r1 (raw file):
const grpc_channel_security_connector* sc1, const grpc_channel_security_connector* sc2) { return sc1 == sc2;
I assume this was added for testing? I don't think we want to require identity in comparisons.
src/core/lib/security/security_connector/security_connector.cc, line 91 at r1 (raw file):
c = GPR_ICMP(sc1->request_metadata_creds, sc2->request_metadata_creds); if (c != 0) return c; c = GPR_ICMP((void*)sc1->check_call_host, (void*)sc2->check_call_host);
We need some way to make sure that both connector objects are the same type.
src/core/lib/security/security_connector/security_connector.cc, line 105 at r1 (raw file):
int c = GPR_ICMP(sc1->server_creds, sc2->server_creds); if (c != 0) return c; return GPR_ICMP((void*)sc1->add_handshakers, (void*)sc2->add_handshakers);
Same here.
markdroth
added this to Not yet started
in Core conversion to idiomatic C++
via automation
markdroth
moved this from Not yet started
to In progress
in Core conversion to idiomatic C++
|
per offline discussion with @soheilhy, we could have create_security_connector returns RefCountedPtr rather than grpc_security_status. A nullptr return value indicates creation failure. |
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
4adacc3
to
e7581db
Compare
soheilhy
commented
Dec 6, 2018
There was a problem hiding this comment.
Thank you for the review!
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @markdroth, @jiangtaoli2016, @yihuazhang, @AspirinSJL, @yashykt, @ncteisen, @vjpai, and @apolcyn)
src/core/lib/security/context/security_context.h, line 64 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I think a better way to address this would be to have the real implementation be a full-fledged C++ class called
GprcAuthContext, and then have a thin C wrapper calledgrpc_auth_context. This would allow us to useRefCountedPtr.Same thing for most of the classes in this PR.
Thank you for the review Mark. As discussed offline:
- We will keep the C naming style for easier review and smaller diff for this PR. I will rename the types in a follow up CL.
- For RefCountedPtr, we don't need a C wrapper. And I will use
reset()where needed. Also, we wouldn't be able to changegrpc_auth_contextsince it's leaked in the API.
I'm not sure if the forward decl of the typedef (i.e., struct) is part of the API. If it's not, we can potentially do something like:
typedef struct grpc_t grpc_t
typedef struct GrpcT grpc_t
For the same reason we cannot move these structs into a namespace.
src/core/lib/security/context/security_context.h, line 69 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Nit: Prefer
!= nullptrto make it clear to the reader that it's a pointer and not a bool.
sure. done.
src/core/lib/security/context/security_context.h, line 127 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
!= nullptr
this is removed now.
src/core/lib/security/context/security_context.h, line 130 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
!= nullptr
this is removed now.
src/core/lib/security/context/security_context.h, line 148 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
explicit
sure done.
src/core/lib/security/context/security_context.h, line 149 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
creds != nullptr
done.
src/core/lib/security/context/security_context.h, line 153 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
!= nullptr
this is removed now.
src/core/lib/security/context/security_context.h, line 154 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
creds != nullptr
this is removed now.
src/core/lib/security/context/security_context.cc, line 83 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
!= nullptrI'll stop noting these in each individual place, but please fix throughout this PR.
They are mostly removed now with RefCountedPtr. I tried to change the rest to use == nullptr. apologies if I have missed anything.
src/core/lib/security/credentials/credentials.h, line 99 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
explicit
sure. done.
src/core/lib/security/credentials/credentials.h, line 174 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
explicit
sure done.
src/core/lib/security/credentials/credentials.h, line 245 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please call these methods
auth_metadata_processor()andset_auth_metadata_processor().
sure changed them. but I feel they are too verbose.
src/core/lib/security/credentials/credentials.h, line 273 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
explicit
sure. done.
src/core/lib/security/credentials/alts/alts_credentials.h, line 30 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please use C++ style for C++ classes (e.g., class names and method names shoulld be
MethodNameinstead ofmethod_name). Also, please move them into thegrpc_corenamespace.Same thing throughout.
As we discussed offline, this patch is already XXL and changing the names would have made the review significantly harder. Will do those changes and naming fixes once this is merge.
src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 345 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I assume this can be removed.
thank you for catching the commented. removed.
src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 497 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please remove.
Thanks. removed.
src/core/lib/security/credentials/plugin/plugin_credentials.h, line 30 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Perhaps this should be nested inside of
grpc_plugin_credentials?
sure moved there and called it "pending_request"
src/core/lib/security/security_connector/security_connector.h, line 166 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I think this should be the
cmp()method (overriding the one from the base class) instead of being a standalone function. Subclasses can override and call the method from thegrpc_channel_security_connectorclass if needed.
Sure I moved it there. I kept it hear to make sure every class is implementing a cmp function. now it's not needed.
src/core/lib/security/security_connector/security_connector.h, line 195 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Same comment as above.
Sure I moved it there. I kept it hear to make sure every class is implementing a cmp function. now it's not needed.
src/core/lib/security/security_connector/security_connector.cc, line 73 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I assume this was added for testing? I don't think we want to require identity in comparisons.
thank you for catching this. I thought I have removed this one.
src/core/lib/security/security_connector/security_connector.cc, line 91 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
We need some way to make sure that both connector objects are the same type.
I discussed this with Jiangtao and adjusted the implementation. thanks.
src/core/lib/security/security_connector/security_connector.cc, line 105 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Same here.
thanks please see the comment above.
|
|
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
83ae50d
to
f4d7ace
Compare
|
|
|
|
|
All the issues are gone now, yay! The failure is known flake #17399 |
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
f4d7ace
to
1b42610
Compare
|
|
|
|
This is to use `grpc_core::RefCount` to improve performnace. This commit also replaces explicit C vtables, with C++ vtable with its own compile time assertions and performance benefits. It also makes use of `RefCountedPtr` wherever possible.
soheilhy
force-pushed
the
worktree-security-cpp
branch
from
1b42610
to
9decf48
Compare
|
|
|
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to use
grpc_core::RefCountto improve performnace.This commit also replaces explicit C vtables, with C++ vtable
with its own compile time assertions and performance benefits.
Note that these classes cannot be made
RefCountedPtrbecausethe type and its related functions are all leaked as naked pointers to
the C API.
This change is