jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501459974
##########
File path: python/manylinux1/scripts/build_grpc.sh
##########
@@ -16,8 +16,61 @@
# specific language governing permissions and limitations
# under the License.
-export GRPC_VERSION="1.29.1"
-export CFLAGS="-fPIC -DGPR_MANYLINUX1=1"
+export GRPC_VERSION="1.32.0"
Review comment:
Reverted this actually, since we only need gRPC 1.27 for this
functionality to work.
##########
File path: python/manylinux1/scripts/build_re2.sh
##########
@@ -31,5 +31,7 @@ make prefix=/usr/local -j${NCORES} install
popd
-# Need to remove shared library to make sure the static library is picked up
by Arrow
-rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.so*
+# Need to remove static library to make sure the shared library is picked up
by Arrow
+# The static library fails to link when using gRPC 1.32.
+rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.a
Review comment:
Reverted since we only need gRPC 1.27.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
A few lines lower there's a line which is just
disable_server_verification=disable_server_verification which exceeds the line
limit. Do you know if there's a way around this?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any
server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck =
std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Done.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any
server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck =
std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
Review comment:
Done
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme ==
kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any
server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck =
std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
+ std::shared_ptr<ge::TlsKeyMaterialsConfig> materials_config(
+ new ge::TlsKeyMaterialsConfig());
Review comment:
Done.
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -90,6 +90,8 @@ class ARROW_FLIGHT_EXPORT FlightWriteSizeStatusDetail :
public arrow::StatusDeta
class ARROW_FLIGHT_EXPORT FlightClientOptions {
public:
+ FlightClientOptions();
Review comment:
The default constructor was already there prior to this patch, it was
just being implicitly defined instead of explicitly. I agree in principle that
the Defaults() method should be used, however the constructor has already been
public and I'm not sure it's worth breaking existing application code.
We're not really consistent about this internally either.
The C++ unit tests make use of both the public constructor and Defaults()
method. The Python wrapper uses the public constructor.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -835,6 +843,31 @@ class GrpcMetadataReader : public FlightMetadataReader {
std::shared_ptr<std::mutex> read_mutex_;
};
+namespace {
+// Dummy self-signed certificate to be used because TlsCredentials
+// requires root CA certs, even if you are skipping server
+// verification.
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+const char BLANK_ROOT_PEM[] =
Review comment:
Made a constexpr
----------------------------------------------------------------
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:
[email protected]
