Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extends multicast options #291

Merged
merged 2 commits into from
Jan 17, 2022
Merged

Extends multicast options #291

merged 2 commits into from
Jan 17, 2022

Conversation

jkralik
Copy link
Member

@jkralik jkralik commented Jan 14, 2022

  • Add option to send multicast to an unspecified interface

Co-authored-by: Jozef Kralik jojo.lwin@gmail.com

@jkralik jkralik force-pushed the jkralik/feature/extendMulticast branch from f98cd18 to 3de255f Compare January 14, 2022 08:37
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #291 (1f4c560) into master (697c9f0) will increase coverage by 0.17%.
The diff coverage is 60.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   63.70%   63.88%   +0.17%     
==========================================
  Files          76       76              
  Lines        5756     5875     +119     
==========================================
+ Hits         3667     3753      +86     
- Misses       1701     1725      +24     
- Partials      388      397       +9     
Impacted Files Coverage Δ
dtls/session.go 81.33% <0.00%> (-4.59%) ⬇️
net/blockwise/blockwise.go 67.99% <0.00%> (-0.60%) ⬇️
tcp/clientconn.go 69.53% <0.00%> (+0.94%) ⬆️
udp/session.go 80.76% <0.00%> (-7.97%) ⬇️
net/options.go 53.33% <44.00%> (-46.67%) ⬇️
net/connUDP.go 63.75% <63.24%> (+3.05%) ⬆️
udp/message/pool/message.go 86.25% <66.66%> (+0.54%) ⬆️
udp/client/clientconn.go 72.01% <69.13%> (-0.92%) ⬇️
udp/discover.go 54.54% <100.00%> (+1.35%) ⬆️
net/conn.go 84.37% <0.00%> (-1.57%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 697c9f0...1f4c560. Read the comment docs.

@jkralik jkralik force-pushed the jkralik/feature/extendMulticast branch 2 times, most recently from 6f9870f to b4a61b8 Compare January 14, 2022 09:32
net/connUDP.go Outdated Show resolved Hide resolved
net/connUDP.go Outdated Show resolved Hide resolved
@HRogge
Copy link
Collaborator

HRogge commented Jan 14, 2022

The changes look fine...
I am still a bit unsure about the whole "iterate over all IPs of an interface because as far as I know the "source address" can be the unspecified address when you select an interface... is this a problem on Windows or Mac?

@jkralik
Copy link
Member Author

jkralik commented Jan 14, 2022

The changes look fine... I am still a bit unsure about the whole "iterate over all IPs of an interface because as far as I know the "source address" can be the unspecified address when you select an interface... is this a problem on Windows or Mac?

There is a problem with overall platforms. For example, if you have multiple IP addresses on the interface, some listeners will have incorrect source IP addresses.

@jkralik jkralik force-pushed the jkralik/feature/extendMulticast branch from fdcfba2 to 9029ff7 Compare January 14, 2022 14:27
@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

I just tested the current version of the branch with my multicast test-code and it worked fine (after fixing the reference to the WithMulticastInterface function in my code). I will later test it in a network-namespace to make sure the interface selection works fine (which has to be done in the absence of a default-route to prevent Linux from faking things ^^)

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

I get the following error on a system without a default route:
dial udp 224.0.1.187:5683: connect: network is unreachable

I am calling coapudp.Dial("224.0.1.187:5683", coapudp.WithBlockwise(false, 0, 0)) from my code.

I am not sure the code should call "connect()" for multicast targets, but I am still looking for the place in the code where it happens.

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

I traced the origin of the error, its happening in
net/sock_posix.go:71
if err := fd.dial(ctx, laddr, raddr, ctrlFn); err != nil {

the problem at this point is that the API tries to build an UDP "connection", which will fail for a multicast address in the absence of a default route. The default route is not necessary for sending, but something in the setup go wrong.

We need a socket which has NOT set the remote address so that the socket API doesn't call "connect()" in sock_posix.go:150
connect() is mostly for making unicast connections easier.

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

Okay, I got the issue resolved with the help of a new option I call WithMulticastTarget(), which allows you to overwrite the destination of a WriteMulticast() call... this way you can open the socket without the multicast IP (e.g. ":5634" instead of "224.0.1.187:5683") but still can specify the destination of the Multicast.

How do you want to get the additions (a bit of code in net/connUDP.go and net/options.go)?

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

I can also provide the scripts I use to setup my network namespaces for testing...

@jkralik
Copy link
Member Author

jkralik commented Jan 17, 2022

Okay, I got the issue resolved with the help of a new option I call WithMulticastTarget(), which allows you to overwrite the destination of a WriteMulticast() call... this way you can open the socket without the multicast IP (e.g. ":5634" instead of "224.0.1.187:5683") but still can specify the destination of the Multicast.

How do you want to get the additions (a bit of code in net/connUDP.go and net/options.go)?

You can use (s *Server) DiscoveryRequest() where you can put to server listen socket(":5634"). And then just call discovery. Im not sure with (s *Client) if this will be worked, because it doesn't support multiple peer sessions - just one.

Copy link
Member

@Danielius1922 Danielius1922 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SonarCloud complains about some smell again, otherwise I see nothing else.

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

I have (in my local repository) added an option to allow a client to send Multicast to several addresses by selecting a target for each multicast simultaneously.

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

Can we see if you add #293 to this change set before merging it to master? Multicast support for the server is not that useful if the client side doesn't work correctly...

@jkralik jkralik force-pushed the jkralik/feature/extendMulticast branch from c8a9a46 to 4bafdac Compare January 17, 2022 11:10
@jkralik
Copy link
Member Author

jkralik commented Jan 17, 2022

Can we see if you add #293 to this change set before merging it to master? Multicast support for the server is not that useful if the client side doesn't work correctly...

Please, could you explain your use case?
3fcddc7

Because as I said: If you want to discover clients you need to use
https://github.com/plgd-dev/go-coap/blob/master/udp/discover.go#L29 or
https://github.com/plgd-dev/go-coap/blob/master/udp/discover.go#L43

In the handler of responses, you will have the client which you can store somewhere for eg:
https://github.com/plgd-dev/go-coap/blob/master/examples/observe/server/main.go#L75

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

I am using Discovery/Hello-Messages in terms of networking protocols, not specifically coap. I am working for a German Fraunhofer Institute (Fraunhofer FKIE in Wachtberg) and we are doing work on decentral radio networks, so protocols with "Hello/Beacon messages" are pretty common. Messages which already contain information for the receivers and don't need any answer.

I am not sure if the COAP discovery mechanism is that useful for this purpose.

Originally all of this stuff was done in custom binary messages but I like the idea of having COAP in between and being able to use its semantics without having to reinvent the wheel.

My current project is a lightweight decentralized data store which use broad/multicasted Hash-values to communicate its current state (so that the "steady state" is low-traffic). Similar to the HNCP Homenet protocol (see RFC 7788) but useful for pure low/medium datarate radio networks, not for ethernet/wifi.

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

Just as an additional explanation, in slow radio networks "media access" is often more important than "data rate"... when using a server discovery via multicast (and redo it from time to time because of limited radio range) and then use unicast to get the data, you square the number of packets involved in the process, which can be really bad.

@jkralik
Copy link
Member Author

jkralik commented Jan 17, 2022

Ok, Now I understand :), So I added address as an argument in WriteMulticastMessage in ClientConn - it is much clear. Is it ok now ?

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

Just pulled your commit, changed a few lines in my code and it seems to work fine.

Network Namespaces are just fun for testing code...

@HRogge
Copy link
Collaborator

HRogge commented Jan 17, 2022

hmm... just a thought... did anyone already test the code with IPv6 linklocal addresses and multicast?

Edit: just tested it, linklocal IPv6 multicast works fine.

jkralik and others added 2 commits January 17, 2022 14:04
- Add option to send multicast to an unspecified interface

Co-authored-by: Henning Rogge <hrogge@gmail.com>
@jkralik jkralik force-pushed the jkralik/feature/extendMulticast branch from 1f4c560 to 6ad7efe Compare January 17, 2022 14:04
@sonarcloud
Copy link

sonarcloud bot commented Jan 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

73.5% 73.5% Coverage
0.0% 0.0% Duplication

@jkralik jkralik merged commit ad01d9e into master Jan 17, 2022
@jkralik jkralik deleted the jkralik/feature/extendMulticast branch January 17, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants