lidavidm commented on a change in pull request #7130:
URL: https://github.com/apache/arrow/pull/7130#discussion_r422696732
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -531,14 +531,19 @@ class FlightClient::FlightClientImpl {
std::stringstream grpc_uri;
std::shared_ptr<grpc::ChannelCredentials> creds;
- if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls) {
+ if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls || scheme == kSchemeGrpcMTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
+ if ((scheme == "grpc+tls") || (scheme == "grpc+mtls") {
grpc::SslCredentialsOptions ssl_options;
if (!options.tls_root_certs.empty()) {
ssl_options.pem_root_certs = options.tls_root_certs;
}
+ if (scheme == 'grpc+mtls'){
Review comment:
Is a whole new scheme necessary? I'd say that so long as the client has
provided a list of client cert/key pairs, then mTLS should just be enabled, no?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -531,14 +531,19 @@ class FlightClient::FlightClientImpl {
std::stringstream grpc_uri;
std::shared_ptr<grpc::ChannelCredentials> creds;
- if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls) {
+ if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls || scheme == kSchemeGrpcMTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
+ if ((scheme == "grpc+tls") || (scheme == "grpc+mtls") {
grpc::SslCredentialsOptions ssl_options;
if (!options.tls_root_certs.empty()) {
ssl_options.pem_root_certs = options.tls_root_certs;
}
+ if (scheme == 'grpc+mtls'){
Review comment:
Also, this doesn't build and needs to be formatted - the developer guide
has instructions: https://arrow.apache.org/docs/developers/cpp/index.html
##########
File path: cpp/src/arrow/flight/server.cc
##########
@@ -665,16 +665,20 @@ Status FlightServerBase::Init(const FlightServerOptions&
options) {
const Location& location = options.location;
const std::string scheme = location.scheme();
- if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls) {
+ if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls || scheme == kSchemeGrpcMTls) {
std::stringstream address;
address << location.uri_->host() << ':' << location.uri_->port_text();
std::shared_ptr<grpc::ServerCredentials> creds;
- if (scheme == kSchemeGrpcTls) {
+ if ((scheme == kSchemeGrpcTls) || (scheme == kSchemeGrpcMTls)) {
grpc::SslServerCredentialsOptions ssl_options;
for (const auto& pair : options.tls_certificates) {
ssl_options.pem_key_cert_pairs.push_back({pair.pem_key,
pair.pem_cert});
}
+ if (scheme == kSchemeGrpcMTls) {
Review comment:
I'd say instead of using a new scheme, just add a flag to
`FlightServerOptions`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
