search

WireGuard / wireguard-vyatta-ubnt

WireGuard for Ubiquiti Devices
https://www.wireguard.com/
GNU General Public License v3.0
1.46k stars 68 forks source link

Template rewrite #55

Closed whiskerz007 closed 3 years ago

whiskerz007 commented 3 years ago

This should be a good start towards rewriting the templates.

FossoresLP commented 3 years ago

Hi @whiskerz007, thank you very much for your work on this. This looks very promising.

I will add a new CI pipeline in the next few days. Then we can release test builds based on your work for all supported devices.

whiskerz007 commented 3 years ago

While looking at /opt/vyatta/share/vyatta-cfg/templates/interfaces/ethernet/node.def, it shows that comments are allowed in the action fields. If the code is kept in the templates, it would eliminate the need to look in multiple locations for code logic. Should the code be moved to a monolithic script (what is suggested with this PR), or should it be moved back into the template?

FossoresLP commented 3 years ago

IMO the monolithic scripts provide better readability than the Vyatta templates and reduce code duplication so I would keep it the way you wrote this.

@Lochnair Do you have some input on this? You were the one to suggest the external scripts so I would think this is what you were thinking about?

@whiskerz007 It would be great if you could pull in the changes from the current master to run the new CI for this PR.

Lochnair commented 3 years ago

@whiskerz007 Thanks for taking the time to work on this =)

@FossoresLP Yes something like this is what I had in mind. I've only skimmed over the scripts, but they generally look good to me.

I'd be curious to try a build with this whenever you've figured out the CI.

FossoresLP commented 3 years ago

@Lochnair You can find the packages here Look for the release_* artifacts.

Lochnair commented 3 years ago

@FossoresLP Thanks!

Gave it a spin on the ERL I use at home. Upgraded and rebooted. Seems fine so far.

Checked out the commit log and found this:

[ interfaces wireguard wg0 route-allowed-ips false ]
Error: argument "dev" is wrong: "protocol" value is invalid

Didn't find anything that seemed obvious to me that would cause that error.

Lochnair commented 3 years ago

Kept it running on my box, and somehow managed to break it while reconfiguring the tunnel.

Added a new IP address so the config looked like:

wireguard wg0 {
    address 192.168.10.1/30
    address 123.123.123.123/32
}

Tried to remove one of them:

$ delete interfaces wireguard wg0 address 192.168.10.1/30
$ commit
[ interfaces wireguard wg0 address 123.123.123.123/32 ]
RTNETLINK answers: File exists

Commit failed

Trying to commit again gives:

[ interfaces wireguard wg0 address 192.168.10.1/30 ]
RTNETLINK answers: Cannot assign requested address

Commit failed

Current state:

wg0: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1440 qdisc noqueue state UNKNOWN group default qlen 1
    link/none 
    inet 123.123.123.123/32 scope global wg0
       valid_lft forever preferred_lft forever

    RX:  bytes    packets     errors    dropped    overrun      mcast
    3034320444    4143195          0          0          0          0
    TX:  bytes    packets     errors    dropped    carrier collisions
     540408292    2715816          8          0          0          0
whiskerz007 commented 3 years ago

Any advise on how to resolve the conflicts would be greatly appreciated. I believe the merge is where it diverged.

Lochnair commented 3 years ago

@whiskerz007 There might be a better way, but I'd probably create a new local branch based on master and cherry-pick each commit from template_rewrite.

Generally you'll want to use a rebase instead of a merge commit when working on a feature branch, so you don't end up with a cluttered history (and conflicts).

FossoresLP commented 3 years ago

@whiskerz007 You don't have to worry about it, I can pull in the commits manually (instead of merging the PR). I think this needs some more testing before it is ready for a full release. Some people mentioned using your update script. Does it handle prereleases on GitHub or would it just install those like a normal release?

whiskerz007 commented 3 years ago

@FossoresLP I just pushed a new commit for the script to only grab the latest released version by default. You are able to use the same script to install pre-release versions.

whiskerz007 commented 3 years ago

@Lochnair Have you tried the latest commit to this PR? Have you discovered any other problems?

FossoresLP commented 3 years ago

The changes are now finally available as a pre-release. With some testing from other users I hope we can get this ready and merged soon.

FossoresLP commented 3 years ago

@whiskerz007 Could you please take a look at #63? The pre-release was pulled in by an update script but it got us some feedback. There seems to be an issue regarding the description field. Were there any intentional changes?

whiskerz007 commented 3 years ago

@FossoresLP I've corrected the problem with having the description set on the interface. Should be safe to pull into template-rewrite.

FossoresLP commented 3 years ago

Yes, commit looks good. I'll do another pre-release as soon as I can.

FossoresLP commented 3 years ago

@whiskerz007 Pre-release is out now. I'm not quite sure what the best way to get some testers is but I might try the UBNT forums thread. EDIT: Sorry this was a bit early, the CI failed because of some timeout. Rerun should hopefully fix it ASAP.

dc361 commented 3 years ago

The re-write3 code seems to work well on an ER-X that I use for testing. Descriptions present on the peers.

mistermatt2u commented 3 years ago

@FossoresLP Just deployed this on my new EdgeRouter 6P. Seems to work well with a peer in AWS Lightsail. Just have to work on getting routing thru the tunnel. Thanks so much to the team for the effort to develop WG for EdgeOS.

I am new to this project. Let me know if I can do any specific testing that would help.

dcava commented 3 years ago

Thanks @FossoresLP @whiskerz007

Deployed on a Gateway Pro 4 without any dramas at present.

Out of curiosity, will this have any effect on the issues raised by @jollyjinx with his scripts here.

When I added the info to the README regarding configuration persistence on the USG3/4 Pro I have to admit I didn't realise this was an issue, but have now run into it a bunch of times (can't provision if a link is up - completely borks).

Happy to start a new PR to try an incorporate @jollyjinx ideas as effectively, the README info for USG3/4 persistence is wrong.

whiskerz007 commented 3 years ago

@dcava The changes in this PR should correct the problem with deleting the WireGuard node in the configuration.

There is another script you can try to use to update WireGuard periodically and maintain WireGuard through firmware upgrades. I do not have any of the UniFi gateways to test with, however I do believe others have had success with using it. The maintainer would gladly accept assistance with testing and documenting the process for the UniFi gear so others can benefit as well.

dcava commented 3 years ago

@whiskerz007 Thanks - your script works nicely! Very elegant.

The main issue on the USG3/UGW4 side is provision failure (not so much a persistence failure - a catastrophic failure to actually boot the device) as the config has to be stored on the controller in the config.gateway.json - it's much more cludgy then the Edge hardware. I've actually been locked out of 2 devices and had to physically connect to the controller and delete the config.gateway.json file to allow a reboot.

With this release (rewrite-3) provision does not fail (I've tried on a USG3 and Pro 4 now) and it has also now survived a UGW4 pro upgrade, so the work-arounds from @jollyjinx don't appear necessary, and the config can continue to be stored in config.gateway.json. I'll keep testing - but so far, happy days.

dc361 commented 3 years ago

Re-write 4 seems to be working well on an ER-x. I will install on the house border router (ER4) and test in the next few days.

FossoresLP commented 3 years ago

The PR is now merged into master. After some additional testing there will be a new release with these changes as well as the new WireGuard tools.

dc361 commented 3 years ago

Wonderful news! Congratulations to you both (@whiskerz007 @FossoresLP ) on a job well done.

FossoresLP commented 3 years ago

Changes are now in master. Closing since everything is merged. @whiskerz007 Thank you very much for your contribution!

amiga23 commented 3 years ago

Upgrade worked fine for me. Just after upgrade I had to change the owner of the keyfiles to "0:102" and the group needs to have read rights. Thank you all for great work. edit: I am using USG3 with latest firmware