You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/28 16:28:45 UTC
[GitHub] [arrow-rs] avantgardnerio opened a new pull request, #2211: Update `FlightSqlService` trait to proxy handshake
avantgardnerio opened a new pull request, #2211:
URL: https://github.com/apache/arrow-rs/pull/2211
# Which issue does this PR close?
Closes #2210.
# Rationale for this change
Apache Arrow Ballista now supports FlightSQL, and can run simple statements like select 1;, however due to its architecture, tables need to be registered before any substantive queries can be run. Unfortunately, tables have to be registered on a context, or else the next query will have no idea that they were registered. The native Ballista protocol uses a [custom field](https://github.com/apache/arrow-ballista/blob/6bd0f6a58d0a7bdaf75af2c2485d56d5a812bea3/ballista/rust/core/proto/ballista.proto#L668) to implement this behavior, but for FlightSQL, we should use its [native method](https://arrow.apache.org/docs/format/Flight.html#authentication). However the existing FlightSqlService trait forces this [to return](https://github.com/apache/arrow-rs/blob/bc493d92c2a032a95d92dab81642f05182f20d81/arrow-flight/src/sql/server.rs#L261) an unimplemented Err.
# What changes are included in this PR?
Proxying of the `handshake()` method
# Are there any user-facing changes?
Other implementers of `FlightSqlService` will have to implement the method (they can just return `NotImplemented` as that was the previous behavior).
--
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-rs] alamb commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r933392469
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
I believe we are in total agreement
--
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-rs] avantgardnerio commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r933016516
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
When we have TPC-H up and running, I'll come back around to this and look at the main issues: cloning the server state & testing. Thanks for your feedback and helping get it merged!
--
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-rs] avantgardnerio commented on pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#issuecomment-1198435741
Tagging @alamb and @viirya just in case you don't see.
--
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-rs] avantgardnerio commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r933381161
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
Sorry, I didn't mean to imply integration tests were the only way. I think they become exponentially impossible to maintain as code coverage increases - so unit tests are also required. I was just (and still am) hoping to start with a FlighSql-rs test then add unit tests in addition to that.
--
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-rs] viirya commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932572658
##########
arrow-flight/src/sql/server.rs:
##########
@@ -47,6 +47,16 @@ pub trait FlightSqlService:
/// When impl FlightSqlService, you can always set FlightService to Self
type FlightService: FlightService;
+ /// Accept authentication and return a token
+ /// <https://arrow.apache.org/docs/format/Flight.html#authentication>
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ >;
Review Comment:
Yea, as you see, you can "override" default implementation in a trait nowadays.
--
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-rs] viirya commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932758912
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
Agreed that we eventually should add some test coverage for flight sql server. Currently we don't have any test coverage for it, not just 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-rs] avantgardnerio commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932536712
##########
arrow-flight/src/sql/server.rs:
##########
@@ -47,6 +47,16 @@ pub trait FlightSqlService:
/// When impl FlightSqlService, you can always set FlightService to Self
type FlightService: FlightService;
+ /// Accept authentication and return a token
+ /// <https://arrow.apache.org/docs/format/Flight.html#authentication>
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ >;
Review Comment:
I'm still learning Rust. Would this be done via `impl specialization`?
https://stackoverflow.com/questions/31461902/is-it-possible-to-extend-a-default-method-implementation-of-a-trait-in-a-struct
--
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-rs] alamb commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932576947
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
Looks good 👍
It would be awesome to get some test coverage of this code eventually 🤔
##########
arrow-flight/src/sql/server.rs:
##########
@@ -256,9 +270,10 @@ where
async fn handshake(
Review Comment:
Yeah, this isn't cool that the `impl` doesn't call the underlying implementation
--
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-rs] viirya merged pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
viirya merged PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211
--
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-rs] avantgardnerio commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932630211
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
I like to follow [acceptance test driven development](https://en.wikipedia.org/wiki/Acceptance_test-driven_development) when possible. Do you think the rust FlightSqlClient is far enough along to integration test with?
--
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-rs] ursabot commented on pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#issuecomment-1198732734
Benchmark runs are scheduled for baseline = 48cc6c3e9fea0027055990f9b398eb4b341d0697 and contender = acd8042b43806a601f0e1557792eb9eedf77d7ed. acd8042b43806a601f0e1557792eb9eedf77d7ed is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/4fbe7fcbbe5442db9915e15f1d65e726...4d07e5ec248d4cb9a07129d0ed2d786c/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/9279c662fcd946e796a6b9c7efa476fc...9b5340d93a6640b7bbe363f0b5e492f8/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f5fd3611325e4281950fc20f5d50a203...d736983781dd4f4889c1e82de4f2ef61/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/1a1ae0992c5e4e4dab6cbf540bbde01f...3e6510e567794c0ca2148298fdc7bede/)
Buildkite builds:
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-rs] avantgardnerio commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932543831
##########
arrow-flight/src/sql/server.rs:
##########
@@ -47,6 +47,16 @@ pub trait FlightSqlService:
/// When impl FlightSqlService, you can always set FlightService to Self
type FlightService: FlightService;
+ /// Accept authentication and return a token
+ /// <https://arrow.apache.org/docs/format/Flight.html#authentication>
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ >;
Review Comment:
Oh wow, I just threw the default implementation right on the trait and it compiled! I had no idea that would work nowadays :smile:
--
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-rs] viirya commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r932524332
##########
arrow-flight/src/sql/server.rs:
##########
@@ -47,6 +47,16 @@ pub trait FlightSqlService:
/// When impl FlightSqlService, you can always set FlightService to Self
type FlightService: FlightService;
+ /// Accept authentication and return a token
+ /// <https://arrow.apache.org/docs/format/Flight.html#authentication>
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ >;
Review Comment:
Should we provide default implementation that returns "Not yet implemented" error?
--
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-rs] alamb commented on a diff in pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#discussion_r933192051
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -41,6 +43,52 @@ pub struct FlightSqlServiceImpl {}
#[tonic::async_trait]
impl FlightSqlService for FlightSqlServiceImpl {
type FlightService = FlightSqlServiceImpl;
+
+ async fn do_handshake(
+ &self,
+ request: Request<Streaming<HandshakeRequest>>,
+ ) -> Result<
+ Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>,
+ Status,
+ > {
+ let basic = "Basic ";
+ let authorization = request
+ .metadata()
+ .get("authorization")
+ .ok_or(Status::invalid_argument("authorization field not present"))?
+ .to_str()
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ if !authorization.starts_with(basic) {
+ Err(Status::invalid_argument(format!(
+ "Auth type not implemented: {}",
+ authorization
+ )))?;
+ }
+ let base64 = &authorization[basic.len()..];
+ let bytes = base64::decode(base64)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let str = String::from_utf8(bytes)
+ .map_err(|_| Status::invalid_argument("authorization not parsable"))?;
+ let parts: Vec<_> = str.split(":").collect();
+ if parts.len() != 2 {
+ Err(Status::invalid_argument(format!(
+ "Invalid authorization header"
+ )))?;
+ }
+ let user = parts[0];
+ let pass = parts[1];
+ if user != "admin" || pass != "password" {
+ Err(Status::unauthenticated("Invalid credentials!"))?
+ }
+ let result = HandshakeResponse {
+ protocol_version: 0,
+ payload: "random_uuid_token".as_bytes().to_vec(),
+ };
+ let result = Ok(result);
+ let output = futures::stream::iter(vec![result]);
+ return Ok(Response::new(Box::pin(output)));
+ }
+
Review Comment:
> I like to follow [acceptance test driven development](https://en.wikipedia.org/wiki/Acceptance_test-driven_development) when possible. Do you think the rust FlightSqlClient is far enough along to integration test with?
I think there is also a place for targeted for regression tests (so that, for example, if someone breaks the code accidentally in some future refactoring, they also get a test failure). However, the right balance and where to draw the line between the two is always a matter of judgement
--
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-rs] codecov-commenter commented on pull request #2211: Update `FlightSqlService` trait to proxy handshake
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2211:
URL: https://github.com/apache/arrow-rs/pull/2211#issuecomment-1198503602
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2211](https://codecov.io/gh/apache/arrow-rs/pull/2211?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (32a6aed) into [master](https://codecov.io/gh/apache/arrow-rs/commit/bc493d92c2a032a95d92dab81642f05182f20d81?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bc493d9) will **decrease** coverage by `0.03%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #2211 +/- ##
==========================================
- Coverage 82.56% 82.53% -0.04%
==========================================
Files 239 239
Lines 62269 62299 +30
==========================================
+ Hits 51415 51418 +3
- Misses 10854 10881 +27
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/2211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow-flight/examples/flight\_sql\_server.rs](https://codecov.io/gh/apache/arrow-rs/pull/2211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctZmxpZ2h0L2V4YW1wbGVzL2ZsaWdodF9zcWxfc2VydmVyLnJz) | `0.00% <0.00%> (ø)` | |
| [arrow-flight/src/sql/server.rs](https://codecov.io/gh/apache/arrow-rs/pull/2211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctZmxpZ2h0L3NyYy9zcWwvc2VydmVyLnJz) | `0.00% <0.00%> (ø)` | |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/2211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `63.00% <0.00%> (+0.31%)` | :arrow_up: |
| [...rquet/src/arrow/record\_reader/definition\_levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/2211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci9kZWZpbml0aW9uX2xldmVscy5ycw==) | `89.02% <0.00%> (+0.42%)` | :arrow_up: |
| [...row/src/array/builder/string\_dictionary\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvc3RyaW5nX2RpY3Rpb25hcnlfYnVpbGRlci5ycw==) | `91.36% <0.00%> (+0.71%)` | :arrow_up: |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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