fix: eliminate parallel Rust test flakes#2434
Conversation
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Label |
|
|
||
| /// Parse `/proc/<pid>/net/tcp` (and `/proc/<pid>/net/tcp6`) to find the socket | ||
| /// inode for a given local port. | ||
| /// inode for a complete local-to-remote TCP tuple. |
There was a problem hiding this comment.
Question: Should the (pid, peer, proxy) tuple be a concrete type?
There was a problem hiding this comment.
I think a small concrete type helps here. I pushed a proposal branch with the change split out so it is easier to inspect: https://github.com/NVIDIA/OpenShell/tree/codex/main-refactor-then-pr2434
Shape:
509e0942introducesWorkloadProxyTcpConnection::new(workload, proxy)plusDisplayfor theworkload -> proxyordering.ef20b59creapplies this PR’s fix on top, preserving Piotr as the author, and uses the named connection for the full tuple lookup.
This keeps the ordering explicit at the proxy call sites (workload_addr, proxy_addr) while making the procfs lookup signatures harder to mix up.
There was a problem hiding this comment.
This is great, let me pull your change into this PR.
There was a problem hiding this comment.
Applied your change, thanks!
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Summary
Eliminate three recurring parallel Rust unit-test failure modes seen in Actions run 29951858582 and nearby runs. The changes make listener and tracing assertions deterministic and harden Linux proxy process-identity resolution against unrelated connections that reuse the same ephemeral port.
Related Issue
N/A — follow-up to the linked CI investigation.
Changes
/proc/net/tcp{,6}.WorkloadProxyTcpConnectionand useProcTcpAddressFamily::{Ipv4, Ipv6}instead of a boolean when selecting and decoding procfs tables.Failure details
SSH forward-listener failure
Failure:
wait_for_forward_listener_rejects_missing_listenerasked the OS for an ephemeral port, closed that listener, and then expected a connection to the released port to fail. Under parallel test load, another process could claim the port between the close and the probe, so the test's premise was not stable.Fix: The listener-wait loop now accepts an internal probe function. Production still uses the real TCP probe, while the renamed failure-path test injects a deterministic
connection refusedresult and verifies that the diagnostic retains the underlying probe error. The existing real-listener success test continues to cover the integration path.Tracing capture failures
Failures:
evaluation_log_emits_structured_log_annotationsandrequest_id_appears_in_trace_spanintermittently observed an empty capture buffer. A tracing callsite can be registered by another parallel test while no interested subscriber is active, leaving its process-wide interest cache disabled even after these tests install a thread-local subscriber.Fix: Each test installs its capture subscriber for the narrow emission scope and rebuilds tracing's callsite interest cache inside that scope before emitting the event or span. This keeps the tests isolated while ensuring the callsite is enabled for the subscriber that performs the assertion.
Linux proxy process-identity failure
How identity resolution works
Linux does not attach the originating client PID to the proxy's accepted TCP socket. OpenShell therefore resolves it indirectly:
/proc/<entrypoint-pid>/net/tcpandtcp6. These files expose the active TCP table for the entrypoint's network namespace, not only sockets owned by that PID./proc/<pid>/fdfor every process holdingsocket:[inode].What failed
resolve_process_identity_hashes_live_exe_after_hot_swapsometimes resolved the test harness PID instead of the child process on a busy runner. The old/proc/net/tcp{,6}lookup matched only the workload peer's ephemeral port and returned the firstESTABLISHEDrow with that port.A port alone does not identify a TCP connection. For example, these are distinct connections even though both use local port
50000:If the old lookup selected inode
222, the later inode-to-PID scan worked as designed but resolved the wrong connection. The result waswrong table row -> wrong inode -> wrong PID -> wrong binary identity; Linux itself did not return an incorrect PID.The hot-swap setup was incidental. That test creates a real Bash child with a real TCP connection so it can verify live executable hashing after the on-disk binary is replaced. Parallel CI placed many unrelated sockets in the same host network namespace, making the under-specified lookup much easier to expose.
Production relevance
This is a real production correctness issue, although it is less likely in an isolated sandbox network namespace than in the shared CI namespace. It can occur whenever another active connection in the same namespace has the same numeric local port but differs by local address, remote address, remote port, or address family.
The most likely production effect was a false denial or incorrect audit attribution because the request could be evaluated as the wrong binary. Because binary identity participates in authorization, using an unrelated allowed binary's identity could theoretically produce a false allow as well. This PR does not claim a demonstrated production bypass, but the old port-only association was too weak for a security-sensitive policy boundary.
Fix
Identity resolution now wraps both endpoints obtained from the accepted socket in a
WorkloadProxyTcpConnectionand passes that concrete type through the CONNECT and forward-proxy paths. Naming theworkloadandproxyendpoints makes their ordering explicit at construction and prevents adjacentSocketAddrarguments from being accidentally reversed.The procfs lookup matches the complete workload-to-proxy tuple:
It checks both IPv4 and IPv6 tables and normalizes IPv4-mapped IPv6 addresses before comparison. Table selection and address decoding use the explicit
ProcTcpAddressFamily::{Ipv4, Ipv6}enum rather than anipv6boolean, so the family is readable and type-checked throughout the parser.A synthetic regression test places two entries on the same local port and verifies that the remote endpoint selects the correct inode; the live hot-swap and shared-owner tests now also use the named full tuple.
Compatibility and failure behavior
Valid connections in OpenShell's supported proxy topologies should continue to resolve. The workload receives an explicit proxy URL and connects directly either to the host-side veth address (
10.200.0.1:<port>) or to the sidecar/loopback proxy. There is no endpoint translation between the client socket recorded in/procand the accepted proxy socket, so the two views should contain the same reversed endpoints. IPv4-mapped IPv6 is handled explicitly.The intentional behavior change is that a lookup which previously "succeeded" only because it found an unrelated same-port socket will no longer resolve that socket. If no exact tuple exists, identity lookup fails and policy evaluation denies the request rather than falling back to the unsafe port-only match.
A custom or future topology that transparently NATs or rewrites either endpoint between the workload and proxy could therefore fail closed until the resolver is taught how to correlate that topology. OpenShell's current veth, loopback, and sidecar paths do not perform such rewriting, so this is not expected to regress supported deployments. The additional address parsing is negligible compared with the existing
/procscan and executable hashing.Testing
mise run pre-commitpassesAdditional stress coverage:
Checklist