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

Added an option to disable mDNS #1716

Merged
merged 6 commits into from May 6, 2021
Merged

Added an option to disable mDNS #1716

merged 6 commits into from May 6, 2021

Conversation

dnetguru
Copy link
Contributor

@dnetguru dnetguru commented Apr 24, 2021

What does this implement/fix?

This PR adds a boolean config option enable_mdns (default=true) to the esphome section that can be used to enable/disable mDNS in firmware codegen.

Type of changes

  • New feature (non-breaking change which adds functionality)

Related issues:
esphome/feature-requests#983
esphome/feature-requests#769

Pull request in esphome-docs with documentation:
esphome/esphome-docs#1118

Test Environment

  • ESP32
  • ESP8266
  • Windows
  • Mac OS
  • Linux

Example entry for config.yaml:

esphome:
  enable_mdns: false

Explain your changes

The enable_mdns option will control the inclusion of ESPmDNS and ESP8266mDNS libraries AND defines the USE_MDNS flag for the preprocessor.
All the related mDNS code is wrapped around conditional preprocessor directives so they are omitted when enable_mdns is set to false.

I have tested this with ESP32, ESP8266 using WiFi.
I don't have access to an ESP32 ethernet module, so I would appreciate it if someone can test this change with an ethernet shield.

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@glmnet
Copy link
Member

glmnet commented Apr 25, 2021

I'm ok with this but I believe it will be better if the options is under wifi and ethernet to maintain cohesion

@dnetguru
Copy link
Contributor Author

That is what I initially had in mind but I noticed that for some reason the include flags for mDNS libraries were added in core_config.py AND it looks like there is no straightforward way to access other sections of config from that part of the code.

I will move the inclusion of mDNS libraries to WiFi and ethernet components and move the config option with them.

@probot-esphome probot-esphome bot removed the small-pr label Apr 26, 2021
@dnetguru
Copy link
Contributor Author

Done! Now enable_mdns can be defined under wifi or ethernet.

dnetguru /workspace/esphome/config $ ls
test2  test_node.yaml
dnetguru /workspace/esphome/config $ grep -B1 mdns test_node.yaml 
ethernet:
  enable_mdns: true
dnetguru /workspace/esphome/config $ esphome test_node.yaml compile 2>&1 | grep SUCCESS
========================= [SUCCESS] Took 3.02 seconds =========================
dnetguru /workspace/esphome/config $ readelf --syms $(find . -name firmware.elf) | grep -i mdns | wc -l
128
dnetguru /workspace/esphome/config $ sed -i 's/mdns: true/mdns: false/' test_node.yaml 
dnetguru /workspace/esphome/config $ grep -B1 mdns test_node.yaml 
ethernet:
  enable_mdns: false
dnetguru /workspace/esphome/config $ esphome test_node.yaml compile 2>&1 | grep SUCCESS
========================= [SUCCESS] Took 41.22 seconds =========================
dnetguru /workspace/esphome/config $ readelf --syms $(find . -name firmware.elf) | grep -i mdns | wc -l
0
dnetguru /workspace/esphome/config $ 

@glmnet
Copy link
Member

glmnet commented Apr 26, 2021

disabling mDNS when using MQTT and/or static IP addresses

is there a validation rule that must be done? e.g. disabling mdns and using dhcp should not validate?
I think not as dhcp can work with a fixed ip lease and no need for host name

@dnetguru
Copy link
Contributor Author

dnetguru commented Apr 26, 2021

No, there's no need for enforcing that. Since as you pointed out the user can assign a static lease or determine the IP address in some other way.

The most common use case however is going to be MQTT and static IP addresses which I mentioned I'll cover in the docs.

I did consider having a static IP OR not enabling the API server to imply not enabling mDNS by default but ultimately I decided against it since it's possible some users might be relying on mDNS for discovery in Hass even when they're using a static IP.
The online status functionality in the dashboard seems to rely on mDNS as well, so this might confuse some users.

With that said, it might be worth considering changing the default behavior to disable mDNS when MQTT is solely used.

@dnetguru
Copy link
Contributor Author

@glmnet added a PR for docs

@glmnet
Copy link
Member

glmnet commented Apr 27, 2021

The online status functionality in the dashboard seems to rely on mDNS as well, so this might confuse some users.

Please add this on docs too. E.g

If you disable mdns then you will need to set an static IP and use ping for online status to work in the dashboard.

(You can link to the faq note about that, see docker entry)

Also please test this, we definitely are going to get issue reports about this otherwise

@dnetguru
Copy link
Contributor Author

Done. I added a section on this to the FAQ and linked to it from the components. See: esphome/esphome-docs#1118

As for testing, I've fully tested this on ESP32 and ESP8266 nodes using WiFi.
I don't have access to an ethernet module, so I haven't done any testing with the ethernet component besides making sure that this config flag controls the inclusion of the mDNS code (and mDNS library symbols) in the resultant firmware.elf file using static analysis.

Feel free to tag me on any issues that are opened about this.

@glmnet
Copy link
Member

glmnet commented Apr 30, 2021

Looks good to me, we don't use hass.io anymore, in this case you mean Home Assistant itself, you can say Home Assistant Supervisor if you mean old hass.io specifically

@glmnet
Copy link
Member

glmnet commented May 3, 2021

I can test Ethernet

What should I check?

@dnetguru
Copy link
Contributor Author

dnetguru commented May 3, 2021

First we need to make sure that the existing mDNS functionality continues to work when this flag is enabled.
To do this, just compile a firmware with ethernet and enable_mdns set to true. Use wireshark or tcpdump to see the mDNS broadcasts are being sent and everything works as expected.

Then change the ebable_mdns to false (and switch to a static IP if not already set) and recompile/flash then use wireshark or tcpdump again to verify that the mDNS broadcasts are absent from this node and everything else works as expected.

@dnetguru
Copy link
Contributor Author

dnetguru commented May 5, 2021

@glmnet were you able to do these tests? Can we get this and esphome/esphome-docs#1118 merged?

@probot-esphome
Copy link

probot-esphome bot commented May 6, 2021

Hey there @esphome/core, mind taking a look at this pull request as its been labeled with an integration (network) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@glmnet glmnet removed the needs-docs label May 6, 2021
@glmnet
Copy link
Member

glmnet commented May 6, 2021

Thanks 🦙

@glmnet glmnet merged commit 2225594 into esphome:dev May 6, 2021
@dnetguru dnetguru deleted the disable_mdns branch May 6, 2021 18:14
This was referenced May 9, 2021
martgras pushed a commit to martgras/esphome that referenced this pull request May 13, 2021
* Added an option to disable mDNS

* Fixed linter issues

* Moved the enable_mdns option to WiFi and Ethernet components

* extracted common method for add mdns library

* lint

Co-authored-by: Guillermo Ruffino <glm.net@gmail.com>
This was referenced May 18, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants