You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "srebhan (via GitHub)" <gi...@apache.org> on 2023/04/11 19:27:37 UTC
[GitHub] [arrow] srebhan opened a new pull request, #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
srebhan opened a new pull request, #35051:
URL: https://github.com/apache/arrow/pull/35051
This PR adds the ability to enable and customize TLS parameters for Golang's `database/sql` driver.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164345810
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -19,9 +19,53 @@ import (
"crypto/tls"
"fmt"
"net/url"
+ "sync"
"time"
+
+ "github.com/google/uuid"
+)
+
+// TLS configuration registry
+var (
+ tlsConfigRegistry = map[string]*tls.Config{
+ "skip-verify": {InsecureSkipVerify: true},
+ }
+ tlsRegistryMutex sync.Mutex
Review Comment:
I agree that it's not likely in a hot path and unlikely to be a cause for contention. It's probably alright for it to be a regular mutex. this is fine.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] srebhan commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "srebhan (via GitHub)" <gi...@apache.org>.
srebhan commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164344490
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -19,9 +19,53 @@ import (
"crypto/tls"
"fmt"
"net/url"
+ "sync"
"time"
+
+ "github.com/google/uuid"
+)
+
+// TLS configuration registry
+var (
+ tlsConfigRegistry = map[string]*tls.Config{
+ "skip-verify": {InsecureSkipVerify: true},
+ }
+ tlsRegistryMutex sync.Mutex
Review Comment:
I can certainly do this, but I don't expect this to be in a hot-path. Remember, the mutex is locked if you register a config _and_ when you are calling `Open` (which in turn does the DSN parsing). Do you want me to use a `RWMute`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164492804
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -30,14 +74,15 @@ type DriverConfig struct {
Timeout time.Duration
Params map[string]string
- TLSEnabled bool
- TLSConfig *tls.Config
+ TLSEnabled bool
+ TLSConfigName string
+ TLSConfig *tls.Config
}
func NewDriverConfigFromDSN(dsn string) (*DriverConfig, error) {
u, err := url.Parse(dsn)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("invalid URL: %w", err)
}
// Sanity checks on the given connection string
Review Comment:
Created issue #35080
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] srebhan commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "srebhan (via GitHub)" <gi...@apache.org>.
srebhan commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164355487
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -30,14 +74,15 @@ type DriverConfig struct {
Timeout time.Duration
Params map[string]string
- TLSEnabled bool
- TLSConfig *tls.Config
+ TLSEnabled bool
+ TLSConfigName string
+ TLSConfig *tls.Config
}
func NewDriverConfigFromDSN(dsn string) (*DriverConfig, error) {
u, err := url.Parse(dsn)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("invalid URL: %w", err)
}
// Sanity checks on the given connection string
Review Comment:
I guess that's because I wanted to keep the same scheme other SQL drivers use (see https://github.com/xo/dburl#example-urls). I think we could also support the ones above even though I don't like the redundant `grpc`...
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade merged pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #35051:
URL: https://github.com/apache/arrow/pull/35051
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] srebhan commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "srebhan (via GitHub)" <gi...@apache.org>.
srebhan commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164448787
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -30,14 +74,15 @@ type DriverConfig struct {
Timeout time.Duration
Params map[string]string
- TLSEnabled bool
- TLSConfig *tls.Config
+ TLSEnabled bool
+ TLSConfigName string
+ TLSConfig *tls.Config
}
func NewDriverConfigFromDSN(dsn string) (*DriverConfig, error) {
u, err := url.Parse(dsn)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("invalid URL: %w", err)
}
// Sanity checks on the given connection string
Review Comment:
I think this is unrelated to this PR. Are you OK with opening an issue for this request and proceed with this PR?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] disq commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "disq (via GitHub)" <gi...@apache.org>.
disq commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1163331553
##########
go/arrow/flight/flightsql/driver/README.md:
##########
@@ -35,7 +35,7 @@ connection pooling, transactions combined with ease of use (see (#usage)).
## Prerequisits
Review Comment:
```suggestion
## Prerequisites
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164337668
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -19,9 +19,53 @@ import (
"crypto/tls"
"fmt"
"net/url"
+ "sync"
"time"
+
+ "github.com/google/uuid"
+)
+
+// TLS configuration registry
+var (
+ tlsConfigRegistry = map[string]*tls.Config{
+ "skip-verify": {InsecureSkipVerify: true},
+ }
+ tlsRegistryMutex sync.Mutex
Review Comment:
would it make sense to use an `RWMutex` so the `GetTLSConfig` function can use `RLock` and `RUnlock` ?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35051:
URL: https://github.com/apache/arrow/pull/35051#issuecomment-1509578633
Benchmark runs are scheduled for baseline = 5184d7c175994a0193d0368019f11b7cd4febf18 and contender = 631c68a3068ab36487469d9aaebe4ab9bc4cbff7. 631c68a3068ab36487469d9aaebe4ab9bc4cbff7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/6a155f29bb1243c6a09a3d146560a95b...580054ddb12b4eda8d5725f720b13176/)
[Failed] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8765a802e8cb404a8e4a198d49d0cb7d...32e5b12d40484cf8a5e4feb169b47def/)
[Finished :arrow_down:1.28% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2c494def97ef446aa23338c05c5653d1...67bb82acaf6c4925809d1d7afca801c6/)
[Finished :arrow_down:0.58% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/d47792bdcd654cee9a5369974a6b5830...fd773df92379412fb1d8e56d3f55ef57/)
Buildkite builds:
[Finished] [`631c68a3` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2701)
[Failed] [`631c68a3` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2735)
[Finished] [`631c68a3` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2699)
[Finished] [`631c68a3` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2726)
[Finished] [`5184d7c1` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2700)
[Failed] [`5184d7c1` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2734)
[Finished] [`5184d7c1` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2698)
[Finished] [`5184d7c1` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2725)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164488132
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -30,14 +74,15 @@ type DriverConfig struct {
Timeout time.Duration
Params map[string]string
- TLSEnabled bool
- TLSConfig *tls.Config
+ TLSEnabled bool
+ TLSConfigName string
+ TLSConfig *tls.Config
}
func NewDriverConfigFromDSN(dsn string) (*DriverConfig, error) {
u, err := url.Parse(dsn)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("invalid URL: %w", err)
}
// Sanity checks on the given connection string
Review Comment:
sure thing
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164364544
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -30,14 +74,15 @@ type DriverConfig struct {
Timeout time.Duration
Params map[string]string
- TLSEnabled bool
- TLSConfig *tls.Config
+ TLSEnabled bool
+ TLSConfigName string
+ TLSConfig *tls.Config
}
func NewDriverConfigFromDSN(dsn string) (*DriverConfig, error) {
u, err := url.Parse(dsn)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("invalid URL: %w", err)
}
// Sanity checks on the given connection string
Review Comment:
I just want to maintain the consistency with the other existing implementations (pyarrow.flight, arrow c++ flight, etc.) so I think we should at minimum add the support for the ones above. there's nothing wrong with also supporting the `flightsql://` scheme too.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35051:
URL: https://github.com/apache/arrow/pull/35051#issuecomment-1509579525
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2c494def97ef446aa23338c05c5653d1...67bb82acaf6c4925809d1d7afca801c6/)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35051:
URL: https://github.com/apache/arrow/pull/35051#issuecomment-1503979147
* Closes: #35042
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35051:
URL: https://github.com/apache/arrow/pull/35051#issuecomment-1503979203
:warning: GitHub issue #35042 **has been automatically assigned in GitHub** to PR creator.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35051: GH-35042: [Go][FlightSQL driver] Add TLS configuration
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35051:
URL: https://github.com/apache/arrow/pull/35051#discussion_r1164343474
##########
go/arrow/flight/flightsql/driver/config.go:
##########
@@ -30,14 +74,15 @@ type DriverConfig struct {
Timeout time.Duration
Params map[string]string
- TLSEnabled bool
- TLSConfig *tls.Config
+ TLSEnabled bool
+ TLSConfigName string
+ TLSConfig *tls.Config
}
func NewDriverConfigFromDSN(dsn string) (*DriverConfig, error) {
u, err := url.Parse(dsn)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("invalid URL: %w", err)
}
// Sanity checks on the given connection string
Review Comment:
as I'm looking at this, is there any reason that this doesn't follow the general consistency of other implementations by using a scheme of `grpc+tcp` vs `grpc+unix` vs `grpc+tls`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org