wireguard: Reinitialize peer when public key changes #35750
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
3 tasks
5307ea3 to
2a7a694
Compare
|
/test |
80472cd to
484baae
Compare
When a node's public key changes, as is the case after a node restart, all other nodes delete the peer entry for the old key and create a new peer entry for the new key. However, the logic introduced by commit 3ebfa4d ("wireguard: Use dummy peer for allowed IP removal") fails to reinitialize the allowed IPs state for the new entry, namely the node IPs. TestAgent_AllowedIPsRestoration actually did have test coverage for this scenario, but passed with a false positive since k8s2PubKey and k8s2PubKey2 were not different enough it seems. Setting k8s2PubKey2 to a completely different key makes the unit test fail as it should have. Reinitialize peer inside UpdatePeer() whenever a node's public key changes to trigger a full resync of node and pod IPs for the new key, and ensure that TestAgent_AllowedIPsRestoration properly tests for this scenario. Fixes: cilium#35644 Fixes: cilium#34612 Fixes: 3ebfa4d ("wireguard: Use dummy peer for allowed IP removal") Signed-off-by: Jordan Rife <jrife@google.com>
484baae to
39d463f
Compare
jschwinger233
approved these changes
Nov 6, 2024
|
/test |
|
Given that #34612 was backported to v1.16, also adding the label here. |
13 tasks
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.
When a node's public key changes, as is the case after a node restart, all other nodes delete the peer entry for the old key and create a new peer entry for the new key. However, the logic introduced by commit 3ebfa4d ("wireguard: Use dummy peer for allowed IP removal") fails to reinitialize the allowed IPs state for the new entry, namely the node IPs.
TestAgent_AllowedIPsRestorationactually did have test coverage for this scenario, but passed with a false positive sincek8s2PubKeyandk8s2PubKey2were not different enough it seems. Settingk8s2PubKey2to a completely different key makes the unit test fail as it should have.Reinitialize peer inside
UpdatePeer()whenever a node's public key changes to trigger a full resync of node and pod IPs for the new key, and ensure thatTestAgent_AllowedIPsRestorationproperly tests for this scenario.Testing
cilium install --set image.override=$MY_REGISTRY/cilium:latest --set image.pullPolicy=Always --set operator.image.override=$MY_REGISTRY/operator-generic:latest --set operator.image.pullPolicy=Always --set operator.image.suffix="" --set encryption.enabled=true --set encryption.type=wireguard --set encryption.nodeEncryption=true --set kubeProxyReplacement=true --set bpf.masquerade=true --set cluster.name=jrifewg showandcilium statusfor the node that was rebooted.Before This Fix
wg showshows the public key for node10.13.106.162change and does not resync the node IP(s) until an agent restart. The output below is before and after the node in question reboots.cilium statusshows 1/2 nodes are reachable.After This Fix
wg showshows the public key for node10.13.106.162change and a full set of allowed IPs, including the node IPs. The output below is before and after the node in question reboots.cilium statusshows 2/2 nodes are reachable.Fixes: #35644
Fixes: #34612
Fixes: 3ebfa4d ("wireguard: Use dummy peer for allowed IP removal")