search

brianhealey / hacs_amplipi

Adds media player support for the AmpliPi MultiZone Audio Player
MIT License
12 stars 9 forks source link

Filter private IPs according to spec in rfc1918 #10

Closed linknum23 closed 1 year ago

linknum23 commented 1 year ago

As suggested by @bs42. Tested on the latest HA: 2022.12.8

bs42 commented 1 year ago

172.16/12 doesn't mean 16 and 12, it is CIDR notation meaning the IPs between 172.16.0.0 and 172.31.255.255.

Another option might be to use the python ipaddress module as it can do all the checks to see if an IP is within a range.

linknum23 commented 1 year ago

Oops, my mistake. I'll test out a sensible alternative with that library.

bs42 commented 1 year ago

Awesome!

linknum23 commented 1 year ago

This needs a little more testing, still a work in progress...

brianhealey commented 1 year ago

I am not sure the responsibility for filtering should be on homeassistant. Can you fix your development installs to not emit invalid ip addresses instead?

In fact, wouldn't it make more sense for each reporting ip address to show up as a distinct device in HA, and you can choose to enable/disable them instead? I think that is more inline with how other home assistant integrations work.

As we discussed at the beginning of this project, the amplipi API was severely lacking and we needed to make some tradeoffs in order to implement the amplipi integration. One of those being distinct ID's for each amplipi instance. Instead of accepting this MR, I would prefer to revisit the shortcomings in the API and purge any and all code that implements awkward workarounds. For example, if the API was to emit a flag telling the integration whether it was public/private, HA could observe that instead of using an IP.

linknum23 commented 1 year ago

Yeah I think that makes a ton of sense. Sorry about this PR. I'll close this and make one that removes the filtering.

Just a heads up the last PR https://github.com/brianhealey/hacs_amplipi/pull/9 added distinct ID strings that have the MAC address embedded in them. It also had the awkward filtering that should be removed.