Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support containers running --net=host (#1537) #2222

Merged
merged 6 commits into from Jun 7, 2023

Conversation

rot169
Copy link
Contributor

@rot169 rot169 commented Apr 19, 2023

Containers which run host networking do not have their own IP address, which means the template doesn't know what to use as the upstream server. This change detects if a target container is running host networking, and if so, uses the IP address of the bridge net gateway as the upstream target.

Fixes #1270
Fixes #832

@buchdag

This comment was marked as outdated.

@rot169

This comment was marked as outdated.

Detect if a target container is running host networking, and if so, use the IP address of the first bridge net gateway.
@rot169 rot169 force-pushed the support-hostnet-containers branch from 71c8de5 to 325fd01 Compare April 23, 2023 13:14
@buchdag

This comment was marked as outdated.

@buchdag

This comment was marked as outdated.

@rot169

This comment was marked as outdated.

@buchdag
Copy link
Member

buchdag commented Apr 28, 2023

In the initial issue (#1537), the issue creator also run the proxy in host network mode networking.

I don't know if running the proxy in bridge network mode and the proxied containers in host network mode is a common use case in the issues related to this. I think that people are most likely to run everything in host network mode rather than just a few containers but I might be wrong.

@rot169
Copy link
Contributor Author

rot169 commented Apr 28, 2023

Not sure if they particularlly wanted to run the proxy in host mode, or if it was just a way of trying to get it working with homeassistant (which has to run in a host mode container if you want it to do a load of network discovery). This is my exact use case too - I want to proxy to several containers running in a 'normal' bridge mode, and also to homeassistant running in host mode. I feel like most folks would want to keep bridge networking wherever possible, as it's part of the segregation benefits of containerisation, but I have no evidence to back up this assertion!

@buchdag
Copy link
Member

buchdag commented Apr 28, 2023

That does work with the proxy in bridge network mode and the container in host network mode.

It requires VIRTUAL_PORT and works in a very specific way so I think it needs to be documented.

Ideally I would also like it to be tested.

@buchdag buchdag added status/pr-needs-tests This PR needs new or additional test(s) status/pr-needs-docs This PR needs new or additional documentation type/fix PR for a bug fix labels Apr 28, 2023
@buchdag buchdag linked an issue Apr 28, 2023 that may be closed by this pull request
@buchdag buchdag removed the status/pr-needs-tests This PR needs new or additional test(s) label Apr 28, 2023
@buchdag
Copy link
Member

buchdag commented Apr 28, 2023

I added a minimal test.

@rhansen I'd like your advice on this, should we work a bit more on this feature (support proxy + container in host network mode, warn if (index $.globals.CurrentContainer.Networks 0).Gateway is empty because the network is private, iterate over $.globals.CurrentContainer.Networks, etc) or do you think the fix could be released in its current state with documented limitations ?

@rhansen
Copy link
Collaborator

rhansen commented Apr 30, 2023

I'll take a look when I have some time next week.

I'm OK with marking this as experimental while we continue to evaluate.

@buchdag buchdag linked an issue Apr 30, 2023 that may be closed by this pull request
@buchdag buchdag force-pushed the support-hostnet-containers branch 2 times, most recently from 604d6ce to 15c738f Compare May 8, 2023 22:41
@buchdag buchdag force-pushed the support-hostnet-containers branch from 15c738f to b88d33d Compare May 8, 2023 22:58
@buchdag
Copy link
Member

buchdag commented May 9, 2023

@rot169 just to be sure: for a given container, host network mode and the other network modes are all mutually exclusive ?

@buchdag buchdag removed the status/pr-needs-docs This PR needs new or additional documentation label May 9, 2023
@buchdag
Copy link
Member

buchdag commented May 9, 2023

I added the following to the PR:

  • iteration over the proxy networks to find a suitable one rather than picking the first
  • compatibility with the proxy running in host network mode itself
  • tests for base PR scenario + proxy in host network mode
  • documentation

@rhansen I'd still like your review on this, the template code might be sub optimal. Other than that I'm okay with the PR in its current state.

@rot169 could you test the PR in its current state ?

buchdag
buchdag previously approved these changes May 9, 2023
@buchdag
Copy link
Member

buchdag commented May 10, 2023

@rot169
Copy link
Contributor Author

rot169 commented May 10, 2023

@rot169 just to be sure: for a given container, host network mode and the other network modes are all mutually exclusive ?

That's correct; as far as I understand it is not possible to have a container with both host-based network and a bridge network.

@rot169 could you test the PR in its current state ?

Sure, will probably take me a few days, but will do so and report back.

@buchdag
Copy link
Member

buchdag commented May 16, 2023

I'm tempted to use host.docker.internal / host.containers.internal like Traefik does (with the current method as a fall back) because that seems more straightforward and robust but that would require installing a DNS resolution util in the image to check if they actually resolve, and the lightest I could find (bind9-host) adds ~40MB to the image.

@prashker
Copy link

prashker commented May 25, 2023

For my workaround with Home Assistant container being host networked, I used qoomon/docker-host. The same solution is what I suspect people are using to make nginx-proxy handle non-dockerized applications. I don't like the approach, but it has worked for me.

The proposed pull req commits looks great, and while I figure out how to build my own image based off this pull req, I will give a friendly ping to @rot169 if they've done their testing yet.

@rot169
Copy link
Contributor Author

rot169 commented May 25, 2023

Thanks for the ping - and apologies this'd dropped off my radar. Yes, the latest code seems to work great for me, and is clearly more robust than my initial proposal.

@buchdag
Copy link
Member

buchdag commented May 30, 2023

I spotted a typo in the doc I added, I'll fix this then probably merge this PR by the end of the week.

@buchdag buchdag merged commit c1a2b31 into nginx-proxy:main Jun 7, 2023
2 checks passed
@prashker
Copy link

prashker commented Jun 7, 2023

Nice! Looks like a little oops in Markdown

https://github.com/nginx-proxy/nginx-proxy#host-networking

@buchdag
Copy link
Member

buchdag commented Jun 7, 2023

Fixed, thank you @prashker

@prashker
Copy link

Thank you - sorry for the LONG delay. I can confirm this functionality works.

My --net=host Home Assistant instance has VIRTUAL_HOST LETSENCRYPT_HOST and VIRTUAL_PORT assigned and functioning as expected.

Good riddance to my workaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix PR for a bug fix
Projects
None yet
4 participants