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

Bind receiving socket to specific interface #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cmair
Copy link

@cmair cmair commented Jun 18, 2021

This makes sure the receiving socket only gets one answer from the switch,
even if multiple interfaces are connected to the same network.

This makes sure the receiving socket only gets one answer from the switch,
even if multiple interfaces are connected to the same network.
@cmair cmair marked this pull request as draft June 18, 2021 17:12
Copy link
Owner

@rgl rgl left a comment

Choose a reason for hiding this comment

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

Can you please explain a bit more about the scenario where/when this happens?

Even better if you could draw some kind of physical network diagram that shows what is connected to the switch ports that makes this happen :-)

plugins/module_utils/network.py Outdated Show resolved Hide resolved
plugins/module_utils/network.py Outdated Show resolved Hide resolved
@cmair
Copy link
Author

cmair commented Jun 18, 2021

My laptop has two network interfaces, both are connected to the same switch. This might be unusual, but I use it to play with different vlans without loosing internet connectivity when I screw up things on one port. I'm now trying to manage this switch (and later others as well) without the web interface.

When I first tried your Ansible collection (and also the original smrt code it is based on) I figured that neither worked at all. The switch answered as expected but the software somehow didn't get the correct answers. After hours of debugging I found that the the debug logs showed the exact same response beeing parsed twice, but for different requests. I also found that wireshark captured two answer packets per request, when listening on the 'any' interface. This seems plausible as the switch answers to the broadcast address, which should obviously go to all ports. Having one machine connected to multiple ports at the same time triggers the issue.

My first quick fix was to call rs.recvfrom() twice, discarding the first result. As this would break with just one or more than two connections to the switch, I tried to find a way to get around this issue.

Another possibility could be to just read all remaining data from the socket before sending a new request or keep track of expected and received/processed answers to detect duplicates, but this seemed to be a simple and elegant solution.

@rgl
Copy link
Owner

rgl commented Jun 18, 2021

I now understand! And I like the solution :-)

Can you also bind the sending socket to that interface? I think that would make it more consistent with the intent of "use the same interface to send/receive data despite the host network configuration".

@cmair
Copy link
Author

cmair commented Jun 18, 2021

I think the sending interface is already bound to the correct interface, as bind is called with the corresponding IP.

I'm not sure if I already understood the overall picture, but maybe it would make even more sense to specify the interface in the inventory.yml and defer the IP from there, as the specified IP is just used to bind to the correct interface? That would make sure the correct interface is used, as someone could also assign the same IP to different interfaces.

@rgl
Copy link
Owner

rgl commented Jun 18, 2021

Ah indeed it is. Scrap what I've said :-)

Since I assign multiple IPs to the same interface, I'd rather not use the interface to infer its IP address.

How about if we binded the receiving socket to the IP instead of the broadcast address? I'm not really sure whether that socket would receive broadcast packets from other interfaces thou... but, if that works, the code would be even simpler.

@cmair
Copy link
Author

cmair commented Jun 18, 2021

I tried binding to the IP, but could not make it work.

Multiple IPs on the interface should'nt be a problem. The IP just shows up as the sender address but as far as I know, the switch does not care. So, just picking the first IPv4 should be fine, as long as the interface is correct. Others are doing so as well.

I'd be more worried about breaking existing configurations. Thats why I didn't look into that yet. Also, because I just started with Ansible today and I still have no clue how things work 🙈

@cmair
Copy link
Author

cmair commented Jun 18, 2021

What does seem to work:

binding the sending socket to 255.255.255.255:28809 and to the interface. Then, a single socket can be used to send/receive stuff. No IP needed at all.

The communication with the switch uses UDP broadcast. Therefore, a specific IP is not required.
Replace host MAC and IP configuration options with host interface. The MAC can be read from
the interface, the IP is not required due to UDP broadcast communication.
@cmair
Copy link
Author

cmair commented Jun 18, 2021

New proposal: I tried to change the configuration to use the interface instead of MAC and IP. My tests where successful so far.
I didn't touch the take_ownership files yet as I can't test them right now (will need to find some switch downtime).

@rgl
Copy link
Owner

rgl commented Jun 19, 2021

Its looking good :-)

What switch model are you testing this on?

@cmair
Copy link
Author

cmair commented Jun 20, 2021

I own two TL-SG108.

@cmair
Copy link
Author

cmair commented Jun 20, 2021

I also changed switch_take_ownership_client.py to the changed constructor of Network. I think this should work now, but I still didn't test yet.

I'll now change the PR to ready for review, as I think all necessary changes are in now.

@cmair cmair marked this pull request as ready for review June 20, 2021 10:06
Copy link
Owner

@rgl rgl left a comment

Choose a reason for hiding this comment

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

Its looking good :-)

plugins/module_utils/switch_take_ownership_client.py Outdated Show resolved Hide resolved
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.

2 participants