Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(java/driver/flight-sql): add basic auth #1487

Merged
merged 16 commits into from
Feb 2, 2024
Merged

feat(java/driver/flight-sql): add basic auth #1487

merged 16 commits into from
Feb 2, 2024

Conversation

jduo
Copy link
Member

@jduo jduo commented Jan 23, 2024 •
edited
Loading

  • Removed unused FlightClient from FlightSqlDatabase
  • FlightClient creation moved from FlightSqlDatabase to FlightSqlConnection.
  • Use separate allocators for each FlightClient.
  • Add HTTP basic auth (username/password).
  • Add support for arbitrary headers.
  • Add support for TLS and mTLS
  • Add the arrow-testing library as a submodule for TLS certificates.
  • Update CI for arrow-testing library.

@jduo jduo requested a review from lidavidm as a code owner January 23, 2024 22:34
@github-actions github-actions bot added this to the ADBC Libraries 0.10.0 milestone Jan 23, 2024
@jduo
Copy link
Member Author

jduo commented Jan 23, 2024

Looking to extend this PR to add token authentication, cookie middleware, and TLS. This would bring the driver close to the Flight JDBC Driver in terms of connectivity functionality.

@jduo jduo marked this pull request as draft January 24, 2024 23:00
@jduo
Copy link
Member Author

jduo commented Jan 24, 2024

Tests to be added.

@jduo jduo force-pushed the java-flight-sql-auth branch 2 times, most recently from 0e146eb to 8959431 Compare January 26, 2024 19:20
@jduo
Copy link
Member Author

jduo commented Jan 26, 2024

I've been working on the tests for this. I'm planning to make some changes to the infrastructure:

  • Import the arrow-testing project to get the same TLS certificates as the ones used in the Arrow repo.
  • Change the project to import the Arrow BOM instead of each dependency.
  • Add tests which validate sent headers.
  • Add tests which start a FlightSqlServer with TLS configured with certs in various ways.

@jduo jduo force-pushed the java-flight-sql-auth branch 2 times, most recently from c840157 to c6a3b79 Compare January 31, 2024 19:23
tokoko and others added 13 commits February 1, 2024 05:34
@tokoko @jduo
feat(java/driver/flight-sql): add basic auth
4fec557
@jduo
Rebase on current
46712e0
@jduo
Bring connection options to parity with Go
c492248 
- Propagate bearer tokens to spawned connections.
- Allow for cookies and propagate to spawned connections.
- Implement support for user-specified headers.
- Implement TLS options.
@jduo
linter issues
62a2dd2
@jduo
Use the Arrow BOM instead of individual dependencies
87dfa12
@jduo
Add arrow-testing as a submodule named testing
6a07035
@jduo
Add TLS and MTLS tests
7d5c673 
- Use FlightServerTestRule to build end-to-end tests for TLS and MTLS.
- Add conversion of FlightRunTimeExceptions to AdbcExceptions
@jduo
Fix RAT
5c0a0e3
@jduo
Update CI to fetch submodules
05b02fc
@jduo
Remove recursive from errorprone job and add to java job
a9758c4
@jduo
Add recursive to errorprone job
f348053
@jduo
Attempt to fix CI again
96c9632 
Fix POMs
Correct use of submodules recursive in errorprone job
@jduo
Add tests and bugfixes
676be98 
Add tests for arbitrary headers, cookies, auth.
Fix bugs retaining the bearer token.
@jduo jduo marked this pull request as ready for review February 1, 2024 13:41
@jduo jduo requested a review from zeroshade as a code owner February 1, 2024 13:41
lidavidm
lidavidm approved these changes Feb 1, 2024
View reviewed changes
...sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlClientWithCallOptions.java
import org.apache.arrow.flight.sql.util.TableRef;
import org.apache.arrow.util.AutoCloseables;

/** A wrapper around FlightSqlClient which automatically adds CallOptions to each RPC call. */
Copy link
Member

Choose a reason for hiding this comment

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

~nit: would it make more sense to wrap the FlightClient and give that to the FlightSqlClient? Ah, I guess we can't subclass...

...-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlConnectionProperties.java Outdated

/** Defines connection options that are used by the FlightSql driver. */
public interface FlightSqlConnectionProperties {
//
Copy link
Member

Choose a reason for hiding this comment

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

nit: did you mean to leave this?

lidavidm
lidavidm approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm lidavidm merged commit 5e21134 into apache:main Feb 2, 2024
9 checks passed
@jduo jduo mentioned this pull request Feb 6, 2024
Closed
@tokoko
Copy link
Contributor

tokoko commented Feb 6, 2024

@jduo I also planned on adding Dremio test suite following this one, the work is here main...tokoko:arrow-adbc:java-flight-sql-dremio. I'll try to rebase it when i find some time and see if it's still worth merging.

@jduo jduo mentioned this pull request Feb 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidavidm lidavidm lidavidm approved these changes

@zeroshade zeroshade Awaiting requested review from zeroshade zeroshade is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
ADBC Libraries 0.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jduo @tokoko @lidavidm