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

Template rewrite #55

Closed
wants to merge 1 commit into from
Closed

Template rewrite #55

wants to merge 1 commit into from

Conversation

whiskerz007
Copy link
Contributor

This should be a good start towards rewriting the templates.

@FossoresLP
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@Lochnair
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@Lochnair
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@FossoresLP
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

@FossoresLP
Copy link
Collaborator

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

@FossoresLP
Copy link
Collaborator

FossoresLP commented Jan 5, 2021

@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
Copy link

dc361 commented Jan 6, 2021

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

@mistermatt2u
Copy link

mistermatt2u commented Jan 13, 2021

@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
Copy link
Contributor

dcava commented Jan 15, 2021

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
Copy link
Contributor Author

@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
Copy link
Contributor

dcava commented Jan 16, 2021

@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
Copy link

dc361 commented Feb 20, 2021

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
Copy link
Collaborator

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
Copy link

dc361 commented Feb 27, 2021

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

@FossoresLP
Copy link
Collaborator

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

@FossoresLP FossoresLP closed this Mar 2, 2021
@amiga23
Copy link

amiga23 commented Mar 2, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants