-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
wireguard, linuxnodehandler: untangle WireGuard agent from the linux node handler #25419
wireguard, linuxnodehandler: untangle WireGuard agent from the linux node handler #25419
Conversation
e1dc7b4
to
183d611
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I'm assuming this is blocked on the subscriber interface refactoring, but overall the approach looks sound to me.
183d611
to
bb7a3f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebased on top of the NodeHandler
interface changes in #25450, other comments addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending Sebastian's comments
bb7a3f4
to
2523387
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
2523387
to
00d797f
Compare
Afaik, the reason the WireGuard node event handling was contained within the
linuxNodeHandler
code had to do with routing (and that IPSec did it this way). That dependency appears to be gone and entangling the two leads to a deadlock, as diagnosed in #24574.This patch thus implements
NodeHandler
for the WireGuard agent, untangling it from thelinuxNodeHandler
. The wait cycle of the deadlock is thus broken, as thelinuxNodeHandler
doesn't otherwise acquire the IPCache lock while holding its lock.From the perspective of the agent, the invocations of the callbacks change insofar that previously, only once the
linuxNodeHandler
considered itself initialised, it would forward node events. Specifically, this excluded the intial sync of nodes performed on subscribe. However, I didn't see a reason to specifically replicate this behaviour, but I'm open to feedback on that.Fixes: #24574