Skip to content

WIP: No publishing ports#254

Open
castorinop wants to merge 2 commits intoalmeidapaulopt:mainfrom
castorinop:no-publishing-ports
Open

WIP: No publishing ports#254
castorinop wants to merge 2 commits intoalmeidapaulopt:mainfrom
castorinop:no-publishing-ports

Conversation

@castorinop
Copy link
Copy Markdown

work well running on host network mode.

avoid need export ports.

voanhduy1512 and others added 2 commits April 6, 2025 02:44
When tsdproxy can talk to internal ports of the container, there is no
need to publish the port to host machine.
Copy link
Copy Markdown
Owner

@almeidapaulopt almeidapaulopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution, @castorinop! Supporting host network mode is definitely a valuable feature.\n\nI noticed this PR is marked as WIP in the title. Here are my observations from the review:\n\n### What it does:\n- Uses dcontainer.Config.ExposedPorts (container-level) in addition to HostConfig.PortBindings (host-level) for port detection\n- This allows containers in host network mode to work without published port bindings\n\n### Concerns:\n1. Removed early error check — The original code had if internalPort == "" && publishedPort == "" { return error } which was an early guard. Removing this means errors might surface later in less clear ways.\n2. Changed auto-detect conditionif c.autodetectif c.autodetect || internalPort != "" — this means autodetect runs for ALL containers with an internal port, even when autodetect is disabled. Is that intentional?\n3. Changed fallback conditionif publishedPort == ""if internalPort == "" || publishedPort == "" — this changes the fallback logic significantly.\n\nSince this is WIP, would you like to continue working on it? I think the approach is sound but the edge cases need more careful handling. Happy to provide more detailed feedback once you're ready! 🙂

@almeidapaulopt almeidapaulopt force-pushed the main branch 2 times, most recently from b720a3f to 28506e0 Compare May 7, 2026 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants