Fix: allow host names with tapo plugs#29768
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When resolving hostnames,
addr = addrs[0]assumes at least one result; consider explicitly handling thelen(addrs) == 0case to avoid a potential panic from an empty resolution result. - Consider whether
context.Background()is appropriate forLookupNetIPhere, or if you should thread through a cancellable context from the caller to avoid potentially hanging DNS lookups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When resolving hostnames, `addr = addrs[0]` assumes at least one result; consider explicitly handling the `len(addrs) == 0` case to avoid a potential panic from an empty resolution result.
- Consider whether `context.Background()` is appropriate for `LookupNetIP` here, or if you should thread through a cancellable context from the caller to avoid potentially hanging DNS lookups.
## Individual Comments
### Comment 1
<location path="meter/tapo/connection.go" line_range="37-41" />
<code_context>
if err != nil {
- return nil, fmt.Errorf("invalid ip address: %s", uri)
+ // not an IP address, try to resolve as host name
+ addrs, err := net.DefaultResolver.LookupNetIP(context.Background(), "ip", host)
+ if err != nil {
+ return nil, fmt.Errorf("failed to resolve hostname %q: %w", host, err)
+ }
+ addr = addrs[0]
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Be explicit about which resolved IP to prefer (IPv4 vs IPv6) and how to choose among multiple results.
`LookupNetIP` with network "ip" may return a mix of IPv4 and IPv6 in unspecified order. If consumers expect a specific family or deterministic behavior, consider filtering to the desired family and/or defining a clear selection strategy instead of always using `addrs[0]`.
Suggested implementation:
```golang
addr, err := netip.ParseAddr(host)
if err != nil {
// not an IP address, try to resolve as host name
addrs, err := net.DefaultResolver.LookupNetIP(context.Background(), "ip", host)
if err != nil {
return nil, fmt.Errorf("failed to resolve hostname %q: %w", host, err)
}
if len(addrs) == 0 {
return nil, fmt.Errorf("hostname %q did not resolve to any IP addresses", host)
}
// Prefer IPv4 for deterministic behavior; fall back to the first IPv6 address if no IPv4 is available.
var chosen net.IPAddr
for _, a := range addrs {
if ip4 := a.IP.To4(); ip4 != nil {
chosen = a
break
}
if chosen.IP == nil {
chosen = a
}
}
if chosen.IP == nil {
return nil, fmt.Errorf("hostname %q resolved to unsupported IP addresses", host)
}
if addr, ok := netip.AddrFromSlice(chosen.IP); ok {
// use resolved address
_ = addr
} else {
return nil, fmt.Errorf("failed to convert resolved IP for hostname %q to netip.Addr", host)
}
}
```
The replacement block assumes that `addr` is used later in the function. Replace the `_ = addr` line with whatever assignment or usage is appropriate in the surrounding code (for example, if `addr` is a named return value, simply assigning to `addr` is enough and the blank identifier can be removed).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| addrs, err := net.DefaultResolver.LookupNetIP(context.Background(), "ip", host) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resolve hostname %q: %w", host, err) | ||
| } | ||
| addr = addrs[0] |
There was a problem hiding this comment.
suggestion (bug_risk): Be explicit about which resolved IP to prefer (IPv4 vs IPv6) and how to choose among multiple results.
LookupNetIP with network "ip" may return a mix of IPv4 and IPv6 in unspecified order. If consumers expect a specific family or deterministic behavior, consider filtering to the desired family and/or defining a clear selection strategy instead of always using addrs[0].
Suggested implementation:
addr, err := netip.ParseAddr(host)
if err != nil {
// not an IP address, try to resolve as host name
addrs, err := net.DefaultResolver.LookupNetIP(context.Background(), "ip", host)
if err != nil {
return nil, fmt.Errorf("failed to resolve hostname %q: %w", host, err)
}
if len(addrs) == 0 {
return nil, fmt.Errorf("hostname %q did not resolve to any IP addresses", host)
}
// Prefer IPv4 for deterministic behavior; fall back to the first IPv6 address if no IPv4 is available.
var chosen net.IPAddr
for _, a := range addrs {
if ip4 := a.IP.To4(); ip4 != nil {
chosen = a
break
}
if chosen.IP == nil {
chosen = a
}
}
if chosen.IP == nil {
return nil, fmt.Errorf("hostname %q resolved to unsupported IP addresses", host)
}
if addr, ok := netip.AddrFromSlice(chosen.IP); ok {
// use resolved address
_ = addr
} else {
return nil, fmt.Errorf("failed to convert resolved IP for hostname %q to netip.Addr", host)
}
}The replacement block assumes that addr is used later in the function. Replace the _ = addr line with whatever assignment or usage is appropriate in the surrounding code (for example, if addr is a named return value, simply assigning to addr is enough and the blank identifier can be removed).
|
Not sure I follow. Why does Tapo need IP addresses in the first place (didn‘t look at the code). Is this even required? |
|
The insomniacslk/tapo library uses netip.Addr, which represents an IP address. The validation in the evcc code parses the string as an Addr, so it will fail when using anything other than an IP address.
To reproduce the issue, just add a Tapo device via hostname rather than IP address, it will fail (or at least it fails for me, saying that it's not a valid addr).
(On mobile right now so I can't add more references)
May 9, 2026 10:18:53 andig ***@***.***>:
…
[Image]*andig* left a comment (evcc-io/evcc#29768)[#29768 (comment)]
Not sure I follow. Why does Tapo need IP addresses in the first place (didn‘t look at the code). Is this even required?
—
Reply to this email directly, view it on GitHub[#29768 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAR7NYHEMBP6KOOZRF5NG7D4Z3SWVAVCNFSM6AAAAACYWSWCVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DIMJSGAYTQNZQGM].
Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub].
You are receiving this because you authored the thread.
[Tracking image][https://github.com/notifications/beacon/AAR7NYFHMHRAJ2YP4WFRJC34Z3SWVBFCNFSM6AAAAACYWSWCVWWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTYAAAAACBX2CAH2M4TFMFZW63VGMF2XI2DPOI.gif]
|
It does, but why? Looking at the code all it does is converting it to string to build a URL. The real solution would thus be to use hostname throughout where hostname might also be an ip. |
|
That's a good point. I'll move the logic into the |
|
ah, that would require an API change. |
|
@andig there's an extra problem with that change in the |
How likely is that to happen? I'd say not at all... |
|
Any why not just cache the hostname instead of the ip? |
|
anyway, that's not an evcc concern ;) |
in my network, Tapo plugs sometimes get a different IP from DHCP, and the DNS record follows that via other automation. That's not infrequent that IPs change, it's less frequent that people use hostnames instead of raw IP addresses
because it breaks the guarantee that a
why not though? I see the grey area here - it's somewhere in between the library and the caller. But if |
I cant say why that is but it sounds broken. none of the other devices we have do any ip fiddling and really shouldn't. It's your library but imho this is fixing a problem that doesn't have to do with Tapo in the first place. |
|
well, DHCP is not static by design, it's not just my network where IP addresses can change for a device. Another scenario is where I have replaced a tapo plug, which would get a different IP. Anyway I've added a new wrapper |
|
Any well-behaved router will assign the same ip to the same device again and again. But that's beside the point. If the hostname is stable that doesn't matter at all. Just use hostnames everywhere. |
Probably. But I really don't see why you'd need ip addresses at all. No other device does that and all of them depend on DHCP. |
d5a6890 to
2cdfbdd
Compare
Signed-off-by: Andrea Barberio <insomniac@slackware.it>
2cdfbdd to
baa9bd2
Compare
The Tapo plug UI mentions hostnames alongside IP addresses, however when using a host name for a Tapo plug, it returns an error (see screenshot):
This patch enables both IP addresses and host names for Tapo plugs. Tested manually with a local, patched instance (see screenshot). Please let me know if there's a better way to test this.