[v1.13] Author backports of Node Handler for wireguard, and dependent PR #25923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ upstream commit d8a0be6 ] [backporter notes: fuzz_test doesn't exist yet] Return pointer to implementing struct instead of implemented interface from the constructor, as is commonly considered idiomatic Go. The constructor is already in a linux-specific package. To ensure that we really do implement the interface we want, add a static type check in form of a variable assignment. As a side-effect, this allows us to drop a number of type assertions in tests. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 8294624 ] [ backporter notes: conflicts due to moved files, #25284 not yet backported, hence no GetNodeIP() to move.] The NodeHandler interface is too large, as can be seen in various implementations which are only implementing a subset of methods. This patch splits off the NodeID handling part, and removes the stub methods from noop implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 1aa06c6 ] [ backporter notes: conflics due to moved files ] The NodeHandler interface still contains two different concepts. Remove the NodeNeighbor discovery/handling from it, and delete the different stub implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit c8598f8 ] [backporter notes: subscribing to node discovery works differently and import path for LocalNodeConfig changed] The reason the WireGuard agent node event handling was contained within the linuxNodeHandler code was routing, which is no longer the case. In addition, entangling the two leads to a deadlock, as diagnosed in GitHub issue #24574. This patch thus implements NodeHandler for the WireGuard agent, and subscribes to the NodeManager itself. That way, the wait cycle of the deadlock is broken, as the linuxNodeHandler doesn't 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 initial sync of nodes performed on subscribe. However, I didn't see a reason to specifically replicate this behaviour. Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
bimmlerd
added
kind/backports
bimmlerd
changed the title
gandro
approved these changes
Jun 6, 2023
|
/test-backport-1.13 |
|
CI triage:
|
|
/test-runtime |
michi-covalent
approved these changes
Jun 9, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
This was referenced Jun 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Once this PR is merged, you can update the PR labels via:
or with