openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
899 stars 415 forks source link

mDNS discovery result overwrites existing thing configuration/property #3753

Open seime opened 10 months ago

seime commented 10 months ago

Tested on OH 4.0.1

Prequisites: A pre-configured thing (using GUI, not files afaik) with a property X set to a particular value.

Initiate a new discovery (or wait for background discovery to run).

A DiscoveryResult is created using the same thing UID, but with a different value for the property X (also used as the representation property).

Expected result: Existing thing is untouched, and a possibly new entry is added to the inbox. Actual Result: The existing, pre-configured thing is having its property X overwritten with the discovery result value.

Possible workarounds:

While this issue has been detected in relation to mDNS, it might be affecting any kind of discovery.

This issue has been reported in the forum earlier for an unrelated binding: https://community.openhab.org/t/is-here-anyone-using-volumio/16678/116

lolodomo commented 10 months ago

It is not an issue, it is a very old feature of openHAB discovery.

You found by yourself the way to avoid it: just use the non default thing UID for your thing. That's the way I do it.

xanderificnl commented 10 months ago

I appreciate OpenHAB's predictable system behavior. However, I was caught off guard by this unexpected behavior, and I strongly believe that any feature capable of overriding manual configurations is undesirable.

In my case, despite my host OS receiving mDNS broadcasts, it wasn't configured to handle them. As a result, my manual settings were constantly overwritten, rendering things unavailable. Although I eventually enabled resolving mDNS hostnames, the potential for misconfigured or test devices to supersede production devices due to mDNS discovery makes me uncomfortable with this aspect. I'll employ one of the other workarounds but I'd argue this is a bug or at the very least undesirable behaviour from my point of view.

seime commented 10 months ago

Ok, interesting "feature".

Could you elaborate why discovery should overwrite? In this other "bug" I reported (while not identical but related) the argument was somewhat in the opposite direction? ("use a fixed thing uid to avoid rediscovery") https://github.com/openhab/openhab-addons/issues/14533

openhab-bot commented 10 months ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/esphome-binding-for-the-native-api/146849/33

lolodomo commented 10 months ago

I probably myself open an issue about that many years ago. Now I do not remember what were the arguments in favor of this feature. I will try to find it. As it is very old, it may be in eclipse smarthome.

lolodomo commented 10 months ago

I found it, it was in 2017 ! You can read @kaikreuzer arguments. https://github.com/eclipse-archived/smarthome/issues/3975

xanderificnl commented 10 months ago

I found it, it was in 2017 ! You can read @kaikreuzer arguments. eclipse-archived/smarthome#3975

Great find! That was a good read.

In certain instances, service discovery can inadvertently lead to things becoming inaccessible — I've encountered this myself.

Once I modified the hostname for an ESPHome Thing, I explicitly communicated my desired configuration to OpenHAB and it shouldn't have been overwritten.

Cheers everyone, I'm looking forward to reading your thoughts.

lolodomo commented 10 months ago

If like me you don't like this feature, just use a thing ID different from the one considered by the discovery service. Then your conf will not be updated.

Now I just have a doubt what happens if your thing ID is different but your thing has the same representation property as the discovered thing. Update or not?

seime commented 10 months ago

IMHO discovery and maintainance of thing configuration is 2 different concerns and should be separated. I do also have my doubts about the reasons given for current functionality; updating ip adresses (which we have DNS doing) and smarthome relocation to a different position (which to me means almost starting from scratch).

But in favour of keeping backwards compatibility, I suggest another parameter on the DiscoveryResult object that PersistentInbox(assuming this one is the culprit) reads and understands. Something along the lines of updateExistingThingConfiguration which is true by default but can be set to false wherever the current functionality isn't desired.

The workaround on the other hand is very simple to implement, but then forces the user to change the thingUID on adding from inbox instead of providing a sensible UID.

xanderificnl commented 10 months ago

It should be noted that mDNS doesn't work in the same way as DNS does. mDNS is comparable to ARP, but instead of broadcasting a request to learn the MAC address of a specific IP, you're broadcasting a request to learn ALL the devices that identify as something.local - the security implications make it easy to understand why a server OS shouldn't resolve such queries by default.

I believe the solution outlined by @seime is sensible, while I can't speak for the specific implementational details, I'm guessing the UI will be able to communicate to users that a Thing's configuration is read only and managed by service discovery while giving users the oppertunity to take control. That would increase the user experience and make it easier to reason about the system.

wborn commented 10 months ago

I'm guessing the UI will be able to communicate to users that a Thing's configuration is read only and managed by service discovery while giving users the oppertunity to take control

Discovery does not always work if you are in a different subnet, use Docker or when a firewall is blocking the discovery. openHAB does not know about all of this.

xanderificnl commented 10 months ago

I'm guessing the UI will be able to communicate to users that a Thing's configuration is read only and managed by service discovery while giving users the oppertunity to take control

Discovery does not always work if you are in a different subnet, use Docker or when a firewall is blocking the discovery. openHAB does not know about all of this.

The quote was meant in the context of the proposal to add a updateExistingThingConfiguration flag.

If such a flag is available, we could use it to render a Thing's configuration form read-only while the flag is true, as it makes no sense that an end user can change a configuration that OpenHAB will blindly overwrite.

openhab-bot commented 1 week ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/esphome-binding-for-the-native-api/146849/180