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

Fix local DNS resolution issue #59

Closed
wants to merge 1 commit into from
Closed

Fix local DNS resolution issue #59

wants to merge 1 commit into from

Conversation

fenichelar
Copy link

  1. Always try local DNS before Cloudflare
  2. If DNS servers are configured manually and via DHCP, only use manually configured DNS servers not both

Note, this does not remove Cloudflare DNS server fallback.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

  1. If DNS servers are configured manually and via DHCP, only use manually configured DNS servers not both

Fallback should be used. The possibility of disabling fallback can be added using within these scopes:

#56 (review)

The solution in this PR does not match that. You already knew that, as that previous PR is yours as well... yet you choose to ignore it?

@Zixim

This comment has been minimized.

@fenichelar
Copy link
Author

fenichelar commented Sep 28, 2021

@frenck Can you please elaborate? Fallback is used:

fallback REFUSED,SERVFAIL,NXDOMAIN . dns://127.0.0.1:5553
I think there is some confusion on how coredns works.

The solution in this PR does not match that. You already knew that, as that previous PR is yours as well... yet you choose to ignore it?

This is a different issue.

@fenichelar
Copy link
Author

This PR has nothing to do with failover. Right now, if DNS servers are both manually configured and provided via DHCP, then both the manually configured and DHCP provided servers are used for load balancing. The manually configured DNS servers should overwrite the DHCP provided servers.

@frenck
Copy link
Member

frenck commented Sep 28, 2021

The manually configured DNS servers should overwrite the DHCP provided servers.

I understand that is what you are trying to achieve, yet, we consider the DHCP servers a possible fallback/route as well, which is removed when servers have been manually set up.

So, for the OS-based installs, resolving anything is considered more important than anything else.

@alexdelprete

This comment has been minimized.

@fenichelar
Copy link
Author

@frenck

So, for the OS-based installs, resolving anything is considered more important than anything else.

The current implementation doesn't resolve local dns names....

@Zixim

This comment has been minimized.

@frenck
Copy link
Member

frenck commented Sep 28, 2021

The current implementation doesn't resolve local dns names...

It does actually; for example, it gets mDNS local domains as known here: https://github.com/home-assistant/plugin-dns/tree/master/plugins/mdns; main reason for this is to work around issues with multicasting and Docker.

As for my personal setup, I used even split DNS with custom local domains in the current method.

Pascal said that even if those things are implemented, the system would be considered unsupported, can you confirm? If so, why?

Because customizations on a managed systems are simply not supported by the core team.

@Zixim

This comment has been minimized.

@jjvandenberg

This comment has been minimized.

@alexdelprete

This comment has been minimized.

@alexdelprete

This comment has been minimized.

@alexdelprete

This comment has been minimized.

@frenck
Copy link
Member

frenck commented Sep 28, 2021

You said you were willing to discuss anything

This is not a generic discussion but a PR review. Your comments are general architectural comments and opinions on decisions made in the past for good reasons and are not reflecting the actual code being reviewed here. Please use or community forums or Discord chat if you'd like to discuss these types of things with others.

Thanks.

@fenichelar
Copy link
Author

@frenck do you want to discuss via email or something? I'm looking for directions on how to move forward

@frenck
Copy link
Member

frenck commented Sep 28, 2021

@frenck do you want to discuss via email or something? I'm looking for directions on how to move forward

No? The direction to move forward has been pointed out already. I'm not sure what else you want from me at this moment. The project is willing to accept the capability of disabling fallback methods within the parameters pointed out already.

@fenichelar
Copy link
Author

I'm not sure what else you want from me at this moment.

I asked a few follow up question and never got a response...

@frenck
Copy link
Member

frenck commented Sep 28, 2021

I asked a few follow up question and never got a response...

Sorry, maybe I missed something because of all the side spamming in this review. Could you point me out the question?

@alexdelprete

This comment has been minimized.

@fenichelar
Copy link
Author

That will not solve the main issue which we solved with this. If you want this then you need to do the following:

What issue?

  1. Is the concern that no DNS servers will be configured (either via DHCP or manually)?
  2. Or is the concern that DNS servers will be configured (either via DHCP or manually) but won't respond to DNS queries Maybe due to DNS server failure, incorrect DNS server IPs being configured, firewall rules, etc.
  3. Or is the concern that DNS servers will be configured (either via DHCP or manually) but won't forward queries and will return NXDOMAIN for all external domains?
  4. Something else?

Open a PR to Supervisor and add an option to disable fallback which marks your system as unsupported and extend it to the API + tests

  1. What should be config flag be called?
  2. What should its type be?

Create an PR to the developer documentation for having it there

Okay

Create an PR to CLI repository for using that options

Okay

Create an PR to this repository to get this options in place

How should this work? My goal would be to find a way to make HA use the DNS server provided via DHCP and nothing else.

@fenichelar
Copy link
Author

@frenck Would it make more sense to just have a way to disable plugin-dns and have HA core use DHCP provided or manually configured DNS servers directly?

@frenck
Copy link
Member

frenck commented Sep 28, 2021

What issue?

Multiple! The OS is aimed at the average Joe running this at home, with limited knowledge. We have had to deal with many many many different issues and network configurations (and misconfiguration) causing an absolute flood of issues. This has slowly (over time) resulted in what you see here.

Most of the issues currently still there, are often the result of customizations or advanced network setup. This is fine, however, that is not the target audience. As a matter of fact, the number of DNS / name resolving issues hasn't been as low as it is right now.

Broken local DNS, wrongly set up split DNS, misconfigured ad blockers, DNS loops, DNS of provides down causing issues, misconfigured Docker setups or virtual machines, wrongly configured firewalls, and many more.

What should be config flag be called?
What should its type be?

A feature flag? I would say a boolean? Name... I dunno, something descriptive, however, that is just a name. Make it something descriptive, in the review of that specific PR that can be discussed (and adjustments to a name can be made quickly in general).

How should this work? My goal would be to find a way to make HA use the DNS server provided via DHCP and nothing else.

Add a flag to the supervisor and describe, pass the flag to the configuration of the DNS plugin so it can use it during the configuration generation.

However, that is a bit out of scope for this PR as described I would say.

Would it make more sense to just have a way to disable plugin-dns and have HA core use DHCP provided or manually configured DNS servers directly?

I don't think so, that will cause a different world of issues, as multiple parts of the system will assume it is there.

@fenichelar
Copy link
Author

Broken local DNS, wrongly set up split DNS, misconfigured ad blockers, DNS loops, DNS of provides down causing issues, misconfigured Docker setups or virtual machines, wrongly configured firewalls, and many more.

Cloudflare DNS is currently setup as both a forwarding DNS server and a failover DNS server. I don't understand why this was done. It is hard to discuss a PR allowing this behavior to be changed when the reason for the behavior is unclear. Do you know why Cloudflare DNS was setup as both a forwarding DNS server and a failover DNS server?

Add a flag to the supervisor and describe, pass the flag to the configuration of the DNS plugin so it can use it during the configuration generation.

But what configuration would this result in?

  1. Would it forward only to manually configured DNS servers (if configured) and DHCP provided DNS servers otherwise?
  2. Would it forward to both manually configured and DHCP provided DNS servers?
  3. What about the Docker DNS server?
  4. Something else?

However, that is a bit out of scope for this PR as described I would say.

How do I discuss a PR I am working on then?

@frenck
Copy link
Member

frenck commented Sep 28, 2021

Cloudflare DNS is currently setup as both a forwarding DNS server and a failover DNS server. I don't understand why this was done. It is hard to discuss a PR allowing this behavior to be changed when the reason for the behavior is unclear

Resolving anything is more important than solving nothing. The reason I gave in my last answer.

But what configuration would this result in?

Disabling any fallback I would say? That is the goal you are trying to achieve, right?

How do I discuss a PR I am working on then?

This PR is titled: "Fix local DNS resolution issue", you are now discussing adding a supervisor feature flags for disabling fallback. I'm sorry, but that feels offtopic and not related to the content written in this PR (nor can this PR be modified to achieve that even remotely).

@fenichelar
Copy link
Author

(nor can this PR be modified to achieve that even remotely).

The first PR you closed could have... but it has been locked so I can't ask for feedback now. Read the discussion in the PR, I actually asked about a feature flag before you even reviewed it. There was a lot of off topic discussion, but not from me. I have just been trying to get feedback on my PR.

I give up :( nevermind

@fenichelar fenichelar closed this Sep 28, 2021
@alexdelprete

This comment has been minimized.

@frenck
Copy link
Member

frenck commented Sep 28, 2021

The first PR you closed could have

Sorry it did not, the feature flag needs to be added to the supervisor as described before. None of the PRs I closed added a flag to the supervisor.

There was a lot of off topic discussion, but not from me.

Yeah I know, sorry about that, its really distracting :(

I give up :( nevermind

:( If you change your mind and are up for a shot, let me know

@frenck
Copy link
Member

frenck commented Sep 28, 2021

@alexdelprete This formal and final warning. That the last comment is unneeded and condescending (and not the first) another one will result in a ban.

@fenichelar
Copy link
Author

Sorry it did not, the feature flag needs to be added to the supervisor as described before. None of the PRs I closed added a flag to the supervisor.

Supervisor just contains the flag, not the logic for what the flag does. I would assume it makes more sense to discuss what the flag does in the plugin-dns repo than the supervisor repo. We are essentially talking about wrapping my existing PR behind a flag, all of the code is still applicable, it would just be wrapped in an if statement.

:( If you change your mind and are up for a shot, let me know

If you are willing to work with me, I would like to tackle this. But I am not going to start working on the PR for supervisor to add a flag until after we figure out what the flag actually does. I don't know about you, but I write code THEN documentation, not the other way around.

I will submit a new PR to plugin-dns for us to discuss shortly. The change will be behind a feature flag that doesn't yet exist. I will submit PRs to add the feature flag after.

@frenck
Copy link
Member

frenck commented Sep 28, 2021

I don't know about you, but I write code THEN documentation, not the other way around.

This is not about adding documentation, this is about adding the controls. If there aren't controls and flags created in the supervisor, there will be nothing to test or control to implement.

As for documentation that will be required before merging the final PRs 😉

I will submit a new PR to plugin-dns for us to discuss shortly.

If you do, please do that in conjunction with the matching supervisor and CLI PRs that go with it.

@fenichelar
Copy link
Author

@frenck

If you do, please do that in conjunction with the matching supervisor and CLI PRs that go with it.

So you are saying you won't discuss the change here until after I write the PR for supervisor and CLI? So if you provide feedback on this PR that requires a change then I will have to rewrite all 3 PRs... doesn't it make more sense to review the DNS change and confirm it is good before implementing the flag?

@frenck
Copy link
Member

frenck commented Sep 28, 2021

The dns part is the smallest part of the implementation and a fairly simple of statement handling the state of the flag (thus barely interesting or discussable imho) The other parts require way more implementation, code and tests.

@fenichelar
Copy link
Author

The dns part is the smallest part of the implementation and a fairly simple of statement handling the state of the flag (thus barely interesting or discuss Anne imho) The other parts require way more implementation, code and tests.

But I don't know what change to make in supervisor until we discuss this. There is a high probability that using more than one feature flag is the best way forward. Or maybe you don't want to use multiple flags but instead use a string config option. I think you will understand once I submit a PR.

@frenck
Copy link
Member

frenck commented Sep 28, 2021

I don't think it's that difficult: use dns fallback or not. If the fallback is disabled only use the configured DNS servers... 🤷‍♂️ we don't need multiple flag for that.

@fenichelar
Copy link
Author

I don't think it's that difficult: use dns fallback or not. If the fallback is disabled only use the configured DNS servers... 🤷‍♂️ we don't need multiple flash for that.

How should the following scenarios be handled?

DNS servers have been provided both manually and via DHCP.

  1. Use manually provided DNS servers only
  2. Use DHCP provided DNS servers only
  3. Use both manually provided DNS servers and DHCP provided DNS servers in the forward line (what order?)
  4. Use manually provided DNS servers in the forward line and DHCP in the fallback line
  5. Use DHCP provided DNS servers in the forward line and manually provided in the fallback line

And this is assuming we aren't including the Docker DNS server anywhere. Should the Docker DNS server be used?

@fenichelar
Copy link
Author

Maybe instead of a single failover feature flag, having a dns_mode flag makes more sense. With the following options:

automatic - current behavior
DHCP - use DHCP provided DNS servers only
manual - use manually provided DNS servers only
docker - use Docker DNS server only

@frenck
Copy link
Member

frenck commented Sep 28, 2021

I really don't think we should handle those cases at all from a flags perspective.

If the single flag is turning off fallback: In case manual server are provided use those, else use DHCP provided one.

@fenichelar
Copy link
Author

If the single flag is turning off fallback: In case manual server are provided use those, else use DHCP provided one.

Ok. What about Docker DNS server? Only use Docker if no servers are provided manually or via DHCP?

@frenck
Copy link
Member

frenck commented Sep 28, 2021

Docker dns has no part in this, nor does it need to be. It will match the already available info, as the supervisor manages the network settings.

@fenichelar
Copy link
Author

@frenck The case I am trying to handle is if no servers are provided manually and no servers are provided via DHCP? Should the forward line just contain dns://127.0.0.11?

@itsdeltalol
Copy link

What issue?

Multiple! The OS is aimed at the average Joe running this at home, with limited knowledge. We have had to deal with many many many different issues and network configurations (and misconfiguration) causing an absolute flood of issues. This has slowly (over time) resulted in what you see here.

Hey, this describes me pretty well, I have some really limited knowledge, probably don't even belong in this discussion, but I'm having trouble getting Home Assistant to recognize the .local hosts that raspberry pi custom builds tend to set up. I have like three of them and I'm really going to hate to have to re-do each card when that device powers up and gets a new IP. It'd be really good if there was a thing to handle that.

@Zixim
Copy link

Zixim commented Nov 27, 2021

"a thing to handle that".. well that would be DNS...except HAOS devs have decided to break it, for "reasons".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants