BUG: Default take IP of first interface for event listener (instead of last)

hklages commented 3 years ago

What

Could you please consider to replace your getIp programm with the following or something with same result. source

getIp: async (idx) => { // idx = 0 means first available address, what works for me on all systems (henning)

    const addresses = []
    const interfaces = networkInterfaces()
    let name
    let ifaces
    let iface

    for (name in interfaces) {
      // eslint-disable-next-line no-prototype-builtins
      if(interfaces.hasOwnProperty(name)) {
        ifaces = interfaces[name]
        if(!/(loopback|vmware|internal)/gi.test(name)) {
          for (let i = 0; i < ifaces.length; i++) {
            iface = ifaces[i]
            if (iface.family === 'IPv4' &&  !iface.internal && iface.address !== '127.0.0.1') {
              addresses.push(iface.address)
            }
          }
        }
      }
    }

    // If an index is passed only return it.
    if(idx >= 0)
      return addresses[idx]
    return addresses
  },

Why

As you know I am creating a new Node-RED custom node based on nodes-sonos-ts. In Node-RED you do install custom nodes via a menu item. It is really not common to work with ENV variable. On my development system (windows 10) and for Docker on Synology (another test system) you algorithm provides the same ip address and there is no need to use ENV.

But unfortunatley on the most importin system - my production system, a Homematic CCU3 with RedMatic - your program provides a 10.xxxxx address. The program above provides the "right" address 192.168.178.25.

Yes- it is possible (editing a specific file) to set the environment variable - but that is really not acceptable for many users. Furthermore any update may override these changes.

Alternatives

fork your project and modify this part
use the original node-sonos. That works as it uses the ip package
any of your ideas to ensure that for these kind of systems we don't need to set any ENV variable

Additional context

I tested now most of the features for the Node-RED event node and it works fine. Currently there are 3 nodes but I just finished the new "general event node" where you can subscribe to any service or special event on that service (about 20 options) with a click. You can add, delete, rearrange events. Next week the new node will be finalized (currently only the ui is prototyped).

NodeREDSonosEventNode

svrooij commented 3 years ago

For what functionality should this be changed?

Or what isn’t working at the moment?

the even listener already uses something like your code. https://github.com/svrooij/node-sonos-ts/blob/b6e73140375569d25839785c441f512aaa7c354d/src/sonos-event-listener.ts#L27

hklages commented 3 years ago

right - the event listener. right - there is a private static getHostIp what looks "something like"

but unfortunately "something like" returns different value for the ip address on a CCU3 system.

svrooij commented 3 years ago

Let me clarify question, in what places are you experiencing getting the wrong host IP?

hklages commented 3 years ago

Maybe you remember our discord dialog with the following information:
Dec 11 15:49:09 ccu3-webui daemon.err node-red: 2020-12-11T14:49:09.908Z sonos:eventlistener Listener created host: 10.220.44.18 port: 6329

So on my CCU3 without any ENV the node-sonos-ts created a listener on 10.220.44.18, port 6329. But the DNS server in my network assigns 192.168.178.25 to that small device. So Node.JS, Node-RED, putty, winscp are at 192.168.178.25.

For testing purposes I copied your getIp code and the code from stackoverflow, created two Node-RED/Node.JS endpoints and compared the results on my platforms.

svrooij commented 3 years ago

Problem:

So the problem is, at some devices (with more then one network card) it doesn’t recognize the right IP to use as a luisteren for the events?

Proposed solution

The above code is just luck, it still takes all interfaces filters a few and the returns the first IP it finds. Your code just filters loopback|vmware|internal and not vEthernet. You can also submit a PR to also filter those other names. But I think that people that use VMs will get really disappointed if this library stops working because we change how it selects the correct ip. An interface with the name vmware might be invalid for you, but not for others.

Simple fix for your situation

Instead of setting the environment variable, you can also set process.env.SONOS_LISTENER_HOST from your node app right before you start the Sonos manager.

current code

similar code is currently used.

https://github.com/svrooij/node-sonos-ts/blob/b6e73140375569d25839785c441f512aaa7c354d/src/sonos-event-listener.ts#L33-L36

hklages commented 3 years ago

Thanks - good points. I will have a closer look at the algorithm. The vmware filter is not the issue - we can leave it out. The interface name for 10.220.44.18 is tun0-00 - a vpn tunnel. I guess we can filter that instead - right?

I am not a friend of ENV variables: They are good for system maintainer but not for user. And I loose control. Everyone can (accidentally) change ENV - no security, access restriction, have to be kept in sync wiht program, ... Why do you prefer ENV over global variable/settings, or just passing as parameters to constructor?

In the moment I can life with that. We can close this issue.

My solution: Before subscribing I use the other algorithm and set the ENV variable in case it is not already set. That means in most of my cases there is no need to use the config screen. You can see the config screen in the attachment. The user can start a search and select the ip address. Unfortunately the changes needs another restart of node-RED.

config screen

svrooij commented 3 years ago

A closer look makes me wonder that it's just a matter of order. Your code gives the ip of the first interface, and the current code gives the ip of the last interface. And since the default interfaces on linux start with ens.. or eth.. taking the first is better then taking the last tun.. in this case.

Also tun can always be filtered? Since you probably wont connect to sonos over a vpn (and the sonos speaker is able to connect back)

hklages commented 3 years ago

Just for your information: I checked the internet for interface names and put together this list:

at -ATM
docker - docker installation
en - used at apple
eth - ethernet wired
fw - firewire
ib - infiniband
lo -loopback also lo0
sit
sl - serial line interface
so - serial optical interface
stf - transition ipv4, ipv6
tun - tunnel, vpn
tr - token ring - still there)
ural
vmk - vmware
vi - virtual ip address
wlan - wireless
ww - wireless lan

svrooij commented 3 years ago

And this list is just for linux/mac they are different on windows.

So it's still luck to pick the right one. Alphabetical it just takes the first one (that should fix your issue) and it allows setting the host/port with SonosEventListener.DefaultInstance.UpdateSettings({ host: 'fake-host' , port: 10000 });

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.3.0-beta.6 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

svrooij / node-sonos-ts