Skip to content

ARROW-5877: [FlightRPC] Fix Python<->Java auth issues#4838

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

ARROW-5877: [FlightRPC] Fix Python<->Java auth issues#4838
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Jul 9, 2019

  • Java servers before didn't actually wait for the Handshake RPC to complete
  • Java servers didn't interrupt auth handlers if the client sent an error
  • Python/C++ clients didn't explicitly finish their end of the connection

Together, this led to the 'hanging forever' issue @rymurr saw.

I've left some TODOs as I would like to raise Flight-specific exceptions (which I'm working on in parallel).

Travis: https://travis-ci.com/lihalite/arrow/builds/118503572
AppVeyor: https://ci.appveyor.com/project/lihalite/arrow/builds/25858510

@codecov-io
Copy link

codecov-io commented Jul 9, 2019

Codecov Report

Merging #4838 into master will increase coverage by 6.45%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4838      +/-   ##
==========================================
+ Coverage   82.62%   89.08%   +6.45%     
==========================================
  Files         335      720     +385     
  Lines       43377   100165   +56788     
  Branches     1418        0    -1418     
==========================================
+ Hits        35841    89230   +53389     
- Misses       7174    10935    +3761     
+ Partials      362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/flight/client.cc 92.13% <66.66%> (ø)
go/arrow/ipc/writer.go
js/src/util/fn.ts
go/arrow/memory/memory_avx2_amd64.go
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
rust/datafusion/src/bin/main.rs
go/arrow/ipc/file_reader.go
rust/parquet/src/util/test_common/rand_gen.rs
js/src/builder/index.ts
... and 930 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7838886...fc35d19. Read the comment docs.

@rymurr
Copy link
Contributor

rymurr commented Jul 10, 2019

fwiw looks good to me! 👍

wesm
wesm approved these changes Jul 10, 2019
View reviewed changes
Copy link
Member

@wesm wesm left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. What's the path to having simulated auth on the integration test side?

@wesm wesm closed this in aa0631a Jul 10, 2019
@ghost
Copy link
Author

ghost commented Jul 10, 2019

My plan is to refactor the integration script, or create a new one, to run tests that aren't based around Arrow datasets, and then add cases to test auth, error propagation, cancellation, and other features thoroughly. As part of that, or as a prerequisite, I may implement a common HTTP basic auth method for C++/Python. I will track the work in ARROW-5875.

@rymurr
Copy link
Contributor

rymurr commented Jul 10, 2019

Shout if you want a hand with that @lihalite, I had planned on exposing the BasicAuth protobuf that Java already uses into c++/python but if you had a more complete solution in mind I would be happy to help.

@ghost
Copy link
Author

ghost commented Jul 10, 2019

Ah if you already were planning on that, please go ahead! I filed a JIRA for that specifically, ARROW-5876.

Not sure when I can get to the testing - I want to have it well before 1.0 but I'm currently busy with internal projects.

wesm pushed a commit that referenced this pull request Jul 13, 2019
@lidavidm @wesm
ARROW-5877: [FlightRPC] Fix Python<->Java auth issues
5f9e2ff 
- Java servers before didn't actually wait for the Handshake RPC to complete
- Java servers didn't interrupt auth handlers if the client sent an error
- Python/C++ clients didn't explicitly finish their end of the connection

Together, this led to the 'hanging forever' issue @rymurr saw.

I've left some TODOs as I would like to raise Flight-specific exceptions (which I'm working on in parallel).

Travis: https://travis-ci.com/lihalite/arrow/builds/118503572
AppVeyor: https://ci.appveyor.com/project/lihalite/arrow/builds/25858510

Author: David Li <[email protected]>

Closes #4838 from lihalite/arrow-5877 and squashes the following commits:

fc35d19 <David Li> Wait for authentication to complete server-side
@asfimport asfimport mentioned this pull request Aug 1, 2019
Closed
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
@lidavidm @kou
ARROW-5877: [FlightRPC] Fix Python<->Java auth issues
4a863c4 
- Java servers before didn't actually wait for the Handshake RPC to complete
- Java servers didn't interrupt auth handlers if the client sent an error
- Python/C++ clients didn't explicitly finish their end of the connection

Together, this led to the 'hanging forever' issue @rymurr saw.

I've left some TODOs as I would like to raise Flight-specific exceptions (which I'm working on in parallel).

Travis: https://travis-ci.com/lihalite/arrow/builds/118503572
AppVeyor: https://ci.appveyor.com/project/lihalite/arrow/builds/25858510

Author: David Li <[email protected]>

Closes apache#4838 from lihalite/arrow-5877 and squashes the following commits:

fc35d19 <David Li> Wait for authentication to complete server-side
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@codecov-io @rymurr @wesm @lidavidm