v1.14 Author backport of #30423 #30534
Merged
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
gandro
added
kind/backports
[ upstream commit 9cd56a2 ] WireGuard-based encryption uses different values when it comes to the EncryptKey field in the IPCache: - For endpoints, the EncryptKey should always be non-zero if pod-to-pod encryption is enabled. - For nodes, the EncryptKey should be non-zero if node-to-node encryption is enabled, and if the node has not opted out of node-to-node encryption. Before this commit, we were deriving the EncryptKey of endpoints written to the kvstore IPCache (see `runIPIdentitySync`) based on the node's EncryptKey - which is the wrong source of truth for that value. Luckily, due to a bug, it is currently not possible for Cilium nodes running in kvstore mode to opt-out of node-to-node encryption, so the value was always effectively non-zero and the result was accidentally correct. However, as we want to fix the node-to-node opt-out mechanism in a subsequent commit, we ought to fix that first. Therefore, this commit fixes up all call sites which set an endpoint's EncryptKey to use the same source of truth. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 42910d4 ] This removes an unnecessary if condition in `GetEndpointEncryptKeyIndex`. If WireGuard is enabled, the WireGuard agent (see `pkg/wireguard/agent.NewAgent`) always sets a public key for the local node. Therefore this if condition is always true and we currently have no plans for nodes to have WireGuard enabled without them also producing a public key. Therefore this commit removes that superfluous condition. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 230bdd9 ] This commit removes the localNodeStore field from the WireGuard agent, as is it not accessed outside of the constructor. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 930d442 ] This commit fixes an issue where the node's EncryptKey field in the kvstore was not set to zero for nodes which opted out of node-to-node encryption, thereby effectively encrypting traffic for nodes which should not have been encrypted. This was due to the fact that the logic zeroing the local node's EncryptKey based on `localNode.OptOutNodeEncryption` was only performed when writing to the K8s CiliumNode CRD (in `NodeDiscovery.mutateNodeResource`), but not when writing to the kvstore (in the controller started by `NodeDiscovery.updateLocalNode`). This commit fixes this issue by zeroing `localNode.EncryptKey` directly at the source of truth in the `LocalNodeStore`. In addition, we need to be careful when that value is being written: - To avoid flapping values, the EncryptKey value needs to be initialized before we publish the local node object to the kvstore or k8s. Therefore, initialization of the EncryptKey field needs to happen before `LocalNodeSynchronizer.InitLocalNode` returns. - In order to determine if the local node has to opt out of node-to-node encryption, we need to know the local node's labels. Those are only available after `localNodeSynchronizer.initFromK8s` has been called - therefore we cannot set the field in a hive constructor like we do for some other fields that need to be initialized early. Therefore, this commit moves the initialization of the WireGuard related fields into `LocalNodeSynchronizer.InitLocalNode`. This satisfies the the above constraints for the `EncryptKey` and `OptOutNodeEncryption` values. The initialization of `WireguardPubKey` and the accompanying annotation can be performed earlier without loss of correctness, but to keep all local node initialization in the same place, we now also write those fields during `InitLocalNode`. This is safe, because `InitLocalNode` happens before the K8s/kvstore node object is published. This commit incidentally also fixes a bug where the `OptOutNodeEncryption` field in the WireGuard agent was read out too early, i.e. before `InitLocalNode` had a chance to initialize it. That bug had little consequence, as it only materialized in the `cilium status` not reporting the opt-out status correctly, and caused node IPs to be unnecessarily added to the WireGuard peer list. Both issues did not affect the encrypted traffic. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit d4be9e8 ] WireGuard-based encryption uses different values when it comes to the EncryptKey field in the IPCache: - For endpoints, the EncryptKey should always be non-zero if pod-to-pod encryption is enabled. - For nodes, the EncryptKey should be non-zero if node-to-node encryption is enabled, and if the node has not opted out of node-to-node encryption. When creating IPCache entries for regular endpoints, the EncryptKey value is taken from either the CEP/CES Kubernetes custom resource, or from the IP entry in the kvstore. However, the IPCache entries for the health and ingress endpoints are dervied from the node resource. Before this commit, those entries therefore also used the node's EncryptKey. When a node opted out of node-to-node encryption however, that meant that we did not encrypt traffic for health and ingress to those nodes. Thus, this commit works around that issue by ignoring the node's EncryptKey for the health and ingress endpoints and instead always encrypt that traffic (which is what we do for regular endpoints with WireGuard encryption already). This commit can be considered a workaround. Ideally, we would have separate fields in the node resource for the node and its special endpoints. That however is a larger schema change, thus this commit focuses on backportable changes. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
force-pushed
the
pr/v1.14-backport-2024-01-30
branch
from
101313f
to
8d29f83
Compare
|
/test-backport-1.14 |
|
I've manually tested this branch with kvstore encryption locally, it seems to work as advertised. |
brb
approved these changes
Jan 31, 2024
julianwiedmann
approved these changes
Jan 31, 2024
maintainer-s-little-helper
bot
added
the
ready-to-merge
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, a GitHub action will update the labels of these PRs: