-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix and improve interface binding #2081
Conversation
Signed-off-by: Thomas De Backer <[email protected]>
Signed-off-by: Thomas De Backer <[email protected]>
Thanks very much for the nice explanation and changes. The move from using I agree with what you've proposed, I'll take a look at testing it properly in a little while. I'm not sure what to say with regards the incomplete binding. If it is possible to implement with a single listener then it makes complete sense we should do so. If we effectively have to create new listeners to be able to bind to multiple IP addresses, then this is potentially a problem. |
Looking at it again, and getting a bit more familiar with the direction you're going (deprecating bind_address and port), I think it's a matter of taste.
Could mean:
Could mean:
Then there are also the cases where multiple listeners are defined, ..
The issue here is recognizing the difference between an interface or a host. Cups solves this by putting I don't think the filter case of the second config is realistic, you could just omit the bind_interface config option. Same for the union case in the first example. Writing this out, the missing feature here seems to really being able to define a listener that listens on an interface and certain port. To avoid the strange syntax, you might want to keep/extend This also doesn't answer on how the issue should be fixed in this version, but it's probably important to notice that there is some expressiveness missing in the current configuration options. |
That is not the case,
"Listen on all available addresses" doesn't apply, because
This is the only one that can apply.
Agreed on both points. This approach would prevent the awkward edge cases around having both an interface and an IP address specified, which I think is the crux of your argument. |
Oh, I see, I was a bit off the mark then. Maybe let's ignore this last issue for now, as it seems like it's something that would be best implemented by people with better knowledge of the codebase. I hope my current contributions in this merge request are still helpful, and at least they fix my usecase. Thank you for maintaining this project! |
Improve handling of invalid combinations of listener address and bind interface configurations. Closes #2081.
Thank you for contributing your time to the Mosquitto project!
Before you go any further, please note that we cannot accept contributions if
you haven't signed the Eclipse Contributor Agreement.
If you aren't able to do that, or just don't want to, please describe your bug
fix/feature change in an issue. For simple bug fixes it is can be just as easy
for us to be told about the problem and then go fix it directly.
Then please check the following list of things we ask for in your pull request:
make test
with your changes locally?Hi Mosquitto developers,
While upstepping this component, we noticed a regression in the interface binding of the broker.
More specifically: the commit 886ee6c changed the interface binding behavior from using
SO_BINDTODEVICE
to a method usinggetifaddrs()
.Overstrict binding
While this approach works fine in most use-cases, it can fail when (1) no host is specified, (2) the specified interface does not support ipv4 (or ipv6) and (3) there are other interfaces that do support the missing sa_family. In this use-case, the call to
getaddrinfo()
will give 2 options to bind to: an ipv4 and an ipv6 version. In the loop, the broker will try to get the address for both families, but it will fail on one of the two, resulting in a failed startup. (It's all or nothing)Steps to reproduce:
will result in:
The first commit fixes this issue:
Bind address overwrite
A second issue occurs when specifying both a host to a
listener
config line, and abind_interface
:Because there is no check in the current
net__bind_interface()
, the specified listener address is overwritten by the first valid address returned fromgetifaddrs()
. The second commit addresses this issue by checking if the interface address matches the host. If it does, the address is accepted. The function now seems somewhat complex, so you might just want to disallow this configuration combination altogether. The current solution results in mosquitto listening to the intersection between (the listen address/host) and (the interface addresses).With the same setup as before, but with the changed configuration, the original code gives:
but ultimately on the "wrong" address:
With the fix, mosquitto refuses to launch as no valid listeners could be created:
Incomplete binding
For this issue, I don't have an immediate fix. The problem here is that one interface can have multiple IP addresses. One might expect the
bind_interface
config option to bind to each and every one of those, instead of only the first (valid) one returned bygetifaddrs()
. The current code is written with the expectation that there is only one IP address that would match a family. A possible, proper fix would be to first enumerate the addresses from an interface, and only then as a next step create a listener for each of those addresses. It would be nice to reuse thenet__socket_listen_tcp()
call from somenet__socket_listen_tcp_interface()
, but then you need to convert structures back to strings first. I do want to take a throw at implementing it, but I would love to hear your opinion first.If there are any other issues with this MR, please don't hold back. I would be glad to fix them.