feat(csharp/src/Drivers/Databricks): Databricks Proxy Configurator #2789

toddmeng-db · 2025-05-06T20:24:52Z

First PR for Proxy support for Databricks Driver. Includes a SparkProxyConfigurator, HttpClientFactory that contains the Configurator, and necessary SparkParameters. Also includes some unit tests.

Tested integration with rest of driver in follow up PR.

Follow-up: Integrate with the rest of the driver. This will also include behavior for Oauth and CloudFetch

Other off-line discussions:

Kerberos proxy login not currently supported in JDBC OSS, so we're not currently supporting here.

eric-wang-1990 · 2025-05-07T00:16:19Z

csharp/src/Drivers/Apache/Spark/SparkParameters.cs

+        /// Whether to use a proxy for HTTP connections.
+        /// Default value is 0 (disabled) if not specified.
+        /// </summary>
+        public const string UseProxy = "UseProxy";


This should be a general option setting for all hive traffic. Conside something like adbc.http_options.proxy.use_proxy, added to here:

arrow-adbc/csharp/src/Drivers/Apache/Hive2/HiveServer2Parameters.cs

Line 55 in e045731

public const string DisableServerCertificateValidation = "adbc.http_options.tls.disable_server_certificate_validation";

eric-wang-1990 · 2025-05-07T00:32:49Z

csharp/src/Drivers/Apache/Spark/SparkHttpMessageHandlerFactory.cs

+    /// <summary>
+    /// Factory for creating HTTP clients with proper configuration
+    /// </summary>
+    public class SparkHttpMessageHandlerFactory


We should not need this, here is already returning a handler:

arrow-adbc/csharp/src/Drivers/Apache/Spark/SparkHttpConnection.cs

Line 151 in e045731

return HiveServer2TlsImpl.NewHttpClientHandler(TlsOptions);

.
We can just call the proxy configurator on top of it.

My main concern is that if we don't have a factory, we will have to configure proxy every where http client is used (cloud-fetch, oauth)

Yeah I think it's fine, there is only cloud fetch and oauth case.
Actually for oauth you probably can just pass the handler to the httpClient constructor:

arrow-adbc/csharp/src/Drivers/Databricks/Auth/OAuthClientCredentialsProvider.cs

Line 69 in ed2fe87

_httpClient = new HttpClient();

Try to see how bad it is, I think it's not that too bad.

Sounds good. I think it's feasible either way, but yea maybe client factory is a bit heavy weight right now.

I'm taking a look at the implementation; other concern is the NewHttpClientHandler method is static, and is called by different httpconnections (SparkHttpConnection, ImpalaHttpConnection, HiveServer2HttpConnection). Since these classes all inherit from HiveServer2Connection (and we probably don't want to expose http logic at this layer), there is no obvious place to initialize http client with proxy logic for all child http connections. I think it actually makes some sense to use factory here, then

Edit, Discussed off-line: We could still put the logic in NewHttpClientHandler method (or new method), since that class is performing many different tls configurations, anyways.
@CurtHagenlocher

move to spark layer unit tests fixes message handler factory

tls refactor delete old files

toddmeng-db · 2025-05-07T17:07:50Z

csharp/src/Drivers/Apache/Hive2/HiveServer2ProxyConfigurator.cs

+                handler.UseProxy = false;
+            }
+        }
+


http client bypass list in c# expects regex strings, hence why some handling is done to make hosts in regex format. I assume we don't want to expect users to pass in regex strings (though we still allow for wildcard pattern here)

add this as comments?

jadewang-db · 2025-05-13T20:40:52Z

csharp/src/Drivers/Apache/Hive2/HiveServer2ProxyConfigurator.cs

+                handler.UseProxy = false;
+            }
+        }
+


add this as comments?

csharp/src/Drivers/Apache/Hive2/HiveServer2Parameters.cs

CurtHagenlocher

Thanks! I seconded the feedback about the "regex" comment. It would also be good to use "true"/"false" for enabling/disabling a feature instead of "0"/"1".

CurtHagenlocher · 2025-05-14T14:40:45Z

csharp/src/Drivers/Apache/Hive2/HiveServer2ProxyConfigurator.cs

+    /// <summary>
+    /// Interface for proxy configuration
+    /// </summary>
+    public interface IProxyConfigurator


Is this intended to be part of the public surface area of the driver or is it "just" public for testing? If the former, does it perhaps make sense to move many proxy-related definitions into the core ADBC assembly where they could be used by multiple drivers? And if the latter, then please make them internal and use the InternalsVisibleTo attribute to expose them to tests.

We could certainly choose to make them internal now and do something different later, but public API surface should be deliberate.

I see - no, it's not meant to be externally used. I'll set it to internal, then.

However, we could still move the proxy parameters to ADBC-level. Would that make sense (even if there's no implementation for proxy by the other drivers)? Otherwise, let's just do that work later? @CurtHagenlocher

Later is fine. CC @davidhcoe because I know we're looking at adding proxy support to the BigQuery driver.

CurtHagenlocher · 2025-05-14T14:41:31Z

csharp/src/Drivers/Apache/Hive2/HiveServer2ProxyConfigurator.cs

+        public static HiveServer2ProxyConfigurator FromProperties(IReadOnlyDictionary<string, string> properties)
+        {
+            bool useProxy = properties.TryGetValue(HttpProxyOptions.UseProxy, out string? useProxyStr) &&
+                useProxyStr == "1";


ADBC convention is to use "true" and "false" for boolean-valued options.

CurtHagenlocher

Thanks!

toddmeng-db force-pushed the toddmeng-db/proxy branch 5 times, most recently from 7218116 to 41fa3e5 Compare May 6, 2025 23:00

toddmeng-db changed the title ~~databricks proxy configuration~~ feat(csharp/src/Drivers/Databricks) Databricks Proxy Configurator May 6, 2025

toddmeng-db force-pushed the toddmeng-db/proxy branch 2 times, most recently from 8bb4bc0 to d2e84db Compare May 7, 2025 00:02

eric-wang-1990 reviewed May 7, 2025

View reviewed changes

toddmeng-db force-pushed the toddmeng-db/proxy branch 2 times, most recently from e9b9272 to f5bb996 Compare May 7, 2025 06:37

toddmeng-db added 2 commits May 7, 2025 09:58

Proxy configuration

cf31fda

move to spark layer unit tests fixes message handler factory

refactor to hiveserver2

4466717

tls refactor delete old files

toddmeng-db force-pushed the toddmeng-db/proxy branch 4 times, most recently from 352fa0a to ce171fb Compare May 7, 2025 17:02

toddmeng-db commented May 7, 2025

View reviewed changes

toddmeng-db force-pushed the toddmeng-db/proxy branch 2 times, most recently from 70d916a to 72c54ca Compare May 7, 2025 17:38

revert http client factory

7b79fd5

toddmeng-db force-pushed the toddmeng-db/proxy branch from 72c54ca to 7b79fd5 Compare May 7, 2025 17:39

toddmeng-db changed the title ~~feat(csharp/src/Drivers/Databricks) Databricks Proxy Configurator~~ feat(csharp/src/Drivers/Databricks): Databricks Proxy Configurator May 7, 2025

toddmeng-db marked this pull request as ready for review May 7, 2025 17:52

toddmeng-db requested a review from CurtHagenlocher as a code owner May 7, 2025 17:52

github-actions bot added this to the ADBC Libraries 19 milestone May 7, 2025

toddmeng-db requested a review from eric-wang-1990 May 8, 2025 18:16

jadewang-db approved these changes May 13, 2025

View reviewed changes

toddmeng-db commented May 14, 2025

View reviewed changes

csharp/src/Drivers/Apache/Hive2/HiveServer2Parameters.cs Show resolved Hide resolved

fix proxy option names

26f7cc5

CurtHagenlocher requested changes May 14, 2025

View reviewed changes

toddmeng-db force-pushed the toddmeng-db/proxy branch from f5368dc to 1f6a45a Compare May 14, 2025 17:21

feedback fixes - internal access

a89b058

toddmeng-db force-pushed the toddmeng-db/proxy branch from 1f6a45a to a89b058 Compare May 14, 2025 18:04

toddmeng-db requested a review from CurtHagenlocher May 14, 2025 18:12

CurtHagenlocher approved these changes May 14, 2025

View reviewed changes

CurtHagenlocher merged commit 27c68c0 into apache:main May 14, 2025
7 checks passed

serramatutu mentioned this pull request Aug 12, 2025

Sync upstream dbt-labs/arrow-adbc#52

Closed

feat(csharp/src/Drivers/Databricks): Databricks Proxy Configurator #2789

feat(csharp/src/Drivers/Databricks): Databricks Proxy Configurator #2789

Uh oh!

Conversation

toddmeng-db commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddmeng-db May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CurtHagenlocher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddmeng-db May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CurtHagenlocher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

toddmeng-db commented May 6, 2025 •

edited

Loading

toddmeng-db May 7, 2025 •

edited

Loading

toddmeng-db May 14, 2025 •

edited

Loading