Fixed error when deleting wireguard interface and route-allowed-ips i…

[jollyjinx]

…s set to true.

[FossoresLP]

Thanks for the PR!
I'm not a fan of evaluating this to true all the time though because AFAIK this could mean errors would not be handled when just removing allowed IPs and therefore there could be routes left in place without the user knowing.
Maybe we should rather look at why the routes fail to be removed instead of just ignoring that they aren't?

[jollyjinx]

Well, I don't like it either not having found out why it fails.
With the current bug the configuration no longer reflects anything correctly anyways as wg is shut down then, but the configuration still says it's up. So it's at least an improvement.

Digging around I found out: When it's doing the deletion of the interface in wireguard/peer/node.tag/allowed-ips/node.def it fails as follows:

sudo ip route del "10.98.0.0/16" dev wg0
RTNETLINK answers: No such process

after enabling more debugging I found out that at that point in time - when it fails - the interface no longer is existent in the routing table. ip route show no longer lists any wg interface. It looks to me like it has been deleted before the peer allowed-ips gets deleted.

From the commit history of the def files it seems I'm not the only one who did not find proper documentation on the vyctta def tree structure. I think the original author had the information stored in wireguards config, while later authors moved to have the configuration tree contain the information.

[jollyjinx]

I've looked at what happens and why the error occurs. Whats happening in order is:

wireguard/node.tag/address/node.def
wireguard/node.def which delete the interface
wireguard/node.tag/peer/node.def delete
wireguard/peer/node.tag/allowed-ips/node.def delete which fails cause there is no interface and therefore no route either.

It seems that nobody has a grasp in which order things are being destroyed. I have set up an environment now in which I can at least figure out things by adding print statements so I can get an understanding on why this is happening.

A test for the interface in wireguard/peer/node.tag/allowed-ips/node.def should be sufficient I think in - I'll try that out.

[Lochnair]

I believe the correct fix would be getting the priority fields right.

Currently we have this:

# /opt/vyatta/sbin/priority.pl | grep wireguard
459 interfaces/wireguard
460 interfaces/wireguard/node.tag/address # Run after interface has been configured
460 interfaces/wireguard/node.tag/mtu
920 interfaces/wireguard/node.tag/peer/node.tag/endpoint

What I can't recall is what the default priority is, if nothing is specified.

[jollyjinx]

You are right, correct priorities should fix this as peers get deleted after the interface.
How does that priority work ? Is there documentation on that ?

For me it would make most sense that for creation it works from trunk to nodes to leaves and priority is only interesting for nodes on the same level ?
And for deletion the other way around (from leaves to trunk) ?

Just guessing here. Where can I find documentation on that old template system ?

From what I've read by now on the old Vyatta system, it would look to me as if having one script to change everything in one place might be easier to maintain then having all the different values spread and changed in all the subnodes.
I will look into this configuration system as I would like to understand it better first.

[FossoresLP]

@jollyjinx Thank you a lot for trying to work this out and find a proper fix.
Looking at the priorities that Lochnair listed, I would think that we would need to give allowed-ips at least a priority higher that the interface itself.
Do you have a build environment to test this or should I create a branch with the priorities set so you can download the CI build from GitLab?
@Lochnair Thanks for jumping in, I really don't know a lot about vyatta.

[jollyjinx]

Thanx for the offer. I have a simple environment on which I can change something on my build machine and distribute that to my usg within seconds. So I could have fiddled with the priority as well.
I don't know what other implications priorities have on other occasions however. What if you add an address later or set or delete the mtu, bandwidth or whatever - are the priorities then right as well ? Are those bandwidth and constraints actually used anyways ?

Will try out a changed priority and see what happens.

[Lochnair]

@jollyjinx All I've ever been able to find are these:

• http://wiki.vyos.net/wiki/Configuration_mode_templates#priority:
• https://docs.vyos.io/en/latest/contributing/development.html#priorities

It's VyOS, but should still apply to the Vyatta variant in EdgeOS.

Nodes can have a priority, and on system bootup - or any other commit to the config all scripts are executed from lowest to highest priority.

Based on what you found, it does go the other way around.

What I'd be curious to try - is removing all the scripting currently in the templates, add print statements everywhere and then fire up a full configuration to see what order everything happens in. Also trying to add/remove peers afterwards, deleting the interface and so forth.

One could then also tweak the priorities until the correct order is achieved.

[Lochnair]

@FossoresLP Not that I know too much either to be fair. Dealing with the templates usually leads to my face meeting the desk.

Oh and @jollyjinx, I did do some preliminary work on moving to a single script file (old issue thread here, but I wasn't up to doing it by myself, so nothing came of it.

[jollyjinx]

@Lochnair Hehe, same documentation I found ;-( I also found the following useful:

https://community.ui.com/questions/How-to-create-a-Vyatta-EdgeOS-package-step-by-step/9092e149-3fd8-436b-82b7-e80c2d10fa4d
https://help.ui.com/hc/en-us/articles/204976224-EdgeRouter-Add-Commands-to-EdgeOS
https://github.com/vyos/vyatta-cfg-system/pull/19/files
https://blog.vyos.io/vyos-2-dot-0-development-digest-number-1
https://blog.vyos.io/vyos-2-dot-0-development-digest-number-2

The print statements is exactly what I've been doing to find a the bug in the first place. I've created a debug branch (here on GitHub so we don't have to duplicate work) with exactly that you can see the order of things.

I've played now with the priorities by setting the peer/node.def higher, but then route-allow get's created before the interface address exists. I think the current system is too clumsy and should be replaced with something at the end of the commit to create a new wireguard configuration from all the nodes below wireguard/node.def.
A way to achieve that is to call eval "$VAR(../wireguard-command/@)" or something.

[jollyjinx]

@Lochnair so you already thought of that as well moving it to a seperate script. Great to hear that !

[jollyjinx]

@FossoresLP I've changed my original pull request to no longer ignore the error but to only delete routes that do exist.
I've successfully tested that with

create interfaces... delete interfaces wireguard wg0 delete interfaces wireguard wg0 peer .. set interfaces wireguard wg0 route-allowed-ips true/false
as well as having a wg1 configured with no routes to also get successfully created/deleted.

[FossoresLP]

@jollyjinx Thank you, I will do some quick tests myself and merge this if everything works as expected.
Should be in the next couple of days. The release later today will not include these changes, yet.

[jollyjinx]

Good to hear :-) I might look into moving everything into one file in the future. It would make it easier to maintain and probably quicker as well. My usg takes ages to commit things.

[karog]

Over the last few days, I have been looking at the same issues. I wanted to add a config-dir option that would point to a dir (under /config) containing a standard wg config file named <interface>.conf using the interface defined at the wireguard node. And as for a single script as mentioned up thread, I have been using wg-quick (why reinvent the wheel). But wg-quick is not contained in the distribution so I request that it be added and placed in /usr/bin where wg is found. For now I just copied it from a linux machine. On commit, the conf file can be copied to /etc/wireguard where wg-quick expects it but be but preserved under /config for firmware updates.

Then I thought I would try to develop an alternate vyatta config design (I am brand new to vyatta config coding) that could take a standard config as mentioned above and use it to populate the vyatta config and maintain copies both in /etc/wireguard as well as in any dir pointed to by config-dir (if it exists). Config changes made and committed in vyatta could create/modify the standard config in both dirs (if the config-dir option is defined) all the while using wg-quick to do the processing.

Before I came to this design idea, I tried integrating into the existing vyatta config a config-file option as in the openvpn interface and process it with wg-quick. I got that working and commit is way faster. But there are some lingering issues like custom MTU values in the std config get overwritten by even the default MTU value (when no value has been specified) from the vyatta config because of order processing. And current vyatta config only supports up-command (PostUp) and down-command (PreDown) but not PreUp nor PostDown. And there is no option for custom routing tables though I did see some orphan code for that. All of these issues would go away in the design above.

Thoughts?

[FossoresLP]

@karog Would be great to have the configuration switched to a script. Give it a shot if you have the time and open a PR if you build something sensible.
The vyatta configuration is a major feature of this package so anything that breaks it will likely not get merged.
For the same reason I will not include wg-quick with the current configuration model so that we do not have two incompatible configuration systems. It's easy enough to copy over if you need it.
To discuss this further please take a look at #20. I think it's a better fit.

[karog]

@karog Would be great to have the configuration switched to a script. Give it a shot if you have the time and open a PR if you build something sensible.
To discuss this further please take a look at #20. I think it's a better fit.

Ok, I will see what I can come up with and report in #20. And I try very hard never to break anything.

[whiskerz007]

I believe #55 will resolve this problem.

[FossoresLP]

@jollyjinx I'd like to apologize for the way I handled this PR.
Thank you for your contribution. I am very sorry I never managed to merge it even though I told you I would.
I was not set up to take external contributions at the time.
Since this PR is now obsolete and the underlying issue should be fixed by #55 I am closing this.

[jollyjinx]

not a problem it was something I did for my setup back then. I no longer use wireguard on my unifi devices any more. I keep them clean now as Ubiquiti clearly want's unifi devices to stay clean and only allows customisation on the edgerouter setup. I was good to have this workaround as long as it lasted - I moved my wireguard to raspberry so it's no longer encumbering the unifi stuff ;-|