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.

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

go/adbc/driver/flightsql: When CookiesMiddleware is enabled, DO_GET requests have a different set of cookies #1194

Closed
Tracked by #1490
aiguofer opened this issue Oct 12, 2023 · 6 comments · Fixed by #1497

Comments

@aiguofer
Copy link
Contributor

I noticed that when cookies are enabled, DO_GET requests seem to be on their own set of cookies while other methods (only tested with GET_FLIGHT_INFO and DO_ACTION) seem to be on another set of cookies.

For example, here's a sequence of logs from establishing a connection and issuing a query, setting a "session_id" in the cookies if not already provided in the cookies:

Through JDBC:

{
  "@timestamp" : "2023-10-11 22:53:05.446",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-0",
  "level" : "DEBUG",
  "request_id" : "b988e6c9-eb14-4e16-95a5-0ce66cd5acdb",
  "session" : "777a01f4-e6d9-4229-aa96-66d95517a241",
  "method" : "HANDSHAKE"
}
{
  "@timestamp" : "2023-10-11 22:53:08.772",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-1",
  "level" : "DEBUG",
  "request_id" : "f83cc5b4-053d-42ca-becf-3628eb365f8c",
  "session" : "777a01f4-e6d9-4229-aa96-66d95517a241",
  "method" : "DO_ACTION"
}
{
  "@timestamp" : "2023-10-11 22:53:10.453",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-1",
  "level" : "DEBUG",
  "request_id" : "f7f19a9c-086c-4327-94d0-467826f150d7",
  "session" : "777a01f4-e6d9-4229-aa96-66d95517a241",
  "method" : "GET_FLIGHT_INFO"
}
{
  "@timestamp" : "2023-10-11 22:53:10.496",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-1",
  "level" : "DEBUG",
  "request_id" : "1857a163-152a-4f3c-ac74-9be0cdd1ca3d",
  "session" : "777a01f4-e6d9-4229-aa96-66d95517a241",
  "method" : "DO_GET"
}
{
  "@timestamp" : "2023-10-11 22:53:10.855",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-1",
  "level" : "DEBUG",
  "request_id" : "cdb1928d-7a86-4cb4-8096-1dc8e129f646",
  "session" : "777a01f4-e6d9-4229-aa96-66d95517a241",
  "method" : "DO_ACTION"
}

Through ADBC:

{
  "@timestamp" : "2023-10-11 22:55:03.414",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-2",
  "level" : "DEBUG",
  "request_id" : "314a6665-f042-414d-a84c-48e2841d9502",
  "session" : "b47e6ed5-f833-4f53-b877-dd324e8fb82b",
  "method" : "GET_FLIGHT_INFO"
}
{
  "@timestamp" : "2023-10-11 22:55:04.020",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-2",
  "level" : "DEBUG",
  "request_id" : "b97a0373-90b1-4c38-adb4-e0738f567cc6",
  "session" : "246bd356-4bfc-4702-934f-369307814de0",
  "method" : "DO_GET"
}
{
  "@timestamp" : "2023-10-11 22:55:04.681",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-2",
  "level" : "DEBUG",
  "request_id" : "656729b2-a531-41f6-91e8-4014fe3f4a98",
  "session" : "b47e6ed5-f833-4f53-b877-dd324e8fb82b",
  "method" : "DO_ACTION"
}
{
  "@timestamp" : "2023-10-11 22:55:05.748",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-3",
  "level" : "DEBUG",
  "request_id" : "7b1ce66f-df50-4cac-8bad-ae9ab0e9c160",
  "session" : "b47e6ed5-f833-4f53-b877-dd324e8fb82b",
  "method" : "GET_FLIGHT_INFO"
}
{
  "@timestamp" : "2023-10-11 22:55:05.754",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-3",
  "level" : "DEBUG",
  "request_id" : "ecda2a81-df07-421a-bac4-2e343141b012",
  "session" : "246bd356-4bfc-4702-934f-369307814de0",
  "method" : "DO_GET"
}
{
  "@timestamp" : "2023-10-11 22:55:06.384",
  "message" : "Call started",
  "thread_name" : "flight-server-default-executor-3",
  "level" : "DEBUG",
  "request_id" : "5e64cf91-67bd-44df-a387-b8bd7423405d",
  "session" : "b47e6ed5-f833-4f53-b877-dd324e8fb82b",
  "method" : "DO_ACTION"
}
@lidavidm
Copy link
Member

The JDBC driver is known to be wrong in the sense that it always reuses the same client for metadata and data requests, by assuming that the same server is used for both. The ADBC implementation properly creates a new client. However then it will reauthenticate and everything. It does cache the clients, though.

Perhaps what we should do is:

  • Make sure that this cache is effective
  • Optimistically copy over cookies (and auth tokens?) from the existing connection for new connections

@aiguofer
Copy link
Contributor Author

Ahhh right, I remember that bug on the JDBC side. Yeah that makes sense! it does seem like the cache is working since the session_id matches between getStreamPreparedStatement and getStreamSqlInfo.

I think copying over cookies would suffice. The auth token and headers are already getting passed along correctly based on both ConnectionOptions and StatementOptions.

@aiguofer
Copy link
Contributor Author

aiguofer commented Nov 3, 2023

Relevant conversation in the JDBC driver: apache/arrow#38576. Maybe this should also be configurable for ADBC drivers.

@lidavidm lidavidm added this to the ADBC Libraries 0.9.0 milestone Nov 4, 2023
@lidavidm
Copy link
Member

lidavidm commented Nov 4, 2023

Yes, IMO the ideal behavior is to copy over the cookies/auth tokens to start with, but let them diverge from there if the server ends up setting something else or requesting authentication

@zeroshade
Copy link
Member

@aiguofer I can put this on my list of things to do, but if you want to take a stab at implementing this, go for it and I'll happily review the code! I'm currently travelling at a conference this week, but I'll try to get to this next week.

@aiguofer
Copy link
Contributor Author

aiguofer commented Nov 6, 2023

Awesome sounds great! I'd love to take a stab at it but I have no experience working in Go and unfortunately don't have the time to learn it right now.

@lidavidm lidavidm removed this from the ADBC Libraries 0.9.0 milestone Dec 19, 2023
zeroshade added a commit to apache/arrow that referenced this issue Jan 29, 2024
…39838)

### Rationale for this change
This is needed for apache/arrow-adbc#1194 to facilitate better connection handling for flight clients in ADBC by copying the existing cookies over when creating a sub-client.

### What changes are included in this PR?
Creating a `Clone` method on the `CookieMiddleware` so that a user can create and hold a reference to a specific cookie middleware instance and then create new ones on the fly that copy over the existing cookies at that moment.

### Are these changes tested?
Yes.

### Are there any user-facing changes?
No

* Closes: #39837

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
lidavidm pushed a commit that referenced this issue Jan 30, 2024
Closes #1194 

This also bumps our dependency up to arrow v16
@lidavidm lidavidm added this to the ADBC Libraries 0.10.0 milestone Jan 30, 2024
soumyadsanyal pushed a commit to soumyadsanyal/arrow-adbc that referenced this issue Jan 31, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…ware (apache#39838)

### Rationale for this change
This is needed for apache/arrow-adbc#1194 to facilitate better connection handling for flight clients in ADBC by copying the existing cookies over when creating a sub-client.

### What changes are included in this PR?
Creating a `Clone` method on the `CookieMiddleware` so that a user can create and hold a reference to a specific cookie middleware instance and then create new ones on the fly that copy over the existing cookies at that moment.

### Are these changes tested?
Yes.

### Are there any user-facing changes?
No

* Closes: apache#39837

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
…ware (apache#39838)

### Rationale for this change
This is needed for apache/arrow-adbc#1194 to facilitate better connection handling for flight clients in ADBC by copying the existing cookies over when creating a sub-client.

### What changes are included in this PR?
Creating a `Clone` method on the `CookieMiddleware` so that a user can create and hold a reference to a specific cookie middleware instance and then create new ones on the fly that copy over the existing cookies at that moment.

### Are these changes tested?
Yes.

### Are there any user-facing changes?
No

* Closes: apache#39837

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
…ware (apache#39838)

### Rationale for this change
This is needed for apache/arrow-adbc#1194 to facilitate better connection handling for flight clients in ADBC by copying the existing cookies over when creating a sub-client.

### What changes are included in this PR?
Creating a `Clone` method on the `CookieMiddleware` so that a user can create and hold a reference to a specific cookie middleware instance and then create new ones on the fly that copy over the existing cookies at that moment.

### Are these changes tested?
Yes.

### Are there any user-facing changes?
No

* Closes: apache#39837

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
kou pushed a commit to apache/arrow-go that referenced this issue Aug 30, 2024
…#39838)

### Rationale for this change
This is needed for apache/arrow-adbc#1194 to facilitate better connection handling for flight clients in ADBC by copying the existing cookies over when creating a sub-client.

### What changes are included in this PR?
Creating a `Clone` method on the `CookieMiddleware` so that a user can create and hold a reference to a specific cookie middleware instance and then create new ones on the fly that copy over the existing cookies at that moment.

### Are these changes tested?
Yes.

### Are there any user-facing changes?
No

* Closes: #39837

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants