balancerd: reland SNI route refactor (#33955) with nightly test fixes#37579
Draft
jubrad wants to merge 5 commits into
Draft
balancerd: reland SNI route refactor (#33955) with nightly test fixes#37579jubrad wants to merge 5 commits into
jubrad wants to merge 5 commits into
Conversation
This un-reverts commit 05c9980 (MaterializeInc#37570), which reverted MaterializeInc#33955. The SNI route refactor is sound; it was reverted only to unblock a release while the balancerd nightly test failures it exposed were addressed. Those fixes follow as the subsequent commits on this branch.
The nightly http and ip-forwarding workflows started failing after the SNI route refactor (MaterializeInc#33955): the client's HTTPS request to balancerd is met with a dropped connection (RemoteDisconnected). Root cause is test configuration, not a product bug. MaterializeInc#33955 changed the default --https-sni-resolver-template from `materialized:6876` to `sni.test:6876`. `sni.test` resolves only through the dnsmasq CNAME, but these two workflows do not run dnsmasq, so balancerd cannot resolve the upstream and drops the connection (resolution failure returns no HTTP response). balancerd itself behaves correctly and does not crash. The resolution code path is unchanged in behavior for resolvable targets, the old code would fail the same way on an unresolvable template. Point the default HTTPS template back at `materialized`, which resolves via docker's embedded DNS without dnsmasq. These tests exercise the generic HTTPS proxy, not SNI routing, so they do not need the CNAME. SNI routing is still covered over pgwire, which keeps the sni.test template and starts dnsmasq. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
workflow_user asserts mz_balancer_tenant_connection_rx{source="pgwire"}
but closed only the cursor, not the connection. balancerd records the
per-connection tx/rx metrics when the proxied connection closes (when
copy_bidirectional returns), so with the connection held open the metric
was never recorded and the assertion failed.
Close the connection instead of the cursor. Also poll in assert_metrics
to cover the short delay between the client disconnecting and balancerd
recording the metric.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The error refactor makes balancerd report an opaque message to clients
when it cannot resolve or reach the upstream, rather than propagating the
raw DNS/connection error. workflow_mz_not_running asserted on the old raw
messages (e.g. "failure in name resolution"), so with Mz down it now
sees "internal error" and failed.
Accept balancerd's hardened messages ("internal error" for a resolve
failure, "upstream server not available" for a connection failure), and
add a message to the assertion for easier diagnosis next time.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When balancerd could not reach a tenant's backend (its hostname did not resolve) it reported a generic "internal error" to the client, which reads like a balancerd bug rather than the environment being down. Add a ResolveError::Upstream variant with the message "upstream server not available" (already the message when the upstream connection is refused) and use it for resolve failures on both the SNI and Frontegg paths, making the resolve-failure and connect-failure cases consistent. It is logged at warn!, not error!: a bogus SNI reaches it from an unauthenticated client, and a down environment is an operational condition, not a balancerd fault. The message stays opaque, so no internal detail leaks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
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
1 participant
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.
Relands #33955 (Balancerd - refactor SNI route), which was reverted in #37570 to unblock a release. The refactor itself is sound; the revert was only to buy time to fix the balancerd nightly failures it exposed. This PR brings it back with those fixes applied.
The first commit un-reverts #37570; the rest are the fixes, each a separate commit for review.
Fixes included
http/ip-forwarding— the refactor changed the default--https-sni-resolver-templatetosni.test, which only resolves via the dnsmasq CNAME; these workflows don't run dnsmasq, so balancerd couldn't resolve the upstream and dropped the connection. Point the default HTTPS template back atmaterialized(resolvable via docker's embedded DNS). SNI routing stays covered over pgwire.user— the workflow asserted the pgwire tenanttx/rxmetrics but closed only the cursor, not the connection. Those metrics are recorded when the proxied connection closes, so they were never emitted. Close the connection, and poll inassert_metricsfor the brief close-to-record window.mz-not-running— the error-hardening made balancerd report an opaque message to clients on resolve/connect failure instead of the raw DNS error the test matched. Accept balancerd's hardened messages.UX: upstream failures — when a tenant's backend can't be reached, balancerd reported a generic
"internal error", which reads like a balancerd bug rather than the environment being down. Report"upstream server not available"(already used for connection refusals) for resolve failures too, making the resolve- and connect-failure cases consistent. The message stays opaque, so no internal detail leaks.The full balancerd nightly suite (
bin/mzcompose --find balancerd run default) passes with these fixes.