JKRhb / node-red-contrib-coap

CoAP support in Node-RED
MIT License
27 stars 29 forks source link

IPv6 AAAA records don't connect, while raw [::] addresses do #15

Open rubdos opened 5 years ago

rubdos commented 5 years ago

What works: coap://[fd01::dead:beef]/abc/def, what doesn't work: coap://lamp01/abc/def, where lamp01 resolves to fd01::dead:beef on a AAAA record.

I had to manually add

reqOpts.agent = coap.globalAgentIPv6;

to function _makeRequest(msg) for the DNS record to resolve.

I fear this is a limitation in the coap library though. If so, I'll direct my issue there. However I think it would imply a major interface redesign there, and maybe the node-red interface should expose a workaround -- somehow.

JKRhb commented 3 years ago

Have you been able to further investigate this issue, @rubdos? I updated the package's dependencies, maybe this solved the problem. If not, let me know :)

rubdos commented 3 years ago

Hey @JKRhb, we couldn't have this lab session at the uni this year because of COVID... We did remote Zigbee stuff instead. I hope to have it back next year though, which would be November 2021.

@thielemans, do you know by any chance whether the dependency bump would have helped here?

JKRhb commented 3 years ago

Hey @JKRhb, we couldn't have this lab session at the uni this year because of COVID... We did remote Zigbee stuff instead. I hope to have it back next year though, which would be November 2021.

Fingers crossed! :)

Regarding the issue, I noticed that there have been similar problems in other packages that have been using the default udp4 agent. Similar to the addition in #17, I would add a checkbox in the request node that lets you choose between the two types of agent. If the checkbox is ticked (which should be the default, I guess) then the line

reqOpts.agent = coap.globalAgentIPv6;

will be included. Do you think this could be a working solution?

rubdos commented 3 years ago

Do you think this could be a working solution?

I'm not 100% sure; if the globalAgentIPV6 has no A-record fall back, then that change will probably break the plugin for all the V4 users out there that use DNS.

Besides, this issue should probably be resolved at https://github.com/mcollina/node-coap and not here, now I think about it. DNS and IP is a lower layer than what this package should provide, but I'll have to dig deeper in the source code for that.

By the way (re #17): binding on [::1] doesn't guarantee a hybrid/multi protocol socket. I think that binding on [::] yields a v4/v6 hybrid socket on some Linux systems, but that's distro-dependent iirc.

Thanks for coming back to this, sorry that I'm currently not very helpful!

JKRhb commented 3 years ago

Thanks for coming back to this, sorry that I'm currently not very helpful!

No worries, thank you for your helpful feedback and for raising this issue in the first place! I will try to investigate this problem further in the next days, especially when it comes to the potential breaking change the switch to globalAgentIPV6 might be. I've seen a similar issues in other packages where a distinction between V4 and V6 URIs was added to ensure continued support for the former. Maybe this could be a solution (or rather: a workaround) for this package as well.

By the way (re #17): binding on [::1] doesn't guarantee a hybrid/multi protocol socket. I think that binding on [::] yields a v4/v6 hybrid socket on some Linux systems, but that's distro-dependent iirc.

You are right – maybe it would be more accurate to label the option as "Use UDP6 socket" and turn it off by default?

JKRhb commented 3 years ago

I just opened #25 as a potential fix/workaround.

JKRhb commented 3 years ago

Hmm, I just noticed that a distinction between IPv4 and IPv6 hostnames is already happening in node-coap (see here), making the changes in #25 unnecessary. Therefore, this issue should actually be resolved there if it still persists, I guess.

rubdos commented 2 years ago

The COVID situation finally allows us to give this lab again on-campus, which means that I'm again bouncing on this.

So, it appears that the way node-coap exposes the API, i.e., via an Agent that creates a socket before sending a packet, this issue needs to be worked around. node-coap should provide an API that only creates the UDP socket after resolving the DNS name. The reason for the API being exposed as it currently stands, is because node-coap seemingly wants to (incorrectly, IMO) unify a listening socket and a sending socket under the Agent abstraction. I have filed https://github.com/mcollina/node-coap/issues/320 to discuss this. I find this extra painful, because NodeJS is a very dynamic thingy where I would expect that connecting to a udp://-style socket with hostname already has all these things built-in.

I suggest we merge #25;, I can confirm after tomorrow 3pm CEST whether that patch is sufficient as workaround.

JKRhb commented 2 years ago

Thank you for the update on this issue and filing mcollina/node-coap#320! I updated the workaround in #25 and hope that works for now. Otherwise I totally agree with you that this should be handled on a lower level and I am looking forward to the discussion over at node-coap!