New features - gateway caching / bypass, bugfixes #9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 14 commits
January 8, 2024 14:00
This could be better implemented using the GetListOfPortMappings api. However, it does require a second parsing of the returned CDATA, and the returned object is of a different format. For miniupnpd, the current implementation will return an error with the text `ArrayIndexInvalid` - so on the first iteration, we can use this to determine if there are no records available, instead of failing on the next test and returning an error - when there isn't. If there is another upnp implementation being used, it will fall back to the prior behavior.
This is handy for a user to be able to cache the router url. Sometimes, multicast will work for the first 300 seconds until the join times out, in this case - the user can cache the URL, and if it fails when using SSDP, it can set the url and try again without multicast.
|
Forgot to mention - I'm running this on one of my nodes (with no changes) - works fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull is a precursor to larger work I'm doing that introduces a new package
fluxport-controllerthat I'm hoping the main flux repo starts using. (I'll do a pull for that later)Fluxport-controller uses multicast and the local nodes elect what port they want. It also factors in previous ports used etc. Means users don't have to configure ports anymore. I'll give more details when I push it to github. (for the flux team to fork and create npm package) It would also be opt in so users just set a new flag for it to operate. I just need to work with someone as Fluxbench will need to be updated to get the api port dynamically (I have about 6 different ways we could do this) Could do it without updating as well by just writing to the config file and restartnodebenchmarks rpc, but that's ugly.
Background for this pull
Flux leans heavily on UPnP, which in turn leans heavily on multicast. Multicast can be tricky - especially for home operators that like to tinker with a bunch of switches are routers in their environment, this can interfeer with multlicast operations.
A common issue is that a node will start up and upnp will work for the first 5 (or so) minutes, then igmp snooping kicks in and breaks the node. This is because multicast is broken, not the actual UPnP router service.
Interop with existing fluxOS
This pull doesn't change any of the existing functionality. It's only when extra parameters are passed in do things behave differently.
Testing
All tests passing. Can be run with
npm run flux-test. If required, I can provide adocker-composefile that includes an isolated network and miniupnpd implementation, that will auto-run the tests.New features
This is a flag that can be turned on in the
Clienti.enew Client({ cacheGateway: true });. What this does: On each call togetGateway()the client will cache the SSDP (multicast) respose that gives the url for the upnp service on the router. If multicast then breaks and the SSDP call times out, it will return the cached SSDP info.This gives the ability to be able to pass in the UPnP service router url and the local address (the target of the port forwards) so that SSDP can be bypassed entirely. This is sort of the same as gateway caching but is meant for users to be able to pass in - in a secenario where SSDP is broken, this is faster as the promise returns immediately, instead of waiting for the timeout.
New tests
Commits worth noting
e537bb9 - This was returning an error that wasn't being handled properly by the upper layer, now just throws.
6ed8841 - There are better ways to implement
getMappingsbut they involve double parsing and changing the types as the api calls return different objects, so I left this. However, I did fix an error here where if there are no mappings, it was returning an error - which is incorrect. Specifically forminiupnpdit will return an index error. We now test for this and return empty array if that's the case. If other implementations return something different, then it will revert to the prior behavior.6763ca0 - I was wrong here. I thought the address was the remote address, but it's the local address used for a mapping target. Fixed this up in a subsequent commit and changed the variable name to better reflect the intent.
3a14b07 - This is the meat of the tests.
d8a1913 - Initially, I bumped all the deps to the latest versions. Tested and all working. However, wasn't sure how this would interop with fluxOS, so reverted.