docs: correct kernel proxy (SOCKS) and session-conf allowlist caveats - #446
Open
eric-wang-1990 wants to merge 3 commits into
Open
docs: correct kernel proxy (SOCKS) and session-conf allowlist caveats#446eric-wang-1990 wants to merge 3 commits into
eric-wang-1990 wants to merge 3 commits into
Conversation
CONNECTION_PARAMETERS.md overstated two behaviors on the kernel path: - Session params: the blanket "passed through on both backends" claim is wrong for kernel, which matches conf keys against an allowlist (dropping non-allowlisted keys, hard-rejecting a few). Note the Thrift-vs-kernel difference and mark WithSessionParams inert (⚠️ ) on kernel. (PECOBLR-4153) - Proxy: the kernel path accepts http(s) proxies only; a socks* URL honored on Thrift is rejected at connect on kernel. (PECOBLR-4152) Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refines the connection-parameter reference doc to more accurately describe behavioral differences between the Thrift and SEA/kernel backends, specifically around session-conf passthrough and proxy handling.
Changes:
- Clarifies that Thrift forwards session confs freely, while kernel applies an allowlist (dropping or rejecting some keys).
- Updates the
WithSessionParamsrow to reflect the kernel path’s allowlist-gated behavior. - Adds a kernel-specific proxy caveat noting that SOCKS proxy URLs are rejected on kernel.
Suppressed comments (1)
CONNECTION_PARAMETERS.md:72
- This row still says "Arbitrary session confs", but the kernel behavior described above is not arbitrary (it is allowlist-gated, with some keys dropped/rejected). Tweaking the wording here would avoid implying full/unchecked support on kernel.
| *(session param)* | `WithSessionParams` | ✅ | ⚠️ | | Arbitrary session confs (e.g. `ansi_mode`, `STATEMENT_TIMEOUT`, `QUERY_TAGS`). Allowlisted confs are honored on both; on kernel a non-allowlisted conf is dropped/rejected (see the note above; PECOBLR-4153). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
27
to
+32
| Any parameter not listed below (e.g. `ansi_mode`) is passed through as a | ||
| **session parameter** on both backends. | ||
| **session parameter**. On the **Thrift** path the session-conf map is forwarded freely. | ||
| On the **kernel** path conf keys are matched (case-insensitively) against an allowlist — | ||
| non-allowlisted keys are dropped with a warning, and a few are hard-rejected — so a conf | ||
| that takes effect on Thrift may silently be ignored on kernel. Broadening the kernel | ||
| allowlist is tracked in PECOBLR-4153. |
Contributor
There was a problem hiding this comment.
Broadened the WithSessionParams) row in one edit. No Go source touched, so no rebuild/tests required.
Pushed 2a283ba.
The driver folds spark.sql.thriftserver.metadata.metricview.enabled into SessionConf on both paths (kernel_config.go), but the kernel hard-rejects that conf (HTTP 400 INVALID_CONF_VALUE), so it does not take effect on the kernel path today. Flip the Kernel cell ✅ ->⚠️ with a caveat. Fix tracked in PECOBLR-4142 / PECOBLR-4153. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
eric-wang-1990
added
the
engineer-bot
Addresses: - #3800438771 at CONNECTION_PARAMETERS.md:32 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #445. Three behaviors were overstated for the kernel path in
CONNECTION_PARAMETERS.md; this corrects them.WithSessionParamsKernel cell from ✅ tosocks*proxy URL that the Thrift path honors is rejected at connect on the kernel path. Added that caveat to the Proxy section. Tracked in PECOBLR-4152.spark.sql.thriftserver.metadata.metricview.enabled=trueintoSessionConfon both paths (kernel_config.go), but the kernel currently hard-rejects that conf (HTTP 400INVALID_CONF_VALUE), so it does not take effect on the kernel path today. Flipped the Kernel cell ✅ →All three surfaced while cross-referencing the doc against the kernel-parity Jira items under PECOBLR-3727.
Note
Opened as a new PR because #445 was already merged and its branch deleted, so a follow-up commit to that PR wasn't possible.
This pull request and its description were written by Isaac.