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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
