First refactoring work, decouple wireguard from clustering #41
Conversation
Move the computation of clusterKey and bindAddress to separate functions.
The cornerstone for exchanging data is the node structure and associated metadata. Moving it to a separate file (maybe later a separate module) will help decoupling.
The wireguard is mostly used to compute metadata. Metadata is now computed by main.go and encoded in node.go, the cluster only receives a function generating the binary metadata.
Decouple the config structure from the cluster management and stop passing the config object around.
Move the calls for metadata decoding from the cluster membership management loop to the main loop. This task was not directly related to the cluster, and was adding complexity, including the need for multierr structures.
|
Also, this is directly related to #37 |
Co-authored-by: Leo Antunes <[email protected]>
|
I have a commit on the way for splitting things into modules. Should I wait for us to settle the current issues before pushing further bits into this PR maybe? |
|
Thanks again for the feedback. I just pushed most requested changes, hope this is still in the right direction. Waiting for your input before I push anything about splitting into modules. |
Co-authored-by: Leo Antunes <[email protected]>
Co-authored-by: Leo Antunes <[email protected]>
|
Thanks for the feedback, and no problem about the delays. After a second thought, passing a |
Not at all! Could just mean I'll need a bit more time to review them, but that's not a deal-breaker. |
Splitting into modules will help keep concerns separate, at the cost of a slightly more verbose code.
This means nodeMeta can be private again, and also makes it easier to pass a Node object to the cluster for local meta, instead of a generic byte[] function. For later updating, that node is passed using Update() instead of New().
Currently unit tests only succeed if the state path is writeable, since it is hardcoded.
Instead of using a trivial function, simply generate the local node from main.go, manually assign fields from the nodeMeta struct.
This also caused renaming local variables to avoid conflicting with the wg package name itself.
Implementing on Node makes a bit more sense than implementing on Cluster even if the difference in code is insignificant.
|
Just pushed the remaining changes. I hope this is not too much for review. I tried implementing the delegate interface on the |
Review suggestions in the documentation Co-authored-by: Leo Antunes <[email protected]>
Also rename setupDelegate to SetLocalNode, so that main can call SetLocalNode then Update().
EncodeMeta and DecodeMeta are more explicit.
Instead, the overlay address is assigned upon creation. Also, the wireguard state is responsible for populating the local node object.
This makes for more straightforward calls. Also, generate the localNode directly in wg.New.
|
Thank you again for the review. I have tried and implemented your suggestions. I will wait for the build. The state test will possibly fail depending on permissions on the builder. |
Co-authored-by: Leo Antunes <[email protected]>
This is a temporary fix until the state path is made configurable.
kaiyou
changed the title
|
Pushed these last details and fixed the tests. Also, I removed the WIP from the title since I feel confident about this. Thanks for guiding the work :) |
|
Nice! This is a definite improvement and will make further work easier! Thanks again! 👍 |
This first PR is still a WIP awaiting feedback.
The overall work is split into mostly consistent commits for easier incremental review, attempting to:
At this point, end to end tests run fine, I have not added any logic yet, and am waiting for the first refactoring work to be (hopefully) merged before actually adding features.
fixes #37