Skip to content

fix(worker): Send a portless :authority from the push client - #795

Merged
evanh merged 1 commit into
mainfrom
evanh/fix/push-client-default-authority
Sep 23, 2026
Merged

evanh merged 1 commit into
mainfrom
evanh/fix/push-client-default-authority

Conversation

@evanh

@evanh evanh commented Sep 22, 2026

Copy link
Copy Markdown
Member

grpc puts the whole host:port target in :authority, so a push worker dialing task-<pool>-broker-grpc:50051 through the mesh matches no route — HttpRoute hostnames are RFC1123 names and cannot carry a port — and envoy's 404 comes back to the worker as UNIMPLEMENTED. That is what broke getsentry/ops#23562.

Setting grpc.default_authority to the target without its port makes the worker look like the broker, which reaches its worker over a portless URL and has always matched. Push client only; the pull client's per-pod channels are untouched.

Safe to ship on its own — with getsentry/ops#23562 reverted the workers reach the broker over its ClusterIP, nothing on that path reads :authority, and the request signature covers method + body rather than the headers.

@evanh
evanh requested a review from a team as a code owner September 22, 2026 20:14
@evanh

evanh commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

Verified against the live mesh config and on the wire.

The sidecar on a running s4s2 push worker, envoy admin on 15000 (9902 is the stats adapter, not envoy):

$ kubectl -n default port-forward pod/task-ingest-push-worker-bdbbbc876-2gcdd 19000:15000
$ curl -s localhost:19000/config_dump | jq ...

route config projects/.../meshes/saas-mesh   ignore_port_in_host_matching=false   38 vhosts, 0 wildcard
  httpRoutes/task-ingest-push-broker-http-route-v2   domains: ["task-ingest-push-broker-grpc"]
  httpRoutes/task-ingest-push-worker-http-route-v2   domains: ["task-ingest-push-worker-grpc"]

One domain each, no port variant, no catch-all, and port matching is not ignored — so task-ingest-push-broker-grpc:50051 matched nothing and envoy 404'd. Changing the dial port would not have helped; interception is a single 0.0.0.0:15001 listener whose catch-all chain is an HCM, so the destination port never selects a route, only the authority does.

What this branch actually sends, read off the HEADERS frame of a real PushTaskbrokerClient call against a raw socket (hpack-decoded):

connect=127.0.0.1:55167  :authority='127.0.0.1'  :path='/sentry_protos.taskbroker.v1.ConsumerService/SetBatchActivationStatus'

Before the change that authority was 127.0.0.1:55167.

Tests:

$ uv run pytest tests/worker/test_push_clients.py -q
6 passed in 1.55s

$ uv run pytest tests -q --ignore=tests/scheduler
3 failed, 234 passed

The three are pre-existing on main — test_run_once_current_task_state and test_child_process_pass_headers fail unchanged with the diff stashed, and test_track_memory_usage is timing-flaky (passes 3/3 in isolation on this branch). tests/scheduler errors out without redis running.

mypy -p taskbroker_client, flake8, black and isort are clean. The mypy-client pre-commit hook fails in my checkout on untouched files too (missing zstandard / types-protobuf stubs in the system env), so the commit is --no-verify; it should pass in CI.

grpc puts the whole host:port target in :authority, so a push worker dialing
task-<pool>-broker-grpc:50051 through the mesh matches no route -- HttpRoute hostnames are RFC1123
names and cannot carry a port -- and envoy's 404 comes back as UNIMPLEMENTED. Setting
grpc.default_authority to the target without its port makes the worker look like the broker, which
reaches its worker over a portless URL and has always matched.
@evanh
evanh force-pushed the evanh/fix/push-client-default-authority branch from 3d9a265 to c2f8850 Compare September 22, 2026 20:15
@evanh
evanh merged commit fb3e24c into main Sep 23, 2026
29 checks passed
@evanh
evanh deleted the evanh/fix/push-client-default-authority branch September 23, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants