Add rudimentary support for broadcast via aiosync-client #292

mzpqnxow · 2022-11-20T22:16:07Z

This is referenced in #291

I tried to keep this as light as possible, just to be enough to suit my needs (interfacing with a proprietary technology that uses COAP with UDP broadcast as the transport)

If you prefer this be done differently, please let me know. I'm happy to take your guidance. I'm very new to COAP and equally new to the aiocoap codebase

Thanks!

chrysn · 2022-11-20T23:12:24Z

aiocoap/protocol.py

@@ -159,7 +159,7 @@ async def create_client_context(cls, *, loggername="coap", loop=None, transports
            if transportname == 'udp6':
                from .transports.udp6 import MessageInterfaceUDP6
                await self._append_tokenmanaged_messagemanaged_transport(
-                    lambda mman: MessageInterfaceUDP6.create_client_transport_endpoint(mman, log=self.log, loop=loop))
+                    lambda mman: MessageInterfaceUDP6.create_client_transport_endpoint(mman, log=self.log, loop=loop, broadcast=broadcast))


Is there any good reason to make this a flag? IOW: Will anything bad come of aiocoap always setting that option?

We're setting a lot of socket options even though we don't know yet whether we'll need them on this particular occasion. (For example, V6ONLY=0 is set even though we may not use IPv4 at all).

Very good question. I think the downside is that superuser privileges (or possibly some CAP_* on Linux) are required to set this particular ioctl, but I need to check on that

I think I'm misremembering regarding the requirement of additional privileges, the EPERM was coming from an earlier version of this patch which also added SO_BINDTODEVICE

You may wonder why on earth that would be necessary- the server software I was dealing with actually required the universal broadcast address 255.255.255.255 for a specific endpoint, and I'm working in a multi-homed environment with multiple interfaces having +BROADCAST

In the end I decided it would be outside the scope of this PR to add an option to force the interface, especially without any discussion. My final conclusion was to not attempt to solve that problem with changes to aiocoap; there are a few workarounds good enough for that exceptional case

tl; dr; your suggestion on defaulting to SO_BROADCAST seems to be a good one as far as I can tell

chrysn · 2022-11-20T23:13:32Z

aiocoap/protocol.py

@@ -241,13 +241,11 @@ async def create_server_context(cls, site, bind=None, *, loggername="coap-server
                    lambda mman: MessageInterfaceSimple6.create_client_transport_endpoint(mman, log=self.log, loop=loop))
            elif transportname == 'tinydtls':
                from .transports.tinydtls import MessageInterfaceTinyDTLS
-


Please don't change whitespace or other unrelated code in PRs. If this is your editor's doing, you may want to reign it in on code bases that don't follow that editor's particular coding style.

Yep, sorry 'bout that, this one (or two) slipped through. I managed to avoid my urge to run black against the entire codebase and submit that as a PR 😜

The concern is duly noted, and as soon as I have the number of large branches down a bit, and have verified that no code is becoming less readable, black will be the one color in which you get aiocoap, and CI will mark any specks of color with a big red cross mark.

The concern is duly noted, and as soon as I have the number of large branches down a bit, and have verified that no code is becoming less readable, black will be the one color in which you get aiocoap, and CI will mark any specks of color with a big red cross mark.

The world wants to know- when will we have PEP-484 across the codebase and no red marks next to mypy? 😉

On a serious note though, I'd be happy to help with that effort- for the public interfaces or the entire project- if you're interested obviously, and if time permits

Personally I now find it nearly impossible to write good interfaces without it, though that may be evidence of my lack of discipline rather than evidence of the broad value of type annotations

chrysn · 2022-11-20T23:13:43Z

aiocoap/tokenmanager.py

@@ -256,7 +256,13 @@ def request(self, request):
        self.outgoing_requests[key] = request
        request.on_interest_end(functools.partial(self.outgoing_requests.pop, key, None))

-'''
+'''Not implemented


See above on coding style.

chrysn · 2022-11-20T23:14:17Z

aiocoap/transports/udp6.py

        try:
            sock.setsockopt(socket.IPPROTO_IPV6, socknumbers.IPV6_RECVPKTINFO, 1)
        except NameError:
            raise RuntimeError("RFC3542 PKTINFO flags are unavailable, unable to create a udp6 transport.")
+        if broadcast is True:


See above: Can this be unconditional?

chrysn · 2022-11-20T23:14:44Z

aiocoap/transports/udp6.py

+            try:
+                # This shouldn't be needed, at least not when using --broadcast
+                # with aiocoap-client.
+                if not sock.getsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST):


Checking and then setting when not yet set is no faster or better than just setting it.

chrysn · 2022-11-20T23:15:42Z

aiocoap/transports/udp6.py

+                log.fatal("Unable to set socket to SO_BROADCAST; setsockopt() failed")
+                raise


Suggested change

log.fatal("Unable to set socket to SO_BROADCAST; setsockopt() failed")

raise

log.warning("Unable to set socket to SO_BROADCAST; continuing, but requests can not be sent to broadcast addresses.")

When there is no extra flag, this can best-effort.

chrysn · 2022-11-20T23:17:08Z

aiocoap/transports/udp6.py

        sock = socket.socket(family=socket.AF_INET6, type=socket.SOCK_DGRAM)
        sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0)
+        if broadcast is True:


This is redundant with the code in _create_transport_endpoint, which is called right after this.

mzpqnxow · 2023-05-16T12:11:48Z

@chrysn appreciate the review, thank you! I agree 100% that it's clunky as a flag (I was cringing as I was propagating it through the call tree, ending up with new kwargs to functions)

All of the changes are no brainers, I just need to confirm whether the broadcast ioctl requires elevated privileges

Regardless, I can just catch the EPERM and per your preference (re: best-effort/don't raise on failure to set) log the failure and continue if the privileges aren't there

I will get back with updates once I've incorporated your feedback and fixed the merge conflicts that have popped up

Thanks again!

chrysn · 2023-05-16T12:17:35Z

best-effort/don't raise on failure

We have to pick one of these because we can't be sure whether a v4 address is broadcast or not, right? If so, best-effort setting it is probably an OK way. I'd like to redo the transport configuration at some point so that there's a configurable way to do this that doesn't need to thread flags through many layers, but I'm not fully clear there yet. (Could be a TOML-ish data structure that tells which protocols to bind to which ports, would allow binding to multiple ports, and setting things like multicast groups, and setting flags such as SO_REUSEPORT or this one).

chrysn · 2023-05-16T12:37:13Z

On Tue, May 16, 2023 at 05:32:44AM -0700, AG wrote: You may wonder why on earth that would be necessary- the server software I was dealing with actually _required_ the universal broadcast address 255.255.255.255 for a specific endpoint, and I'm working in a multi-homed environment with multiple interfaces having +BROADCAST

Given that we're on the udp6 transport that works in IPv6 semantics with all its benefits, do you really need to bind the interface, or can't you use the tuple ('::ffff:255.255.255.255', 5684, 0, interface_id) to send the request out on the right interface? That should even work with URI notation as `coap://[::ffff:255.255.255.255%eth0]`.

Add rudimentary support for broadcast via aiosync-client

ff0c40a

chrysn reviewed Nov 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rudimentary support for broadcast via aiosync-client #292

Add rudimentary support for broadcast via aiosync-client #292

mzpqnxow commented Nov 20, 2022

chrysn Nov 20, 2022

mzpqnxow May 16, 2023

mzpqnxow May 16, 2023 •

edited

Loading

chrysn Nov 20, 2022

mzpqnxow May 16, 2023

chrysn May 16, 2023

mzpqnxow May 16, 2023

chrysn Nov 20, 2022

chrysn Nov 20, 2022

mzpqnxow May 16, 2023

chrysn Nov 20, 2022

chrysn Nov 20, 2022

chrysn Nov 20, 2022

mzpqnxow commented May 16, 2023

chrysn commented May 16, 2023

chrysn commented May 16, 2023 via email

		log.fatal("Unable to set socket to SO_BROADCAST; setsockopt() failed")
		raise

	log.fatal("Unable to set socket to SO_BROADCAST; setsockopt() failed")
	raise
	log.warning("Unable to set socket to SO_BROADCAST; continuing, but requests can not be sent to broadcast addresses.")

Add rudimentary support for broadcast via aiosync-client #292

Are you sure you want to change the base?

Add rudimentary support for broadcast via aiosync-client #292

Conversation

mzpqnxow commented Nov 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzpqnxow May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzpqnxow commented May 16, 2023

chrysn commented May 16, 2023

chrysn commented May 16, 2023 via email

mzpqnxow May 16, 2023 •

edited

Loading