wireguard: restore allowedIPs upon agent restart #34095
Merged
Conversation
This file contains hidden or 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
giorio94
commented
Jul 30, 2024
|
/test |
2 tasks
4e8ac12 to
8736161
Compare
|
/test |
asauber
reviewed
Aug 21, 2024
8736161 to
85e5114
Compare
|
Rebased and fix a conflict with #34373 |
|
/test |
2 similar comments
|
/test |
|
/test |
asauber
approved these changes
Aug 22, 2024
gandro
approved these changes
Sep 3, 2024
85e5114 to
73df40a
Compare
Currently, temporary connection disruption can occur on agent restart when Cilium is configured in native routing mode, and WireGuard encryption is enabled, because the list of AllowedIPs gets recreated from scratch upon the reception of the node event for each given remote node, possibly removing entries for valid endpoints that have not yet been discovered at that point through the CiliumEndpoint CRD or the corresponding kvstore representation. This issue, instead, does not affect the current implementation in tunnel mode, as in that case we encrypt encapsulated traffic, which always has source and destination addresses corresponding to Node Internal IPs, that are immediately added as Allowed IPs. Let's prevent this issue restoring the list of Allowed IPs for each peer from the WireGuard state after agent restart and preserving them until ipcache synchronization has been completed. At that point, we do a final GC pass to clean up possible stale entries for pods that got removed while the given agent was down. Special logic is introduced to prevent a possible flipping behavior in case upon restore an allowed IP is associated with a given peer, but gets associated with a different one later on. Indeed, WireGuard enforces that any allowed IP is associated to at most a single peer. Fixes: 31979 Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
73df40a to
5ba874d
Compare
|
/test |
jrife
added a commit
to jrife/cilium
that referenced
this pull request
Sep 5, 2024
Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see ciliumGH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. In order to preserve the behavior introduced by ciliumGH-34095 ("wireguard: restore allowedIPs upon agent restart") this commit adds syncPeersWithDevice which removes any stale peers or allowed IPs on initialization. This commit mostly sidesteps the issue that ciliumGH-34095 set out to solve since we only ever explitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done in restoredPeers and restoredAllowedIPs. Fixes: cilium#33159 Signed-off-by: Jordan Rife <jrife@google.com>
jrife
added a commit
to jrife/cilium
that referenced
this pull request
Sep 5, 2024
Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see ciliumGH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. This commit mostly sidesteps the issue that ciliumGH-34095 ("wireguard: restore allowedIPs upong agent restart") set out to solve since we only ever explicitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done by restoredPeers and restoredAllowedIPs. However, we retain the behavior in RestoreFinished() that removes stale peers and allowed IPs. Fixes: cilium#33159 Signed-off-by: Jordan Rife <jrife@google.com>
jrife
added a commit
to jrife/cilium
that referenced
this pull request
Sep 5, 2024
Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see ciliumGH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. This commit mostly sidesteps the issue that ciliumGH-34095 ("wireguard: restore allowedIPs upong agent restart") set out to solve since we only ever explicitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done by restoredPeers and restoredAllowedIPs. However, we retain the behavior in RestoreFinished() that removes stale peers and allowed IPs. Fixes: cilium#33159 Signed-off-by: Jordan Rife <jrife@google.com>
13 tasks
jrife
added a commit
to jrife/cilium
that referenced
this pull request
Sep 19, 2024
Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see ciliumGH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. This commit mostly sidesteps the issue that ciliumGH-34095 ("wireguard: restore allowedIPs upong agent restart") set out to solve since we only ever explicitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done by restoredPeers and restoredAllowedIPs. However, we retain the behavior in RestoreFinished() that removes stale peers and allowed IPs. Fixes: cilium#33159 Signed-off-by: Jordan Rife <jrife@google.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 26, 2024
Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see GH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. This commit mostly sidesteps the issue that GH-34095 ("wireguard: restore allowedIPs upong agent restart") set out to solve since we only ever explicitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done by restoredPeers and restoredAllowedIPs. However, we retain the behavior in RestoreFinished() that removes stale peers and allowed IPs. Fixes: #33159 Signed-off-by: Jordan Rife <jrife@google.com>
jibi
pushed a commit
that referenced
this pull request
Sep 30, 2024
[ upstream commit 3ebfa4d ] [ backporter's note: minor conflict in agent_test.go due to missing iter packeage in go 1.22 ] Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see GH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. This commit mostly sidesteps the issue that GH-34095 ("wireguard: restore allowedIPs upong agent restart") set out to solve since we only ever explicitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done by restoredPeers and restoredAllowedIPs. However, we retain the behavior in RestoreFinished() that removes stale peers and allowed IPs. Fixes: #33159 Signed-off-by: Jordan Rife <jrife@google.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 1, 2024
[ upstream commit 3ebfa4d ] [ backporter's note: minor conflict in agent_test.go due to missing iter packeage in go 1.22 ] Recent flakes in the "Network performance GKE" tests, in particular those that use wireguard, uncovered an issue where updates to a peer's allowed IPs can cause connectivity issues. In the best case this may just result in dropped packets while in the worst case may lead to failed calls to sendto/sendmsg (see GH-33159 for more detail on some of the symptoms observed while running netperf). This issue happens when ReplaceAllowedIPs is set, as it causes the wireguard driver to completely clear the set of allowed IPs for a peer before the set is rebuilt. This leaves a gap where packets sent or received at that time will appear to not have a peer associated with them. The current implementation sets ReplaceAllowedIPs every time updatePeerByConfig is called, but we can avoid ever needing to use it. This commit uses a "dummy peer" as a way to remove allowed IPs from a peer rather than using ReplaceAllowedIPs. We use peerConfig to track any pending IP inserts and removals and split these into two different sets of updates in updatePeerByConfig. The first call to ConfigureDevice() adds any IPs that are pending insertion. Next, any IPs that are pending removal are moved to the "dummy peer" before removing the dummy peer altogether, the net effect being that the IP is removed from the original peer's list of allowed IPs. This commit mostly sidesteps the issue that GH-34095 ("wireguard: restore allowedIPs upong agent restart") set out to solve since we only ever explicitly remove allowed IPs, meaning we can do away with a lot of the accounting previously done by restoredPeers and restoredAllowedIPs. However, we retain the behavior in RestoreFinished() that removes stale peers and allowed IPs. Fixes: #33159 Signed-off-by: Jordan Rife <jrife@google.com>
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.
Currently, temporary connection disruption can occur on agent restart when Cilium is configured in native routing mode, and WireGuard encryption is enabled, because the list of AllowedIPs gets recreated from scratch upon the reception of the node event for each given remote node, possibly removing entries for valid endpoints that have not yet been discovered at that point through the CiliumEndpoint CRD or the corresponding kvstore representation. This issue, instead, does not affect the current implementation in tunnel mode, as in that case we encrypt encapsulated traffic, which always has source and destination addresses corresponding to Node Internal IPs, that are immediately added as Allowed IPs.
Let's prevent this issue restoring the list of Allowed IPs for each peer from the WireGuard state after agent restart and preserving them until ipcache synchronization has been completed. At that point, we do a final GC pass to clean up possible stale entries for pods that got removed while the given agent was down. Special logic is introduced
to prevent a possible flipping behavior in case upon restore an allowed IP is associated with a given peer, but gets associated with a different one later on. Indeed, WireGuard enforces that any allowed IP is associated to at most a single peer.
Fixes: #31979