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: [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]
