Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ts-proxyd utility to support running multiple Tailscale proxy servers concurrently. It introduces a modular configuration discovery system through the internal/configsources package, allowing servers to be defined via command-line flags or environment variables using the TSPROXY_<NAME>_<OPTION> pattern. Key additions include a parallel execution model using goroutines, a dry-run mode for configuration validation, and automated defaulting for ports and state directories. Review feedback identifies critical issues regarding potential state directory conflicts and port collisions when running multiple servers, as well as a regression where the mandatory check for a listen address was omitted for non-HTTP configurations.
| if c.StateDir == "" && baseStateDir != "" { | ||
| c.StateDir = filepath.Join(baseStateDir, c.Name) | ||
| } |
There was a problem hiding this comment.
When multiple servers are configured, they must each have a unique state directory to avoid conflicting on Tailscale node state. If baseStateDir is not provided and individual servers don't specify a StateDir, they will all default to an empty string, causing them to use the same default Tailscale state path. It is recommended to enforce a base state directory or ensure uniqueness when multiple servers are discovered.
| } | ||
|
|
||
| func runServer(cfg configsources.ServerConfig) { | ||
| opts := tsproxy.TailscaleProxyServerOptions{ |
There was a problem hiding this comment.
The original implementation ensured that a listen address was always provided. In this refactored version, if EnableHTTP is false and no listen address is specified via flags or environment variables, the server will attempt to start with an empty Listen field, which will likely fail or lead to non-deterministic behavior. Consider adding an explicit check to ensure Listen is not empty before creating the server.
| opts := tsproxy.TailscaleProxyServerOptions{ | |
| if cfg.Listen == "" { | |
| slog.Error("failed to start server: listen address not defined", "name", cfg.Name) | |
| return | |
| } | |
| opts := tsproxy.TailscaleProxyServerOptions{ |
| if c.Listen == "" && c.EnableHTTP { | ||
| if c.EnableFunnel || c.EnableTLS { | ||
| c.Listen = ":443" | ||
| } else { | ||
| c.Listen = ":80" | ||
| } | ||
| } |
There was a problem hiding this comment.
This defaulting logic can lead to port collisions if multiple servers are configured with EnableHTTP set to true but without explicit listen addresses. Both will attempt to bind to :80 (or :443), causing one of them to fail at startup. Consider validating that listen addresses are unique across all discovered configurations in main or providing a warning.
1 participant
No description provided.