- Notifications
You must be signed in to change notification settings - Fork926
chore: refactor TestServer_X11 to use inproc networking#18564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:spike/testutil-in-proc-net
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
spikecurtis commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stackon Graphite.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
1e438a6 tob39371fCompareb60589d to212e2a5Compare| // It is intended mainly for testing; production code uses the default | ||
| // implementation backed by the operating system networking stack. | ||
| type X11Network interface { | ||
| Listen(network, address string) (net.Listener, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: It'd be nice to keep this as aListen(ctx, ...) as previously via ListenConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I decided against this because I wanted to reuseInProcNet, which needs to matchthis interface in Serpent. Code reuse beat out the presumably slight benefit of being able to bail out of a Listen call before the OS responds.
agent/agentssh/x11.go Outdated
| if originPort == 0 { | ||
| p := X11StartPort + session.display | ||
| if p > math.MaxUint32 { | ||
| panic("overflow") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we should panic here as that risks killing the whole agent in the unlikely event that we don't have aTCPConn. Logging and closing the connection seems more appropriate especially considering the comment above "or non-TCP connections" suggesting it's not only for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On closer inspection,0 is a perfectly acceptable value from the perspective of the protocol, and will only occur in testing. No need to panic.
212e2a5 toa272b09Compareb39371f to319c17dCompare
Uh oh!
There was an error while loading.Please reload this page.
relates to#18263
Refactors the x11Forwarder to accept a networking
interfacethat we can fake out for testing. This isolates the unit tests from other processes listening in the port range used by X11 forwarding. This will become extremely important in up-stack PRs where we listen on every port in the range and need to control which ports have conflicts.