Repository navigation
feat: add https connection option to allow plain HTTP transport - #588
vuanhphung wants to merge 4 commits into
Conversation
DBSQLClient hardcoded `https: true`, so callers behind a trusted local reverse proxy had to monkey-patch the private getConnectionOptions(). `https: false` now selects plain HTTP (port 80 by default) on the Thrift transport, and the kernel backend receives an explicit http://host[:port] host. HTTPS stays the default; mTLS client identity is rejected over HTTP. Fixes #503 Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good overall, with one low-severity edge case. On the kernel path with https: false, setting a port both in host and in port produces a doubled port in the URL. The mapping is correct otherwise: Thrift defaults, the kernel http:// host and rejecting mTLS over plain HTTP all work, and mTLS is rejected on both backends because the kernel path also calls createConnectionProvider.
With `https: false`, the kernel host name appended `port` even when `host` already carried a `:port` suffix (the kernel's documented convention), producing an invalid `http://localhost:3015:3015`. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good, with one low-severity edge case in how the kernel builds the host URL for bare IPv6 hosts. The option mapping, the mTLS rejection, the docs and the tests (including a real plain-HTTP Thrift round trip) match the intended behaviour. The Thrift-only mTLS check is fine because the kernel enforces it itself.
GitHub Actions
Inactive
`/:\d+$/` matched literals like `::1`, so the kernel dropped `port`. Only `name:port` and `[ipv6]:port` now count as an embedded port, giving the same URL shape as the Thrift transport. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good: one low-severity concern about a trailing slash in the kernel http:// host. The https option maps cleanly onto HttpConnection's existing plain-HTTP support. The defaults stay HTTPS-only, mTLS is rejected up front, and both backends have unit and e2e coverage.
Thrift strips a trailing `/` from `host` before adding the port; the kernel mapping did not, producing `http://localhost/:3015`. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
DBSQLClient.getConnectionOptions()hardcodedhttps: true, so apps that reach Databricks through a trusted local reverse proxy (app --HTTP--> proxy --HTTPS--> Databricks) had to monkey-patch that private method, even thoughHttpConnectionalready supports plain HTTP.This adds a public opt-in,
https?: boolean, toConnectionOptions. It matches the internalIConnectionOptions.httpsit maps to. HTTPS stays the default, and only an explicitfalseturns it off. Withhttps: falsethe default port is 80, andclientCert/clientKeyare rejected instead of being silently dropped (the kernel rejects that combination too). The kernel binding keeps an explicithttp://host, so the kernel backend getshttp://host[:port]. That also keeps Thrift's Reyden fallback to the kernel on the same transport. On Thrift the scope is deliberately limited to the SQL connection: OAuth, token federation, telemetry and feature-flag requests still use HTTPS. CloudFetch presigned URLs are untouched.CONNECTION_PARAMETERS.mddocuments this and warns that plain HTTP is only for trusted local or private links.Verification: new unit tests cover the option mapping and run a real open/close round trip against a local plain-HTTP Thrift server. Without the fix, that round trip fails with a TLS
wrong version numbererror. A new e2e spec runsSELECT 1on a live warehouse overhttps: falsethrough a local reverse proxy on both backends. The existing proxy e2e test now uses the public option instead of overriding the private method. On the default path, a live 200k-row query on both backends confirmed Thrift still uses anhttps.Agenton port 443 and the kernel host is passed through unchanged.npm test(1537 passing), lint, type-check and build pass. Performance isn't affected: the change runs once perconnect()while building options, and no per-request or per-row code changed.Fixes #503
This pull request and its description were written by Isaac.