Skip to content

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Nov 22, 2019

Fixes fib lookup case for IPv6 and ensures when disabling encryption remaining state is remove.


This change is Reviewable

We do not currently remove the 'dir in' encryption rules when encryption
is disabled. The original thinking is we could simply leave these xfrm
policy/state around because they would only ever be triggered if the
datapath marked packets for encryption which it wouldn't because encryption
was disabled. Then if encryption was ever (re)enabled the rules would
already be there. But, subtle detail if the cilium_host IP changes
after encryption is disabled then it (re)enabled with a new IP the
'dir in' state/policy in the xfrm table will no longer be valid. And
if the new cilium host IP is in the same CIDR we can end up with
a collision in the table and possible use old out of date rules.

Fix by removing any 'dir in' rules when encryption is disabled.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab jrfastab requested a review from a team November 22, 2019 17:47
@jrfastab
Copy link
Contributor Author

test-me-please

When we added FIB support we only added it for IPv4. This adds support
IPv6 and fixes an issue where IPv6 packets were being dropped due to
fib lookup failing.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab
Copy link
Contributor Author

test-me-please

@coveralls
Copy link

coveralls commented Nov 22, 2019

Coverage Status

Coverage decreased (-0.06%) to 45.749% when pulling 77e7f17 on ci-fixes-v2 into 9975bba on master.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked over this, other than the extra debug statements around the place (which I think we should probably handle through monitor instead), it looks good.

@jrfastab
Copy link
Contributor Author

test-me-please

@jrfastab
Copy link
Contributor Author

pushed update with printing dropped and some consolidation of #define if foo else blah logic

@jrfastab jrfastab added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Nov 26, 2019
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Nov 26, 2019
@aanm aanm merged commit 82d394f into master Nov 26, 2019
@aanm aanm deleted the ci-fixes-v2 branch November 26, 2019 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants