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

Add MDNS and LLMNR status to API #3545

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Add MDNS and LLMNR status to API #3545

merged 3 commits into from
Apr 11, 2022

Conversation

mdegat01
Copy link
Contributor

@mdegat01 mdegat01 commented Apr 6, 2022

Proposed change

Add MDNS and LLMNR status to /dns/info. Returns true if MDNS and LLMNR are enabled in systemd-resolved, returns false if they are disabled or systemd-resolved is unreachable (as then the DNS plugin will not be able to resolve these names).

Also added llmnr_hostname to /host/info. Usually this will be identical to hostname. But it is possible for it to be different if there was a conflict on the network (like say if there was another machine already named homeassistant). Figured it might help someone debug a tricky situation.

Lastly added resolved to host features in /host/info. I missed this when I added resolved, I feel like it should be there.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:
  • Link to cli pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints of add-on configuration are added/changed:

@mdegat01 mdegat01 added the new-feature A new feature label Apr 6, 2022
@mdegat01 mdegat01 requested a review from pvizeli April 6, 2022 00:10
@mdegat01
Copy link
Contributor Author

mdegat01 commented Apr 6, 2022

@pvizeli I should note that when going through this I realized something. I cannot add mdns and llmnr to /dns/options currently because the DBus API of org.freedesktop.resolve1 doesn't allow you to change the global default for mdns and llmnr, which is what we're returning here. It only allows you to change the mdns and llmnr setting per network interface.

Which means it is possible for this to return true but llmnr and/or mdns still not to work because it has been turned off for the active interface. I imagine this is pretty unlikely but technically possible. Do you think we need to account for this possibility? If so maybe I can move these attributes to /host/info to reflect that they're global settings and then map the info from the current active interface into /dns/info.

If so I'll do the second part in a separate PR though since I'd have to explore org.freedesktop.resolve1.Link and see how the info in NetworkManager matches up (since org.freedesktop.resolve1 just has a GetLink method, not a ListLinks method). Plus then i would want to add options to change these values for the active interface.

@agners
Copy link
Member

agners commented Apr 7, 2022

Which means it is possible for this to return true but llmnr and/or mdns still not to work because it has been turned off for the active interface. I imagine this is pretty unlikely but technically possible. Do you think we need to account for this possibility? If so maybe I can move these attributes to /host/info to reflect that they're global settings and then map the info from the current active interface into /dns/info.

While it is nice, I think I'd rather prefer to have it not at all then it to be "unreliable". IMHO, it should reflect the current active network profiles setting and be in the dns options.

Exposing the global setting in /host/info, doesn't seem to be worth it.

supervisor/plugins/dns.py Show resolved Hide resolved
supervisor/const.py Outdated Show resolved Hide resolved
@mdegat01 mdegat01 requested a review from pvizeli April 7, 2022 16:05
Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Let's start with that and looking how it goes

@ludeeus
Copy link
Member

ludeeus commented Apr 7, 2022

Remember to create a dev documentation PR when the API changes 👍

Copy link
Member

@agners agners left a comment

Choose a reason for hiding this comment

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

LGTM.

@pvizeli pvizeli added the missing-documentation Added to pull requests that needs a docs, but none is linked label Apr 7, 2022
@pvizeli pvizeli merged commit 12da8a0 into main Apr 11, 2022
@pvizeli pvizeli deleted the llmnr-mdns-api branch April 11, 2022 10:08
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed missing-documentation Added to pull requests that needs a docs, but none is linked new-feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants