Repository navigation
test(OMN-15744): golden and error chain tests for the MSK SNI bastion path - #3418
Conversation
… path
Nothing watched the lab -> SNI bastion -> MSK routing path, which is why a
2 -> 6 broker scale-out on 2026-08-06 left four of six brokers mis-routed for
a month. Bootstrap and metadata are served by any broker, so every liveness
surface stayed green; only leader-routed requests failed, with
NotLeaderForPartition, which reads as transient noise. It was found by hand
during an unrelated diagnosis. These tests replace that luck.
Three tests in tests/integration/bastion:
golden — every live broker answers AS ITSELF through the map. Discriminates
on the broker's own error code: a leader-routed offsets request returns 0
from the partition leader and 6 from any other broker, so the answer
reports the responder's identity. Carries its own positive control.
error — an SNI the map does not carry must fail at connect, not silently
reach a real broker. The original defect was precisely that an unknown
name got a working session from the wrong broker. A mapped SNI must
succeed in the same run, so a down bastion cannot read as a pass.
resize detector — every broker the MSK control plane reports must be
routable, with an unmapped name required to be unroutable so the claim is
not vacuous. This is the test that turns red on the next resize.
A TLS-only test was considered and rejected on measurement, not taste: all six
broker SNIs present ONE wildcard certificate (sha256[:16] e4224daa249fdb90),
so a handshake test cannot tell which broker answered and would have been
green throughout the outage.
"Produce to a topic led by each of the six brokers" was the first design and is
not achievable: the sanctioned topic path governs replication factor only and
nothing here can influence leader placement, so such a topic would land
wherever the cluster chose. The error-code discriminator covers all six
brokers deterministically instead, broker 4 included.
Scheduled hourly by msk-bastion-canary.yml on the self-hosted omnibase-deploy
runner, the only runner with reach to the bastion. Read-only by design: no
produce and no topic creation against tenant data on a schedule.
tests/ci/test_msk_bastion_canary_workflow.py guards the canary against being
defanged — dropped schedule, wrong runner, continue-on-error, legs no longer
invoked, or a produce creeping in. The bastion marker is excluded from the
nightly sweep, which runs where the bastion is unreachable and would otherwise
go permanently red.
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
…ibase_infra#3418 (#9000) * evidence(OMN-15744): author OCC companion for OmniNode-ai/omnibase_infra#3418 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 485784a0ec507e19ca9e902eae252cc45a111e6a. * evidence(OMN-15744): self-bind OCC#9000 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 2
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 2
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (glm-review)
Golden test cannot discriminate non-leader broker identity | The discriminator rests entirely on error code 6 (NotLeaderForPartition), which is returned by EVERY broker that is not the partition leader. Under the exact stale-map defect this canary exists to detect, SNI b-3..b-6 routed to the wrong non-leader broker still yields error 6. The golden test only detects misrouting when the leader's own SNI lands on a non-leader (or vice versa). For a reference partition led by broker L, the other five SNIs are i
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
TLS certificate verification disabled in _tls_probe | ssl.CERT_NONE with check_hostname=False disables all server identity verification. The test then asserts a security property (fail-closed routing) over a channel where the endpoint is unauthenticated: any host that can intercept traffic to the bastion IP can satisfy the mapped-SNI positive control and complete the handshake. The Resize Detector and Error Chain tests derive their entire evidentiary value from the peer's identity, yet never verify it. Addi
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Global getaddrinfo monkeypatch is not concurrency-safe | _dns_pinned_to_bastion replaces socket.getaddrinfo process-wide. aiokafka resolves via loop.getaddrinfo, which executes the patched callable in the default executor's worker threads. The patch-and-restore is not atomic against other coroutines or threads resolving concurrently within the same process, and if the golden test is ever run alongside other async tests in one process, the window between patch and restore leaks pinned resolution to unrelated
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Reference partition selection is a single point of discrimination | _pick_reference_partition returns the FIRST tenant partition with any live leader. The golden test's discriminating power for the leader leg is then a function of which broker happens to lead that one partition. Combined with the finding above, a run can exercise the error-0 path on only one of up to five leader-bearing brokers, and which one varies per run, making the canary's coverage nondeterministic and partially redundant with itself a
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Reliance on private aiokafka API fetch_all_metadata | consumer._client.fetch_all_metadata() reaches into aiokafka internals. Any aiokafka version bump in uv.lock can rename or remove _client, turning the canary red with an AttributeError that reads as a bastion failure, exactly the alert fatigue the workflow commentary warns against. | Evidence: metadata = await consumer._client.fetch_all_metadata() | Fix: Use the public describe_topics / partitions_for_topic consumer APIs, or pin and document the aiokafka
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Marker-carrying check matches docstrings and comments | test_the_chain_tests_exist_and_carry_their_markers verifies "pytest.mark.integration" via raw substring on the module source. A docstring mentioning the marker, or a move to pytestmark where integration is commented out, satisfies the check. The same applies to the def-name checks, which break on any formatting change (e.g., line wrapping in the signature) and produce a confusing 'was removed or renamed' failure. | Evidence: assert "pytest.mark.integra
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Transport failure of non-leader SNIs is indistinguishable from correct behavior in table output only | _leader_discriminating_error returns a string on transport failure. In the golden test the string fails the verdict correctly, but the misrouted assertion message assumes error-code semantics ('A broker answering 0 when it is not the leader'), which will not describe a timeout-shaped failure. Operators following the remediation text will regenerate the map for what may be a network problem on the runner. |
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MINOR] hostile-reviewer (glm-review)
Workflow guard asserts on raw substring, trivially satisfiable | test_canary_actually_invokes_both_chains joins all run blocks into one string and checks for selector substrings. A future edit that moves the selectors into a matrix variable, an env block, or a shell script invoked by name passes the guard while running nothing. Similarly, test_nightly_integration_sweep_excludes_the_bastion_tests greps for 'not bastion' anywhere in the file, including a comment, so the actual pytest selector could be removed
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Hardcoded bastion IP contradicts the file's own anti-staleness rationale | The module argues at length that hardcoded broker lists are 'the exact artifact that went stale and caused this outage,' then ships a hardcoded tailscale IP as the default for MSK_SNI_BASTION_HOST. A bastion readdress is invisible to every test here; all three legs would report identical connection failures that read as 'bastion down' rather than 'stale config,' and the resize detector's failure message would actively mislead. | Evid
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Adding the `bastion` pytest marker to pyproject.toml changed a component the runner image identity binds, so tests/ci/test_runner_image_identity.py went red. Regenerated with scripts/ci/runner_image_identity.py --mode generate. Identity v7 d2e218e9a49625aad7918e6592d1f11c.
… path (#3418) * test(OMN-15744): golden and error chain tests for the MSK SNI bastion path Nothing watched the lab -> SNI bastion -> MSK routing path, which is why a 2 -> 6 broker scale-out on 2026-08-06 left four of six brokers mis-routed for a month. Bootstrap and metadata are served by any broker, so every liveness surface stayed green; only leader-routed requests failed, with NotLeaderForPartition, which reads as transient noise. It was found by hand during an unrelated diagnosis. These tests replace that luck. Three tests in tests/integration/bastion: golden — every live broker answers AS ITSELF through the map. Discriminates on the broker's own error code: a leader-routed offsets request returns 0 from the partition leader and 6 from any other broker, so the answer reports the responder's identity. Carries its own positive control. error — an SNI the map does not carry must fail at connect, not silently reach a real broker. The original defect was precisely that an unknown name got a working session from the wrong broker. A mapped SNI must succeed in the same run, so a down bastion cannot read as a pass. resize detector — every broker the MSK control plane reports must be routable, with an unmapped name required to be unroutable so the claim is not vacuous. This is the test that turns red on the next resize. A TLS-only test was considered and rejected on measurement, not taste: all six broker SNIs present ONE wildcard certificate (sha256[:16] e4224daa249fdb90), so a handshake test cannot tell which broker answered and would have been green throughout the outage. "Produce to a topic led by each of the six brokers" was the first design and is not achievable: the sanctioned topic path governs replication factor only and nothing here can influence leader placement, so such a topic would land wherever the cluster chose. The error-code discriminator covers all six brokers deterministically instead, broker 4 included. Scheduled hourly by msk-bastion-canary.yml on the self-hosted omnibase-deploy runner, the only runner with reach to the bastion. Read-only by design: no produce and no topic creation against tenant data on a schedule. tests/ci/test_msk_bastion_canary_workflow.py guards the canary against being defanged — dropped schedule, wrong runner, continue-on-error, legs no longer invoked, or a produce creeping in. The bastion marker is excluded from the nightly sweep, which runs where the bastion is unreachable and would otherwise go permanently red. * chore(OMN-15744): regenerate runner image identity lock Adding the `bastion` pytest marker to pyproject.toml changed a component the runner image identity binds, so tests/ci/test_runner_image_identity.py went red. Regenerated with scripts/ci/runner_image_identity.py --mode generate. Identity v7 d2e218e9a49625aad7918e6592d1f11c.
Adds the golden and error chain tests for the lab to MSK SNI bastion to MSK path. Companion to OmniNode-ai/omninode_infra#1313, which fixed the map itself and put it under source control.
Ticket: OMN-15744. Requested by the operator on 2026-09-10, whose point was that this path had no mechanical proof at all.
Why nothing caught the original defect
MSK
omninode-dev-mskscaled 2 to 6 brokers on 2026-08-06. The bastion's map still described the 2-broker cluster and carrieddefault b1, so four of six brokers were mis-routed onto broker 1 for a month.The reason it survived is the design constraint for any test here: bootstrap and metadata are served by any broker, so every liveness surface stayed green. Only leader-routed requests failed, and they failed with NotLeaderForPartition, which reads as transient Kafka noise. Eight of 21 tenant topic-partitions were silently unwritable, including delegation-request, delegation-completed and delegation-failed. It was found by hand during an unrelated diagnosis.
The three tests
All in
tests/integration/bastion/test_msk_sni_bastion_chain.py.test_bastion_golden_every_broker_is_reachable_and_distinct— every live broker must answer as itself through the map. It discriminates on the broker's own error code: a leader-routed offsets request returns 0 from the partition leader and 6 from any other broker, so the answer reports the responder's identity. Emits a per-broker table as the evidence artifact and asserts its own positive control, so a run where every broker errored cannot pass on the not-the-leader rows alone.test_bastion_error_chain_unmapped_sni_fails_closed— an SNI the map does not carry must fail at connect. The original defect was exactly that an unknown name still got a working session from the wrong broker, which made a misconfiguration indistinguishable from health. A mapped SNI must succeed in the same run, so a bastion that is simply down cannot read as a green error chain.test_bastion_map_covers_every_live_broker— every broker the MSK control plane reports must be routable, with an unmapped name required to be unroutable so the claim is not vacuous. This is the one that turns red on the next resize, within one scheduled run.Two designs rejected, on measurement rather than taste
A TLS-only test. The cheap version is "handshake against each broker SNI". It is worthless here and I verified that rather than reasoning about it: all six broker SNIs present one wildcard certificate, sha256 prefix
e4224daa249fdb90, byte-identical across b-1 through b-6. A handshake test cannot tell which broker answered, so it would have been green for the entire month-long outage. Any test that cannot identify the responder does not test this defect."Produce one record to a topic led by each of the six brokers and read it back." This was the requested shape and it is not achievable, for two reasons established against the live system. No visible topic was led by broker 4 at census time. More fundamentally, the sanctioned topic-creation path governs replication factor only; nothing in this codebase can request or influence partition leader placement, so a topic created in the hope of landing on broker 4 would sit wherever the cluster chose and the test would be flaky by construction. The error-code discriminator covers all six brokers deterministically instead, with no topic creation. It is the stronger test, not a weaker substitute.
Broker 4 is genuinely covered. In the live run below the reference partition happened to be led by broker 4, so broker 4 was exercised as the leader returning 0 while the other five returned 6.
Live run
Executed against the live bastion and MSK cluster from a host with tailnet reach, 2026-09-10.
An earlier run of the golden test failed on a connect timeout, and that was my harness rather than the routing: the broker DNS names resolve to VPC-private addresses no external host can reach, while the deployed gateway pins them to the bastion with a dnsmasq sidecar. The test now pins resolution to the bastion for the duration of the block, which is what makes it measure the bastion's routing instead of the caller's DNS. The reasoning is recorded at the helper.
Scheduling
.github/workflows/msk-bastion-canary.yml, hourly at :17, on the self-hostedomnibase-deployrunner. That is the only runner with reach to the bastion, the same fate boundarychain-canary.ymlanddev-lane-liveness.ymlalready document. Offset from the chain canary's :41 so the two do not contend for the single runner. The job is read-only by design: no produce and no topic creation against tenant data on a schedule, unlike the chain canary which publishes a real delegation command.Expect the first scheduled runs to be red, and read the message before fixing it. The
omnibase-deployrunner carries no AWS identity today, and these tests need one. The workflow header names exactly what must be provisioned:kafka:ListClustersV2andkafka:ListNodesfor all three tests, plus an MSK data-plane identity for the golden one. The lab host already has a working data-plane identity in the gateway's IAM Roles Anywhere profile, so referencing it is configuration rather than a new credential, and minting a second identity would be the wrong move.I deliberately did not provision it. That is a credential action and belongs to whoever owns credentials, not to the lane that found a routing bug. Landing a knowingly-red canary that names its own blocker follows the precedent this repo set with
chain-canary.yml, and is the honest alternative to a silent skip that reports green over zero tests.Guard
tests/ci/test_msk_bastion_canary_workflow.pyruns on every PR and makes defanging the canary a red test rather than a review catch: a dropped schedule, a runner that cannot see the bastion,continue-on-error, a leg no longer invoked, a missing marker that would make the selection silently match nothing, a produce creeping into a scheduled cloud job, or the nightly exclusion being removed.The new
bastionmarker is excluded fromnightly-integration.yml, which runstests/integration/on a runner with neither AWS credentials nor bastion reach. Collecting them there would produce a permanent red that people learn to skim past, which is worse than not running them.Local verification:
ruffandmypy --strictclean on all new files, all 56 applicable pre-commit hooks pass, the 7 guard tests pass, and the 3 chain tests pass live as shown above.Evidence-Ticket: OMN-15744
Evidence-Source: OCC#9000