[dataplane]: Own the request-path topology - #127
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR centralizes ownership of the gateway↔per-upstream proxy request-path topology into a new internal/dataplane package, replacing the prior split responsibility across router.Module, proxy.Module, and server.Module. It also introduces a dataplanetest fixture package to standardize e2e-style setup (fake upstreams, ports, dialers, lifecycle), and updates tests accordingly.
Changes:
- Add
internal/dataplaneto assemble routing, gateway server, per-upstream proxies, lifecycle management, and abort propagation in one place (plus anfx.Modulewrapper). - Remove
internal/{server,router,proxy}/fx.gomodules and their fx wiring tests, updating docs/comments to reflect the new ownership model. - Introduce
internal/dataplane/dataplanetestand refactor e2e tests to use it (reducing duplicated setup and avoiding racy “reserve a port then bind it” helpers).
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/transport/socket/socket.go | Update docs to reflect single-derivation ownership of Unix socket paths. |
| internal/server/fx.go | Remove server fx module (superseded by dataplane ownership). |
| internal/server/fx_test.go | Remove server module fx wiring tests (superseded by dataplane tests). |
| internal/router/fx.go | Remove router fx module (superseded by dataplane ownership). |
| internal/router/fx_test.go | Remove router module fx wiring tests (superseded by dataplane tests). |
| internal/router/doc.go | Update package docs to point to internal/dataplane as the assembler/owner. |
| internal/proxy/translation.go | Export dial-option helper (TranslationDialOptions) for dataplane assembly. |
| internal/proxy/translation_test.go | Update tests to use the exported TranslationDialOptions. |
| internal/proxy/server.go | Clarify Start semantics in comment (Stop-driven, ctx for health check). |
| internal/proxy/server_test.go | Add upstream helpers used by proxy tests (serveUpstream/deadUpstream). |
| internal/proxy/fx.go | Remove proxy fx module (superseded by dataplane ownership). |
| internal/proxy/fx_test.go | Remove proxy module fx wiring tests (superseded by dataplane tests). |
| internal/dataplane/reporters.go | Add centralized reporter construction (one-per-registry) with panic recovery. |
| internal/dataplane/lifecycle.go | Implement ordered Start/Stop, rollback-on-failed-start, and abort-on-unexpected-exit. |
| internal/dataplane/lifecycle_test.go | Add lifecycle coverage (startup ordering, rollback behavior, shutdown behavior). |
| internal/dataplane/gateway_test.go | Add gateway forwarding/metrics/auth integration tests through the assembled dataplane. |
| internal/dataplane/fx.go | Add dataplane.Module and lifecycle binding in fx graphs. |
| internal/dataplane/fx_test.go | Test fx lifecycle and shutdown-on-unexpected-stop behavior. |
| internal/dataplane/export_test.go | Export listener access for tests to force unexpected-stop paths. |
| internal/dataplane/doc.go | New package doc for dataplane’s responsibilities. |
| internal/dataplane/dataplanetest/upstream.go | Add reusable fake Temporal frontend upstream (plaintext + TLS variants). |
| internal/dataplane/dataplanetest/upstream_test.go | Tests for upstream TLS reachability + request recording behavior. |
| internal/dataplane/dataplanetest/doc.go | New package doc for dataplane test fixtures. |
| internal/dataplane/dataplanetest/dataplanetest.go | Add fixture helpers to start a dataplane directly or via full fx graph. |
| internal/dataplane/dataplanetest/dataplanetest_test.go | Add fixture tests (forwarding, port assignment, config rejection, etc.). |
| internal/dataplane/dataplane.go | Core dataplane assembly: validates config/deps, builds tiers, derives socket paths once. |
| internal/dataplane/dataplane_test.go | Unit tests for dataplane.New dependency validation and metrics-registration behavior. |
| internal/dataplane/abort_test.go | Tests for abort behavior (fires once, optional, suppressed during clean shutdown). |
| internal/api/fx.go | Update module comment to reference dataplane.Module ordering/validation ownership. |
| internal/api/fx_test.go | Update test comment to match module ordering change. |
| e2e/upstream_credential_socket_test.go | Refactor to use dataplanetest (TLS upstream + direct upstream-socket access). |
| e2e/templated_upstream_test.go | Refactor to use dataplanetest.StartApp and shared upstream helpers. |
| e2e/outbound_credential_override_test.go | Refactor to use dataplanetest and TLS upstream helper. |
| e2e/inbound_credential_no_leak_test.go | Refactor to use dataplanetest and TLS upstream helper. |
| e2e/inbound_auth_strip_test.go | Refactor to use dataplanetest and TLS upstream helper. |
| e2e/harness_test.go | Remove bespoke e2e harness helpers (superseded by dataplanetest). |
| e2e/encryption_test.go | Refactor to use dataplanetest upstream + fixture-based client driving. |
| cmd/proxy/serve.go | Wire dataplane.Module instead of router/proxy/server modules in production app. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The request path could only be assembled as a side effect of building an fx graph. `router.Module`, `server.Module`, and `proxy.Module` each provided one piece, and the gateway met the per-upstream proxies through two anonymous graph keys, stream handler and codec, plus a socket path each side derived from host:port on its own. The contract between the tiers had no real owner: anything else providing a `grpc.StreamHandler` would silently become the gateway's forwarder. 'internal/dataplane' owns that topology now. New takes everything necessary and builds the gateway/proxy request handlers. Serve-exit propagation, which had no test anywhere, goes through a single Abort callback that fires at most once and stays quiet once shutdown has begun, rather than the copy of the fx.Shutdowner call each tier module carried. `dataplane.Module` binds `Start` and `Stop` to the fx lifecycle and points `Abort` at the shutdowner with exit code 1; a nil Abort leaves the logged error as the only record. On the test side, every e2e case hand-rolled the same assembly: an fx app, a fake upstream, a free TCP port, a unix dialer, and start/stop plumbing. Two fake frontend types were stood up three different ways, and the port helper carried a comment admitting its own race, since it closed a listener and hoped nothing else claimed the port before the gateway bound it. internal/dataplane/dataplanetest owns all of it, as a normal sibling package rather than part of pkg/testutil because it returns internal types no external caller can name, and as a non-test package so both package e2e and package dataplane_test can import it. e2e no longer builds a graph at all.
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.
The request path could only be assembled as a side effect of building an fx graph.
router.Module,server.Module, andproxy.Moduleeach provided one piece, and the gateway met the per-upstream proxies through two anonymous graph keys, stream handler and codec, plus a socket path each side derived from host:port on its own. The contract between the tiers had no real owner: anything else providing agrpc.StreamHandlerwould silently become the gateway's forwarder.'internal/dataplane' owns that topology now. New takes everything necessary and builds the gateway/proxy request handlers. Serve-exit propagation, which had no test anywhere, goes through a single Abort callback that fires at most once and stays quiet once shutdown has begun, rather than the copy of the fx.Shutdowner call each tier module carried.
dataplane.ModulebindsStartandStopto the fx lifecycle and pointsAbortat the shutdowner with exit code 1; a nil Abort leaves the logged error as the only record.On the test side, every e2e case hand-rolled the same assembly: an fx app, a fake upstream, a free TCP port, a unix dialer, and start/stop plumbing. Two fake frontend types were stood up three different ways, and the port helper carried a comment admitting its own race, since it closed a listener and hoped nothing else claimed the port before the gateway bound it. dataplanetest package owns all of this now.