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

Fluxport controller - multicast gossip server #1197

Open
wants to merge 46 commits into
base: development
Choose a base branch
from

Conversation

MorningLightMountain713
Copy link
Contributor

@MorningLightMountain713 MorningLightMountain713 commented Jan 22, 2024

Mostly finished. I still need to write tests for the Flux modules, however everything else has unit-tests and integration tests.

One thing I haven't done is test this on old versions of Node - I believe in it's current state it will break on anything prior to node 18. (using structuredClone) I will look at fixing this over the next few days. I'll look at targeting Node 14. (Maybe if I could get access to fluxstats I can check the minimum version on the network)

I am running this on my nodes. Here is an example, showing the fluxOS logs integrated with the fluxport-controller npm package (fluxOS controls the fluxport-controller logs, both write to the same file):

flux_startup

A multicast based server for autoconfiguration of fluxports. Operators just set a new flag in the config file upnp: true and the fluxport controllers works out in conjunction with the other nodes on the local network who gets what port. In a future update, won't need to specify the upnp flag, it will work out if the node is directly plumbed into the internet, in a DMZ or using UPnP.

Features:

  • Uses multicast to determine what nodes / ports are on the local network
  • Algorithm based port determination with tiebreaker
  • Port reuse (based on flux api and port mappings)
  • Includes a multicast echo server (not enabled)
  • Package logs fully integerated with Flux - via logController class
  • Future use for Flux for syncronized node communication and network discovery
  • Able to install without enabling (has no effect if upnp flag disabled)
  • Works with a mix of autoupnp nodes and non autoupnp nodes
  • Unittests and integration tests in fluxport-controller npm package

Why does Flux need this:

For node operators starting out, setting up a Fluxnode can be daunting. Removing the need to configure ports on a Fluxnode makes the setup easier. Also, having a multicast comms server available opens up a lot of avenues for node sync and communication, i.e. starting benchmarks at exactly the same time etc. Checking if nodes are available etc etc. Part of the reason for the echosever is to detect when there are issues with multicast (igmp snooping).

Things that need to happen before this is merged

  • Run flux's test suite and make sure it's still passing (what tests I can)
  • I have a pull request open on @runonflux/nat-upnp - this adds some features and fixes some existing bugs. Update this pull to use @runonflux/nat-upnp instead of my @megachips/nat-upnp package.
  • Runonflux team would need to fork my fluxport-controller repo and push this to npm. Would then need to update this pull to use @runonflux/fluxport-controller instead of @megachips/fluxport-controller.
  • Finish tests for the fluxport controller service in this repo.
  • Fix up readme's in packages
  • Final testing

Some info on the fluxport controller repo

Available at: https://github.com/MorningLightMountain713/fluxportcontroller

Written in typescript. Run tests with npm run unittest. The integration tests were a bit tricky as this requires an isolated network with a functional upnp router. I've done this using docker, where I spin up 8 "fluxnodes", one observer to run the tests and a miniupnpd container. Total of 10 containers. Run using docker compose up --attach observer While the test is running, you can visit http://localhost:33333 and get the test results.

I'll push the containers to dockerhub used for testing in the next day or so.

Notes on some of the changes

  • The logging file had a variable referencing the homeDir, where this was actually the appRootDir. (just renamed)
  • I've added a computed object to the global userconfig - means reused properties can be calculated up front, and a lot of code duplication can be removed. Properties are recomputed on config reload.
  • Added correct return typing for some docstrings.
  • Removed use of capitalization for acronyms, eg apiSSLPort becomes apiSslPort. Having capitals breaks camel casing and makes variable names ambiguous.
  • Since the apiPort is now computed on load, rewrite back the inital apiport that was in the config file. If it was empty, then leave it empty. In my opinion, Flux shouldn't be writing to this config file at all. At least the inital object shoudl be untouched anyway. It is the user supplied config, if Flux needs to write state somewhere, it should do it in another object. (or in the db)

Feedback

Any feedback welcome. I appreciate there is a lot to unpack here, I don't have a good understanding of why some decisions have been made or why code is the way it is. Happy to run someone through it. Maybe you don't like the direction I've gone here, or some of my implementation. Happy to discuss and see if we can find a way forward.

Cheers,

David

@MorningLightMountain713
Copy link
Contributor Author

MorningLightMountain713 commented Jan 23, 2024

From 2067ecc -> b57c961 - Added tags to api and homescreen.

Something that's been missing for Flux - is the ability to tag nodes. E.g. in the config file you specify a tags object, and you are able to see that via api or via gui. Can have multiple tags (I'll add some limits on length)

It's only available to the nodeowner. I.e. they must login. (Hidden if not logged in)

With autoconfiguration of nodes ports, the ability to tag a node is helpful in actually identifying nodes and grouping them.

Screenshot 2024-01-23 at 12 36 06 PM

@MorningLightMountain713
Copy link
Contributor Author

Containers have been pushed to dockerhub.

Integration tests can be run with docker compose up --attach observer from the root of:

https://github.com/MorningLightMountain713/fluxportcontroller

Screenshot 2024-01-23 at 2 16 21 PM

Screenshot 2024-01-23 at 2 16 03 PM

Results on local webserver, will either be pass or fail. If a NAK is received (two nodes requesting the same port at the same time and one gets NAKd) it gives a full commentary of every message received by every node. (This is still a pass)

Screenshot 2024-01-23 at 2 17 13 PM

Copy link

gitguardian bot commented Jan 23, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@MorningLightMountain713 MorningLightMountain713 force-pushed the feature/fluxport_controller branch 3 times, most recently from 79b3f40 to 82234dd Compare January 26, 2024 14:17
@MorningLightMountain713
Copy link
Contributor Author

Have added tests for apiServer.js and fluxportController.js

@MorningLightMountain713
Copy link
Contributor Author

MorningLightMountain713 commented Jan 27, 2024

2024-01-27T09:56:29.942Z          Starting GossipServer
2024-01-27T09:56:29.941Z          Config file changed, reloading userconfig.js...
2024-01-27T09:56:30.256Z [fpc] info: My ip is: x.x.x.x
2024-01-27T09:56:30.321Z          {"type":"fluxappremoved","version":1,"appName":"EthereumNodeLight","ip":"193.34.213.227","broadcastedAt":1706349389119}
2024-01-27T09:56:30.321Z          New Flux App Removed message received.
2024-01-27T09:56:30.432Z [fpc] info: Binding to 16197 on 172.16.32.11
2024-01-27T09:56:30.433Z [fpc] info: Joining multicast group: 239.19.38.57
2024-01-27T09:56:30.426Z          Gossip server got new routerIp: 172.16.32.1, updating
2024-01-27T09:56:36.349Z [fpc] info: Sending type: DISCOVER to: 239.19.38.57:16197
2024-01-27T09:56:37.313Z [fpc] info: Received type: DISCOVER_REPLY from: 172.16.32.10:16197
2024-01-27T09:56:37.428Z [fpc] info: Checking for prior confirmed port via flux api
2024-01-27T09:56:38.051Z [fpc] info: Prior port found: 16127
2024-01-27T09:56:38.052Z [fpc] info: Prior port 16127 found and available, selecting
2024-01-27T09:56:38.052Z [fpc] info: Sending type: PORT_SELECT to: 239.19.38.57:16197
2024-01-27T09:56:38.228Z [fpc] info: Received type: PORT_SELECT_ACK from: 172.16.32.10:16197
2024-01-27T09:56:38.228Z [fpc] info: Port confirmed: 16127
2024-01-27T09:56:38.229Z [fpc] info: {
  '172.16.32.10': { port: 16197, nodeState: 'READY' },
  '172.16.32.11': { port: 16127, nodeState: 'READY' }
}
2024-01-27T09:56:38.228Z          Gossip server got new apiPort: 16127, updating
2024-01-27T09:56:38.584Z          Firewall adjusted for UPNP

Here is one node on new upnp and updating a running second node by adding upnp: true to config file. I'm mostly finished now. Might do a bit more testing over the weekend but will probably remove the WIP soon

@MorningLightMountain713 MorningLightMountain713 changed the title [WIP] Fluxport controller - multicast gossip server Fluxport controller - multicast gossip server Jan 27, 2024
@MorningLightMountain713
Copy link
Contributor Author

Finished final testing. Works well on my nodes

@MorningLightMountain713
Copy link
Contributor Author

Have tested on node 14... works fine.

davew@charlie:~$ node -e "console.log(process.versions)"
{
  node: '14.21.3',
  v8: '8.4.371.23-node.88',
  uv: '1.42.0',
  zlib: '1.2.11',
  brotli: '1.0.9',
  ares: '1.18.1',
  modules: '83',
  nghttp2: '1.42.0',
  napi: '8',
  llhttp: '2.1.6',
  openssl: '1.1.1t',
  cldr: '40.0',
  icu: '70.1',
  tz: '2022f',
  unicode: '14.0'
}
davew@charlie:~$ pm2 --version
5.3.1
davew@charlie:~$ nvm --version
0.39.5

@TheTrunk TheTrunk requested review from Cabecinha84, TheTrunk and XK4MiLX and removed request for TheTrunk January 30, 2024 14:38
David White added 29 commits February 7, 2024 08:11
When going from apiport, routerIP config to just upnp in config file,
It was wiping the old mappings as it detected a port change when there
wasn't. Upon further reflection, there isn't really a good way to
implement this without impacting any existing api / app connections. So
Have removed.

Fixes a logic error with the config file import - on every config
reload, the computed properties were being overwritten. There could have
been edgecases where these properties were needed during the time they
were being re-evaluted. Now only import the initial values and don't
wipe the computed before they are recalculated.
Found a reasonable bug in fluxport-controller state management, so
have bumped verison to pull that in. Small refactor to ipv4 adress
check
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.

1 participant