You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hiboyang (via GitHub)" <gi...@apache.org> on 2026/07/01 17:46:35 UTC
[PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
hiboyang opened a new pull request, #56933:
URL: https://github.com/apache/spark/pull/56933
### What changes were proposed in this pull request?
This PR adds a new `PathAwareChannelBuilder` to the Spark Connect Python client to support a URL path in the Spark Connect connection string, e.g. `sc://host1/path1:15002`.
The new builder accepts both the standard connection form (`sc://host[:port][/;params]`) and the path-routed form `sc://gateway/<prefix>:<port>`. When a path prefix is present, it is prepended to every gRPC method via a client interceptor, and TLS is enabled implicitly when the resolved port is 443 (unless `;use_ssl=` is set explicitly).
Related discussion: https://github.com/apache/spark/issues/56816
JIRA: https://issues.apache.org/jira/browse/SPARK-57793
### Why are the changes needed?
In Kubernetes, users commonly set up an Ingress to expose a Spark Connect driver endpoint behind a matching URL, e.g. `http://host1/path1` routed to the driver endpoint. The existing channel builder cannot carry a path prefix, so the Spark Connect client needs to be updated to support this path-based routing scenario.
### Does this PR introduce _any_ user-facing change?
No behavior change for existing connection strings — the existing host-based `sc://` connection strings continue to work unchanged. This PR adds a new opt-in `PathAwareChannelBuilder` that additionally supports a path inside the URL.
### How was this patch tested?
Added unit tests in `python/pyspark/sql/tests/connect/test_connect_channel.py` covering path parsing, port extraction from the final path segment, implicit TLS on port 443, and the path-prefix interceptor. Also manually tested in a local environment.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "dbtsai (via GitHub)" <gi...@apache.org>.
dbtsai commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4858919668
## Review feedback
Two issues in `PathAwareChannelBuilder._extract_attributes` (`python/pyspark/sql/connect/client/core.py`).
### 1. IPv6 endpoints are rejected (`core.py:667`)
`netloc = self.url.netloc.split(":")` with `len(netloc) in (1, 2)` breaks on IPv6 addresses:
- `sc://[::1]:15002/path1` -> `netloc='[::1]:15002'` -> `split(':')` yields `['[', '', '1]', '15002']` (4 parts) -> falls into the `else` branch and raises `INVALID_CONNECT_URL`.
- `sc://[::1]/path1` -> `['[', '', '1]']` (3 parts) -> same failure.
`DefaultChannelBuilder` (`core.py:486-495`) handles this correctly by using `self.url.hostname` / `self.url.port` and bracket-wrapping IPv6 hosts, so this new builder is a functional regression for IPv6 users. Two related issues on the same path:
- `sc://host:abc/p` -> `int(netloc[1])` raises a raw `ValueError` instead of the wrapped `PySparkValueError`.
- `sc://:15002/p` -> `['', '15002']` (2 parts) -> `host=''` is silently accepted.
Suggested fix: parse host/port via `self.url.hostname` / `self.url.port` as `DefaultChannelBuilder` does (correct IPv6 bracket-wrapping, hostname validation, consistent error path), and derive `netloc_has_port` from `self.url.port is not None`.
### 2. Path port is lost when combined with the standard `/;params` form (`core.py:688-700`)
The trailing-path-port syntax loses the intended port when users combine it with Spark's existing `/;param=value` form. For example `sc://gateway/app/driver:443/;token=abc` parses (via `urlparse`) to `path='/app/driver:443/'`, `params='token=abc'`. Then:
- `last_segment = prefix.rsplit("/", 1)[-1]` on `/app/driver:443/` -> `''` (the trailing slash leaves an empty last segment).
- `":" in last_segment` is therefore `False`, so the port-extraction block (lines 693-700) is skipped.
- Result: `self._port` stays at the fallback `15002`, and `_path_prefix` becomes `/app/driver:443` with `:443` left literally in the prefix.
So the intended port 443 is silently dropped. This is the documented standard param form (the class docstring advertises `sc://host[:port][/;params]`), so combining it with path routing is a natural usage. Without the trailing slash (`sc://gateway/app/driver:443;token=abc`) it works correctly (`last_segment='driver:443'`, port 443 extracted), which confirms the trailing `/` before `;` is the trigger.
Suggested fix: parse the last non-empty path segment (strip a trailing `/` before `rsplit`) so the port is recognized. Given the docstring already promises the `/;params` form, fixing the parser seems preferable to documenting the limitation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4998389513
Hi @viirya , thanks for listing the three options A/B/C. They do not conflict with each other. Ideally we should support all of them, so different companies can use proper solution based on their networking environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4858637039
@viirya @dbtsai this is to support path routing in Spark Connect Python client, would you help to take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-5268035730
Following up on the routing options from the earlier discussion: I ran option C (`:authority`-based routing) end-to-end on a real kind cluster with ingress-nginx v1.15.1, since it's the shape the gRPC maintainers recommended in grpc/grpc#14900. It works, and notably it lets the gateway be reached without mounting it at a URL subpath.
The mechanism: each service is addressed by its own Ingress `host:` value rather than by a path under a shared host. In K8s, different-`host` Ingresses are isolated, so multiple services can each mount at `path: /` on the same ingress-nginx without colliding. I verified this directly — two Ingresses on the same controller, both at `path: /`, `host: sparkconnect` (the gateway) and `host: othersvc` (a dummy) — then sent two gRPC calls to the **exact same connection target** (same host:port, SNI, cert, method path), varying only `default_authority`:
| connection target | `:authority` | routed to |
|---|---|---|
| same endpoint | `sparkconnect` | the gateway (real Spark Connect response) |
| same endpoint | `othersvc` | the other service |
The ingress access log confirms the same `/spark.connect.SparkConnectService/Config` request goes to different upstreams purely by `:authority`. So clients share one entry point (one hostname:port), each service is distinguished by its `:authority` tag, the gRPC `:path` is untouched, and nothing needs a rewrite.
The full picture I verified (current PySpark client, no code change — just channel options):
- **Plaintext (h2c):** `default_authority=sparkconnect` → routed to gateway; a different authority → 404. Routing keys on `:authority`, not SNI (confirmed: real SNI + a nonexistent `:authority` → 404, not a TLS error).
- **TLS:** works too, with one caveat worth stating. gRPC uses the authority as the TLS cert-verification name by default, so if your cert is for the real gateway hostname and you set `default_authority=sparkconnect`, the handshake fails (the tag isn't in the cert SAN). The fix is to keep the two names separate — `grpc.ssl_target_name_override` = the real cert hostname, `grpc.default_authority` = the routing tag. I confirmed both the failure and the fix.
**A correction to my earlier snippet:** I wrote `default_authority` as `"sparkConnect"` (camel case). That's fine for bare nginx `server_name`, but a K8s Ingress `host:` must be a lowercase RFC 1123 name — `host: sparkConnect` is rejected by the API server outright. So the routing tag has to be lowercase (`sparkconnect`) on both sides.
One more operational note: once a `tls:` section is present for that host, ingress-nginx `308`-redirects plaintext `:80` to HTTPS, so clients must connect over TLS.
(I tested the Python client; I haven't checked grpc-java's authority/TLS behavior.)
Net, this doesn't change my earlier reservation about option A — I'd still rather not grow the client toward putting a prefix on the method `:path`. But I wanted to confirm on a real ingress-nginx that the `:authority` route works, so multiple gRPC services can share one entry point without any of them needing a URL subpath or a `:path` rewrite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang closed pull request #56933: [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client
URL: https://github.com/apache/spark/pull/56933
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4910184341
Hi @viirya @dbtsai , thanks for the comments! I updated the PR, would you help to take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4860963092
Thanks for the update — the two issues from the earlier round (IPv6 handling and the path port being lost with the trailing-slash `/;params` form) are both fixed correctly in the current revision, with tests covering them.
A few more issues from a closer pass, roughly in order of importance:
**1. The 443-implies-TLS rule doesn't match the docstring, and one form gets port 443 without TLS**
The docstring says "TLS is enabled implicitly when the resolved port is 443 and `;use_ssl=` was not specified", but the implication sits inside the `if prefix != "/"` block, so:
- `PathAwareChannelBuilder("sc://gateway:443")` (no path) gets no implicit TLS, contradicting the docstring. This matches `DefaultChannelBuilder` behavior, so maybe it's the docstring that should be narrowed to "when a path prefix is present".
- `PathAwareChannelBuilder("sc://host/:443")`: the port-extraction block runs (`last_segment` is `:443`, so `self._port` becomes 443), but the prefix then collapses to `/` and the guarded block is skipped — no TLS implication, no interceptor. The client attempts plaintext gRPC against an HTTPS port. The port assignment and the TLS implication should probably live under the same condition.
**2. A `;key=value` on a non-final path segment is silently absorbed into the prefix**
`urlparse` only strips params from the last path segment, so for `sc://gateway/app;token=abc/driver` we get `url.params == ""` — the token is never set, and `/app;token=abc/driver` becomes the path prefix. Auth silently fails and the credential is sent inside the `:path` of every RPC (and lands in ingress access logs). Since `;` can never be legal inside a route prefix under this grammar, rejecting a prefix containing `;` with `INVALID_CONNECT_URL` would turn this silent failure into a clear error.
**3. Consider subclassing `DefaultChannelBuilder` instead of copying it**
The scheme check in `__init__`, the params/hostname/port block of `_extract_attributes`, the `secure`/`use_ssl`/`host`/`endpoint` properties, and the entire `toChannel` are byte-identical copies of `DefaultChannelBuilder`. The copies have already diverged in one place: the overridden `default_port()` hardcodes 15002 and drops `DefaultChannelBuilder.default_port()`'s `SPARK_TESTING` ephemeral-port resolution, so under the local test harness a portless URL connects to the wrong port.
Since `DefaultChannelBuilder` rejects non-empty paths in `__init__`, the subclass can pre-process the raw URL string instead: extract the path prefix (and the optional trailing `:port`), rebuild a path-free `sc://host:port/;params` URL, delegate to `super().__init__`, then set the SSL default and register the interceptor. That reduces the ~120 duplicated lines to ~30 lines of genuinely new logic, and future fixes to parsing/credential handling apply to both builders automatically.
**4. The connection-string spec and other clients**
`sql/connect/docs/client-connection-string.md` states "The path component must be empty" and lists `sc://myhost.com:443/mypathprefix/;token=AAAAAAA` as an explicitly invalid example — which is the exact form this PR makes valid, in the Python client only (the Scala client's `verifyURI` still rejects non-empty paths). Since the new form is opt-in via a separate builder and the default `sc://` parser is unchanged, I don't think this blocks the PR, but the doc should be updated to describe the extension (or explicitly mark it Python-only), and a follow-up JIRA for Scala-client parity would be good so the same connection string stays portable.
**5. Path-derived port skips validation the netloc port gets**
The netloc form goes through `urlparse`, which enforces 0-65535, but the path form is a bare `int()`: `driver:99999` -> port 99999, `driver:-1` -> port -1, and `driver:4_43` -> 443 via PEP 515 underscores, all silently accepted with the bogus `:PORT` stripped from the prefix. A range/`isdigit()` check raising `INVALID_CONNECT_URL` would make the two forms consistent.
**6. `SparkSession.Builder.channelBuilder` annotation**
`session.py` annotates `channelBuilder` (and `_channel_builder` at L129, and `connection` at L282) as `DefaultChannelBuilder`, but `PathAwareChannelBuilder` extends `ChannelBuilder` directly — so the usage advertised in this PR's description is an arg-type error under mypy/pyright. Runtime is fine, but those annotations should be widened to `ChannelBuilder` as part of this PR, since this is the first shipped builder that isn't a `DefaultChannelBuilder`.
Minor: the class docstring's doctest is malformed (the `...` continuation line holds an independent expression, and the expected output uses double-quoted repr) — it's copied from `DefaultChannelBuilder`'s existing docstring and never collected, but new code needn't repeat it. The four `intercept_*` methods with identical bodies could also collapse into one implementation aliased four times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-5284547540
I followed @viirya 's example/suggestion and got it working. Thanks @viirya ! Will close this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-5284662664
Thank you @hiboyang !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4859971286
> ## Review feedback
> Two issues in `PathAwareChannelBuilder._extract_attributes` (`python/pyspark/sql/connect/client/core.py`).
>
> ### 1. IPv6 endpoints are rejected (`core.py:667`)
> `netloc = self.url.netloc.split(":")` with `len(netloc) in (1, 2)` breaks on IPv6 addresses:
>
> * `sc://[::1]:15002/path1` -> `netloc='[::1]:15002'` -> `split(':')` yields `['[', '', '1]', '15002']` (4 parts) -> falls into the `else` branch and raises `INVALID_CONNECT_URL`.
> * `sc://[::1]/path1` -> `['[', '', '1]']` (3 parts) -> same failure.
>
> `DefaultChannelBuilder` (`core.py:486-495`) handles this correctly by using `self.url.hostname` / `self.url.port` and bracket-wrapping IPv6 hosts, so this new builder is a functional regression for IPv6 users. Two related issues on the same path:
>
> * `sc://host:abc/p` -> `int(netloc[1])` raises a raw `ValueError` instead of the wrapped `PySparkValueError`.
> * `sc://:15002/p` -> `['', '15002']` (2 parts) -> `host=''` is silently accepted.
>
> Suggested fix: parse host/port via `self.url.hostname` / `self.url.port` as `DefaultChannelBuilder` does (correct IPv6 bracket-wrapping, hostname validation, consistent error path), and derive `netloc_has_port` from `self.url.port is not None`.
>
> ### 2. Path port is lost when combined with the standard `/;params` form (`core.py:688-700`)
> The trailing-path-port syntax loses the intended port when users combine it with Spark's existing `/;param=value` form. For example `sc://gateway/app/driver:443/;token=abc` parses (via `urlparse`) to `path='/app/driver:443/'`, `params='token=abc'`. Then:
>
> * `last_segment = prefix.rsplit("/", 1)[-1]` on `/app/driver:443/` -> `''` (the trailing slash leaves an empty last segment).
> * `":" in last_segment` is therefore `False`, so the port-extraction block (lines 693-700) is skipped.
> * Result: `self._port` stays at the fallback `15002`, and `_path_prefix` becomes `/app/driver:443` with `:443` left literally in the prefix.
>
> So the intended port 443 is silently dropped. This is the documented standard param form (the class docstring advertises `sc://host[:port][/;params]`), so combining it with path routing is a natural usage. Without the trailing slash (`sc://gateway/app/driver:443;token=abc`) it works correctly (`last_segment='driver:443'`, port 443 extracted), which confirms the trailing `/` before `;` is the trigger.
>
> Suggested fix: parse the last non-empty path segment (strip a trailing `/` before `rsplit`) so the port is recognized. Given the docstring already promises the `/;params` form, fixing the parser seems preferable to documenting the limitation.
Thanks for the comments! Will make these changes after I get initial PR check passing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-5257771238
Following up on my earlier comment about the three routing shapes — I went and actually ran the experiments this time, both for option A (this PR) and option C (`:authority`), so here's what I found on the record.
**Option A — the prefix on `:path` is a two-sided contract, and I confirmed the failure mode.**
Because the gRPC method name *is* the HTTP/2 `:path`, prepending `/sparkConnect` makes the wire path `/sparkConnect/spark.connect.SparkConnectService/AnalyzePlan`. I verified against current Spark master (in-process `SparkConnectService`, a raw gRPC channel carrying the prefixed path) that the server returns `UNIMPLEMENTED` for the prefixed method and serves the clean one. So the driver only works if the ingress strips the prefix back off — correctness is split across two sides (client prepends, ingress strips), and a client-only misconfiguration fails with a confusing `UNIMPLEMENTED`.
To be precise about the ingress rewrite: I set up nginx with `rewrite ^/sparkConnect/(.*) /$1 break;` + `grpc_pass` and it **did** strip the prefix correctly, for both unary and streaming RPCs (nginx 1.31.3) — so a working setup like yours doesn't surprise me. My concern is narrower than "it doesn't work": it's that this rewrite is not officially supported and has no stability contract. `grpc_pass` takes an address, not a URI, and its docs don't cover rewriting `:path`; the official ingress-nginx gRPC example mounts the service at `/` with no `rewrite-target`; and there's no e2e coverage for the gRPC + rewrite combination. It may work on one nginx build and change on another — it's a rewrite the operator owns and maintains.
**Option C — `:authority` — I ran it end-to-end and it works, including under TLS.**
Topology: **current** PySpark client (no code change) → nginx → Spark Connect Gateway → a real Spark Connect driver. On the client I set the `grpc.default_authority` channel option; on nginx I route with a plain `server_name`. A real `spark.sql()` runs through the whole chain, and `:path` is never touched. Setting the authority to a different value returns 404 from nginx, confirming routing keys on `:authority`.
```python
from pyspark.sql.connect.session import SparkSession
from pyspark.sql.connect.client import DefaultChannelBuilder
cb = DefaultChannelBuilder("sc://cluster1.xyz.com:443")
cb.setChannelOption("grpc.default_authority", "sparkConnect")
spark = SparkSession.builder.channelBuilder(cb).getOrCreate()
```
```nginx
server {
listen 443 ssl;
http2 on;
server_name sparkConnect; # matches :authority
location / { grpc_pass grpc://spark-connect-gateway:15003; }
}
```
On the TLS wrinkle I raised earlier for C: it's resolvable. Overriding `:authority` alone does fail cert verification (gRPC uses the authority as the TLS verification name by default), but keeping the two names separate fixes it — I set `grpc.ssl_target_name_override` to the gateway's real hostname (so the cert only needs to cover that) and `grpc.default_authority` to the routing tag, and the handshake + routing both work. I confirmed this with grpcio.
I do have a real reservation about the direction: option A grows the client toward polluting `:path` and depending on an ingress rewrite that has no support contract to undo it, and that's a shape I'm not comfortable baking into the client. Option C (the one the gRPC maintainers recommended in grpc/grpc#14900) reaches the same goal with no client code change and without touching the request at all — given that, I'm not convinced adding A to the client is the right trade-off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4986691321
Thanks for the update — the client-side implementation reads cleanly and the tests are thorough. Before this lands I want to raise some concerns about the routing approach itself, less about the code than the direction, so they're on the record for you and other reviewers to weigh.
The core question is *where* the client puts the routing hint. There are three known shapes for the same "route to the right driver behind one gateway" need, each with a different catch:
**A — prefix on `:path` (this PR).** Routes on standard ingress path-prefix matching, and the TLS cert only needs the gateway's real hostname. The catch is what comes with putting the hint in `:path`:
- *The gRPC community has looked at this and advised against it.* grpc/grpc#14900 is essentially this exact scenario — routing to different gRPC backends behind nginx by URL. The maintainer's guidance there was explicitly *not* to put routing info in `:path`, since `:path` is the method key that gRPC generates from the proto (`package.Service/Method`); the host used to build the channel is not meant to end up in it. So A goes against the shape the gRPC folks recommended for this problem (their suggested alternative is C, below).
- *The server-side half of the contract isn't documented.* A request goes out as `/mypathprefix/spark.connect.SparkConnectService/ExecutePlan`, so the ingress must strip that prefix back off before forwarding — otherwise the driver gets an unknown method and returns `UNIMPLEMENTED`. That's a two-sided contract (client prepends, ingress rewrites), but the docs only cover the client half; a user who configures only the client will hit a confusing failure. The doc should at least state that the ingress has to rewrite the prefix away.
- *That rewrite may not have a clean story on the most common ingress.* For a gRPC backend, ingress-nginx uses `grpc_pass` (via `backend-protocol: "GRPC"`), not `proxy_pass`, and I couldn't find an officially supported, reliable way to strip a path prefix there. The official gRPC examples (ingress-nginx's own, and NGINX's gRPC API-gateway guide) route with `location` matching and never use `rewrite`; there are reports of `backend-protocol: GRPC` + `rewrite-target` failing the HTTP/2 handshake (kubernetes/ingress-nginx#9493). It may work on Envoy/Istio/Gateway API's URLRewrite or a hand-written nginx snippet, but not out-of-the-box on plain ingress-nginx. A working nginx-ingress config linked from the docs would help a lot.
**B — a custom `x-` metadata header** (the alternative I raised in #56816)**.** Leaves `:path` clean, needs no rewrite (a backend that doesn't know the header just ignores it — no driver change), and keeps the cert story as simple as A. The catch: routing on an arbitrary header isn't part of the standard K8s Ingress model (only host + path are), so it leans on controller-specific config or Gateway API.
**C — `:authority` (virtual hostname).** This is the alternative the gRPC community recommended in grpc/grpc#14900: use `:authority` via the `default_authority` channel option as a virtual hostname, routed by nginx `server_name`. Needs no rewrite either. The catch is a TLS wrinkle, because cert selection happens during the TLS handshake — before any HTTP/2 header (including `:authority`) is on the wire — so the gateway picks the cert from SNI, not from `:authority`. That leaves two ways to set it up, each with a cost:
- *Set SNI to the logical name too* (SNI = `:authority` = `app-1`): routing works, but the handshake now presents SNI `app-1`, so the **cert must cover `app-1`** (and every other logical name) — you can't get by with a cert for just the gateway's real hostname, which is the thing grpc/grpc#14900 warns about.
- *Override only `:authority`, leave SNI as the gateway hostname*: the cert stays simple (SNI still matches the gateway's real cert), but now SNI and `:authority` disagree, and the data plane has to be willing to route on `:authority` in that case — some nginx setups instead require SNI and Host/authority to match. So this path works only if the routing layer supports it.
Either way, C trades A's rewrite problem for a cert/SNI one. (Note grpc/grpc#14900's working example was plaintext h2c, where there's no SNI and `:authority` is the only thing to route on; the wrinkle only appears once TLS is in play.)
I'm not arguing any one is strictly better — each has its own catch. But since C is the officially-suggested shape and B sidesteps the rewrite entirely, it seems worth being explicit about why the PR takes A.
None of this is a blocker from me on the code — it's well done. These are questions about which routing mechanism we want the client to grow, and I'd rather surface them now than after it ships.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org