hassio-addons / addon-adguard-home

AdGuard Home - Home Assistant Community Add-ons
https://addons.community
MIT License
378 stars 47 forks source link

Allow binding to localhost (#371) #382

Closed kevynb closed 1 year ago

kevynb commented 1 year ago

Proposed Changes

Allow adguard to be bound to localhost in addition to the public and docker ip addresses. I did not enable this by defaut in case there is already a resolver enabled on the machine. I created an additional option instead.

My use case is the same as in #371, I want to be able to use Adguard home within my Tailscale network but this could probably also be used with a Zerotier config or any other container that wants to make use of an internal DNS resolver.

I tested this and can confirm that given that this addon and other networking ones use host mode, binding to localhost is enough to achieve our goals.

Startup log: image

Resolving from another machine (using tailscale) image

Note: I created this PR because I had a need for myself and saw that I was not the only one. That's why I didn't wait for a confirmation. If the community doesn't want this, feel free to close my MR.

Related Issues

371

kevynb commented 1 year ago

Hey @frenck, thanks for the quick reply!

I've got a repository with a copy of my changes to try it on a HA instance at https://github.com/kevynb/addons README.md is not up to date but the adguard addon is there.

frenck commented 1 year ago

with an Hassio instance.

Hassio isn't a thing. The use of that terminology was ditched years ago, as it is ambiguous. Could you specific installation types and properties instead?

It's optional because some distributions like ubuntu

Ubuntu isn't a supported OS and, thus, not a scope we have to consider.

kevynb commented 1 year ago

My bad, here is my system: image

Installing Home Assistant using docker or a docker-compose is supported on any platform that supports Docker. Since it is recommended to use host mode, there would be conflicts on any system with a local resolver. I can update the PR to turn it on by default but I was taking a cautious approach to avoid creating a problem for existing users.

frenck commented 1 year ago

Installing Home Assistant using docker or a docker-compose is supported on any platform that supports Docker. Since it is recommended to use host mode, there would be conflicts on any system with a local resolver.

Those won't have access to add-ons, so not relevant to consider when building add-ons.

I can update the PR to turn it on by default but I was taking a cautious approach to avoid creating a problem for existing users.

I think it might make sense to drop the option maybe, yes. It is easier to add an option than to drop one. Adding options also lead to more confusion in general.

Hence my questions, as I was thinking about how much of a risk or issues it would be to enable it by default.

kevynb commented 1 year ago

Ok, that make sense. Thanks for the feedback. PR updated, now binding to localhost by default.