[v1.9] pkg/datapath/linux: Fix asymmetric IPsec logic on delete #18847
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
maintainer-s-little-helper
bot
added
backport/1.9
kind/backports
christarazi
changed the title
[ upstream commit a6e8477 ] Document the intent of NodeValidateImplementation(). Signed-off-by: Chris Tarazi <chris@isovalent.com>
[ upstream commit 7e5022e ] These if-statements are unnecessary because upon code analysis, we can tell that it's not possible for the input to be nil. Remove these statements to simplify the flow of the function. In other words, now we know for a fact that calling these function will result in a route insert. Signed-off-by: Chris Tarazi <chris@isovalent.com>
[ upstream commit e41aea0 ] This helps in scenarios where the user reports this log msg, but we are missing the actual CIDR from the route that failed to be deleted or created. Signed-off-by: Chris Tarazi <chris@isovalent.com>
[ upstream commit 0bd4e04 ] With ENI IPAM mode and IPsec enabled, users were reporting cases where connectivity to particular pods breaks, and correlated with those drops, the following error msg: ``` Unable to delete the IPsec route OUT from the host routing table ``` In addition, it was also reported that the connectivity outage would only last for a few minutes before resolving itself. The issue turned out to be that upon node deletion, the logic to handle the IPsec cleanup is asymmetric with the IPsec logic to handle a node create / update. Here's how: * With ENI mode and IPsec, subnet encryption mode is enabled implicitly. * Background: Users can explicitly enable subnet encryption mode by configuring `--ipv4-pod-subnets=[cidr1,cidr2,...]`. * Background: ENIs are part of subnet(s). * Cilium with ENI mode automatically appends the node's ENIs' subnets' CIDRs to this slice. * For example, node A has ENI E which is a part of subnet S with CIDR C. Therefore, `--ipv4-pod-subnets=[C]`. * This means that each node should have an IPsec OUT routes for each pod subnet, i.e. each ENI's subnet, as shown by (*linuxNodeHandler).nodeUpdate() which contains the IPsec logic on a node create / update. * Upon a node delete [(*linuxNodeHandler).nodeDelete()], we clean up the "old" node. When it gets to the IPsec logic, it removes the routes for the pod subnets as well, i.e. removes the route to the ENI's subnet from the local node. From the example above, it'd remove the route for CIDR C. * This is problematic because in ENI mode, different nodes can share the same ENI's subnet, meaning subnets are NOT exclusive to a node. For example, a node B can also have ENI E with a subnet C attached to it. * As for how the nodes were fixing themselves, it turns out that (*Manager).backgroundSync() runs on an interval which calls NodeValidateImplementation() which calls down to (*linuxNodeHandler).nodeUpdate() thereby running the IPsec logic of a node create / update which reinstates the missing routes. Therefore, we shouldn't be deleting these routes because pods might still be relying on them. By comparing the IPsec delete logic with [1], we see that they're asymmetric. This commit fixes this asymmetry. [1]: Given subnetEncryption=true, notice how we only call enableSubnetIPsec() if the node is local. That is not the case on node delete. ``` func (n *linuxNodeHandler) nodeUpdate(oldNode, newNode *nodeTypes.Node, firstAddition bool) error { ... if n.nodeConfig.EnableIPSec && !n.subnetEncryption() && !n.nodeConfig.EncryptNode { n.enableIPsec(newNode) newKey = newNode.EncryptionKey } ... if n.nodeConfig.EnableIPSec && !n.subnetEncryption() { n.encryptNode(newNode) } if newNode.IsLocal() { isLocalNode = true ... if n.subnetEncryption() { n.enableSubnetIPsec(n.nodeConfig.IPv4PodSubnets, n.nodeConfig.IPv6PodSubnets) } ... return nil } ``` Fixes: 645de9d ("cilium: remove encryption route and rules if crypto is disabled") Co-authored-by: John Fastabend <john@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>
christarazi
force-pushed
the
pr/christarazi/v1.9-backport-18827
branch
from
7c4a641
to
83c0b2a
Compare
|
/test-backport-1.9 |
joestringer
approved these changes
Feb 21, 2022
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