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

Resolve hosts for fritzbox_callmonitor #28761

Merged
merged 5 commits into from Nov 28, 2019

Conversation

@guillempages
Copy link
Contributor

guillempages commented Nov 13, 2019

If the configuration supplied "host" is not an IP address, try resolving it

Description:

I had written a hostname as "host" in my configuration.yaml for the fritzbox_callmonitor platform, and was getting warnings "fritzbox_callmonitor is taking longer than 10 seconds to initialize", and also could not find the phonebook with id 0.

Finally I found out, that I have to pass an IP address, and not a hostname in the host parameter. I personally find it nicer if both can be accepted, so I created this patch that checks whether host is an IP address, and if it isn't, tries to resolve it as a hostname.

Related issue (if applicable): I haven't created (or found) an issue for that; I can create one if required. Might be related to #21143.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11196

Example entry for configuration.yaml (if applicable):

sensor:
  platform: fritzbox_callmonitor
  username: myFritzboxUser
  password: MY_PASSWORD
  host: fritzbox.example.com

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Copy link
Contributor

escoand left a comment

You don't need to check if it's an IP address already:

>>> socket.gethostbyname("localhost")
'127.0.0.1'
>>> socket.gethostbyname("127.0.0.1")
'127.0.0.1'
@guillempages

This comment has been minimized.

Copy link
Contributor Author

guillempages commented Nov 14, 2019

You don't need to check if it's an IP address already:

I also thought about that, and checked a little more:

>>> socket.gethostbyname("192.168.1.1")
'192.168.1.1'
>>> socket.gethostbyname("192.168")
'192.0.0.168'

I would agree, that this is a corner case, but feels wrong to me. If you really want me to change it, I can do it of course; I just see my proposal as more robust.

@escoand

This comment has been minimized.

Copy link
Contributor

escoand commented Nov 14, 2019

I would agree, that this is a corner case, but feels wrong to me. If you really want me to change it, I can do it of course; I just see my proposal as more robust.

https://www.heise.de/security/meldung/l-f-Warum-1-1-eine-gueltige-IP-Adresse-ist-4470232.html

@guillempages

This comment has been minimized.

Copy link
Contributor Author

guillempages commented Nov 14, 2019

Ok, you convinced me. I've updated the PR.

BTW, should I also create a PR for the documentation in home-assistant.io, to specify that it also accepts hostnames?

(P.S. how did you know that I speak German? ;-) )

@escoand

This comment has been minimized.

Copy link
Contributor

escoand commented Nov 14, 2019

(P.S. how did you know that I speak German? ;-) )

Did just know this German article and found no English on quick googling. ;-)

@@ -59,7 +59,8 @@
def setup_platform(hass, config, add_entities, discovery_info=None):
"""Set up Fritz!Box call monitor sensor platform."""
name = config.get(CONF_NAME)
host = config.get(CONF_HOST)
# Try to resolve a hostname; if it is already an IP, it will be returned as-is
host = socket.gethostbyname(config.get(CONF_HOST))

This comment has been minimized.

Copy link
@fabaff

fabaff Nov 16, 2019

Member

The documentation says that host: should be an IP address.

Most users don't run their own DNS server at home thus you need to handle the traceback if the lookup fails.

This comment has been minimized.

Copy link
@guillempages

guillempages Nov 16, 2019

Author Contributor

If someone uses a hostname, they should at least have configured a "hosts" file, but I see your point. Added exception catching, and using DEFAULT_HOST as fallback. Or would you prefer just an error message and not initializing the component at all?

I've also created a PR for the documentation: home-assistant/home-assistant.io#11196

Dev automation moved this from Needs review to Review in progress Nov 16, 2019
@guillempages guillempages mentioned this pull request Nov 16, 2019
2 of 2 tasks complete
host = socket.gethostbyname(host)
except socket.error:
_LOGGER.warning(
"Could not resolve hostname: %s.\nTrying default address %s",

This comment has been minimized.

Copy link
@fabaff

fabaff Nov 16, 2019

Member

Remove the part about the default host. If the lookup fails then the setup should fails. Assuming too much things make it hard to debug for users.

This comment has been minimized.

Copy link
@guillempages

guillempages Nov 16, 2019

Author Contributor

done

If the configuration supplied "host" is not an IP address, try resolving it
Instead of just checking whether it is an IP and if it isn't try to resolve; just resolve it; IPs will be returned unchanged, and hostnames will be resolved.
If the hostname cannot be resolved; don't try to fallback; just print the error message.
If the hostname cannot be resolved, log an error and stop the setup;
no entities will be then created.
@guillempages guillempages force-pushed the guillempages:fritzbox_resolve_hostname branch from 9f9506b to c925d3c Nov 24, 2019
@cgtobi
cgtobi approved these changes Nov 25, 2019
Copy link
Collaborator

cgtobi left a comment

LGTM

@fabaff
fabaff approved these changes Nov 28, 2019
Dev automation moved this from Review in progress to Reviewer approved Nov 28, 2019
@fabaff fabaff merged commit 26e674b into home-assistant:dev Nov 28, 2019
11 checks passed
11 checks passed
CI #20191124.20 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 94f8e63...c925d3c
Details
codecov/project 94.47% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 28, 2019
@guillempages guillempages deleted the guillempages:fritzbox_resolve_hostname branch Nov 29, 2019
@lock lock bot locked and limited conversation to collaborators Nov 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.