feat: Allow for external services and declarative port binding#69
feat: Allow for external services and declarative port binding#69scottpledger wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d42ba6037
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if service.Type == "external_service" { | ||
| return service.WaitUntilHealthy(ctx) |
There was a problem hiding this comment.
Honor health_check_timeout for external services
When an itest_external_service has an HTTP or command health check that keeps failing and health_check_timeout is set, this early return calls WaitUntilHealthy before the timeout-wrapping block below runs. The external-service health loop therefore polls until the outer Bazel/test timeout instead of failing after the configured service timeout, which makes unreachable external dependencies hang much longer than requested.
Useful? React with 👍 / 👎.
| "so_reuseport_aware": ctx.attr.so_reuseport_aware, | ||
| "deferred": ctx.attr.deferred, | ||
| "domain": ctx.attr.domain, | ||
| "port_bindings": _compute_port_bindings(ctx), |
There was a problem hiding this comment.
Allow SO_REUSEPORT with declarative ports
When a service uses only the new ports = {":p": "name"} binding, _compute_port_bindings(ctx) still creates an autoassigned socket, but the validation above only allows so_reuseport_aware with legacy autoassign_port or named_ports. As a result, users adopting first-class itest_port targets cannot enable the collision-avoidance mode for those autoassigned ports and get an analysis failure even though the runtime path supports SoReuseportAware for all bindings.
Useful? React with 👍 / 👎.
|
Thanks for sending this! I've definitely considered a first class port target in the past and I'm excited to look at what you've cooked up here. It might take me a bit to get to it but wanted to ack the PR |
3d42ba6 to
7337a01
Compare
No problem! This is something I've been thinking about for a while, given how we use this library at my company. We currently just create multiple test targets - one for local, test, preview, and prod. However, this approach doesn't give us the flexibility to mix and match service locations (eg, a local web server with test remote APIs). |
|
Hey, @dzbarsky! Any chance you will be able to look at this sometime soon? I now have an actual use-case for it and I'd love to get feedback on this approach before I start adopting it. |
| must(err) | ||
|
|
||
| ports, err := assignPorts(unversionedSpecs) | ||
| ports, portsMap, servicesMap, err := assignPorts(unversionedSpecs) |
There was a problem hiding this comment.
note to self: how is ports different from portsMap?
| // legacy string->port map (for substitution / ASSIGNED_PORTS) and the rich maps. | ||
| register := func(serviceLabel, portName, domain, portStr, target string, aliases []string) { | ||
| info := svclib.BindingInfo{ | ||
| Origin: domain + ":" + portStr, |
There was a problem hiding this comment.
alternately, why not construct it on the fly when needed instead of storing twice?
| s.Env[k] = strings.ReplaceAll(v, "$${PORT}", port) | ||
| } | ||
| } | ||
| if s.Env == nil { |
There was a problem hiding this comment.
when does this happen? This feels like a bit ad-hoc place to normalize it, should it happen earlier if it's truly necessary?
| bindings.append(struct( | ||
| target = label + "." + name, | ||
| name = name, | ||
| # Keep the legacy `:`-separated key working. |
There was a problem hiding this comment.
i think we can do a separate pre-change to cleanup the :-separated legacy stuff, wdyt? I originally said I would break it in 2 months :) https://bazelbuild.slack.com/archives/C075VMU8T0Q/p1770213602739929
| default = "127.0.0.1", | ||
| doc = """The host that this service's ports are reachable on. Defaults to `127.0.0.1` for locally-managed services.""", | ||
| ), | ||
| "ports": attr.label_keyed_string_dict( |
There was a problem hiding this comment.
i wonder if we can make itest_port itself be a flag, wdyt? Would that allow collapsing this attr with port_values?
|
|
||
| bindings = [] | ||
| for port_target, name in ctx.attr.ports.items(): | ||
| info = port_target[_PortInfo] |
There was a problem hiding this comment.
i think if we do the above we can resume of these bad states/etc as well
| value = ctx.attr.port_numbers[name], | ||
| )) | ||
|
|
||
| extra_service_spec_kwargs = { |
There was a problem hiding this comment.
feels like we should reuse more of the code betwen internal/external, wdyt
| // External services are not spawned by us, so there is no process to watch for exit. | ||
| // If a health check is configured, poll it; otherwise assume the service is reachable. | ||
| if s.Type == "external_service" { | ||
| if s.HttpHealthCheckAddress == "" && s.ServiceSpec.HealthCheck == "" { |
There was a problem hiding this comment.
maybe add a s.PollUntilHealthy(ctx context.Context) method so we can reuse it?
Apologies for the delay, I've left some feedbacks. At a high level, I think the general idea makes sense, hopefully we can streamline the API a bit, but I don't think it would require major rework from you to migrate from how it's setup now to whatever the final state ends up, I expect it would be fairly close. I've also tagged in @darkrift to help review this as he is using this ruleset more actively than I am these days :) |
|
I very much like the global idea, while I don't have a use case for this yet, it's something that makes alot of sense. I'm currently a bit busy but will try to get my eyes on this by the end of the week |
7337a01 to
c816346
Compare
Allows services to be described in more detail than just as a local port, so a test suite can run against either locally-managed services or production-like instances (selected with Bazel
select()).Important changes/features
itest_port— declare a port as a first-class target. The binding info (value + host) is supplied by whichever service binds it, and a port can only be bound once.itest_external_service— point at a fixed FQDN instead of a locally-spawned binary. It exposes the same provider asitest_service, so it's a drop-in replacement viaselect(). The manager never starts/stops it (optional health check only).itest_servicegains domain (default127.0.0.1) and aportsattribute.ITEST_PORTS_MAPandITEST_SERVICES_MAP(string-encoded JSON) describe every port/service with{origin, domain, port}, injected into the test and all child services.GET /v0/portsandGET /v0/serviceslist this for all services at once.I also made this fully backward-compatible: existing macros auto-create the needed ports/aliases;
port(),ASSIGNED_PORTS,GET_ASSIGNED_PORT_BIN,/v0/port, and--//pkg:svc.port=Noverrides all still work. The underlying rules are now exported for extension.Example
The test can then read the connection info from either:
ITEST_PORTS_MAP(or/v0/ports) (keyed by port target label, plus any aliases):{ "@@//myapp:db_port": { "origin": "127.0.0.1:54321", "domain": "127.0.0.1", "port": "54321" } }--//myapp:use_external_db, the same keys instead resolve to the production-like instance:{ "@@//myapp:db_port": { "origin": "db.staging.mycompany.com:5432", "domain": "db.staging.mycompany.com", "port": "5432" } }ITEST_SERVICES_MAP(or/v0/services), e.g.:{ "@@//myapp:db": { "sql": { "origin": "127.0.0.1:54321", "domain": "127.0.0.1", "port": "54321" } } }Note: in the example above,
:db_externaland:dbboth bind:db_port— that's valid becauseselect()resolves to only one of them at a time in theservice_testrule, so the port is still only bound once.Testing
Added
tests/ports, which covers internal ports, an external service, theselect()swap, the new maps/endpoints, and ensuring ports & aliases are only ever bound once. The full test suite passes (48/48), and the examples build cleanly.Caveats
/my/appor something. I don't think this would be too hard to add in a follow-up, though.Solves: #70