Skip to content

Conversation

asauber
Copy link
Member

@asauber asauber commented May 5, 2023

Background

This is a fix for a regression in the local addresses logic, introduced in 080857b as part of the implementation for AddressScopeMax. Addresses with the form of a link-local unicast address began to be filtered out of the local address aggregation, causing them to labeled with the "world" identity for the sake of policy enforcement. Examples of such addresses include:

169.254.10.10
fe80::1234

This caused issues for a variety of users, whose policies allowing "host" traffic would no longer allow traffic to these addresses, forcing the use of workarounds involving CIDR policies, which is not the intended behavior for this type of address. This was a regression as of Cilium 1.12.0-rc2. One reason for this regression was that logic prior to the change looked at the address scope, whereas logic after the change looked at the address bytes, and it was found that many users had assigned addresses of the forms above but with scope global, causing them to again be filtered out unconditionally.

Implementation

This patch factors out the local address filtering logic into a function, removes the skip over IsLinkLocalUnicast(), and adds a variety of unit tests for that function.

Important Note

This change has no effect on link-local services such as the AWS Metadata service which operates on 169.254.169.254/32 because that address is not assigned to any interface of the local host and thus is not enumerated by rnetlink. The address is still labeled with the "world" identity as usual. This has been tested on EKS and kind.

Fixes

fixes: #25242
fixes: #23910
fixes: #16308
fixes: #20055

Fix a regression in which link-local addresses were not treated with the "host" identity in some circumstances.

@asauber asauber requested a review from a team as a code owner May 5, 2023 23:09
@asauber asauber requested a review from gentoo-root May 5, 2023 23:09
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 5, 2023
@asauber asauber added release-blocker/1.14 kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. affects/v1.12 This issue affects v1.12 branch and removed affects/v1.12 This issue affects v1.12 branch kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. labels May 5, 2023
@asauber asauber force-pushed the pr/asauber/link-local-host branch from 48e2d1c to 8989ab2 Compare May 5, 2023 23:35
@asauber asauber removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 5, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 5, 2023
@asauber asauber added the release-note/bug This PR fixes an issue in a previous release of Cilium. label May 5, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 5, 2023
@asauber asauber requested review from borkmann and joestringer May 5, 2023 23:38
@asauber asauber force-pushed the pr/asauber/link-local-host branch from 8989ab2 to c2e3610 Compare May 5, 2023 23:50
@asauber asauber changed the title datapath: link-local unicast addresses are "host" datapath: link-local unicast addresses can be "host" May 6, 2023
@michi-covalent
Copy link
Contributor

/test

@asauber
Copy link
Member Author

asauber commented May 8, 2023

/test-1.26-net-next

This is a fix for a regression in the local addresses logic, introduced
in 080857b as part of the
implementation for AddressScopeMax. Addresses with the form of
link-local unicast addresses began to be filtered out of the local
address aggregation, causing them to labeled with the "world" identity
for the sake of policy enforcement. Examples of such addresses include:

169.254.10.10
fe80::1234

This caused issues for a variety of users, whose policies allowing
"host" traffic would no longer allow traffic to these addresses, forcing
the use of workarounds involving CIDR policies, which is not the
intended behavior for this type of address. This was a regression as of
Cilium 1.12.0-rc2. One reason for this regression was that logic prior
to the change looked at the address scope, whereas logic after the
change looked at the address bytes, and it was found that many users had
assigned addresses of the forms above but with scope global, causing
them to again be filtered unconditionally.

This patch factors out the local address filtering logic into a
function, removes the skip over IsLinkLocalUnicast(), and adds a variety
of unit tests for that function.

fixes: #25242
fixes: #23910
fixes: #16308
fixes: #20055

Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com>
@asauber asauber force-pushed the pr/asauber/link-local-host branch from c2e3610 to dac638e Compare May 10, 2023 14:06
@asauber
Copy link
Member Author

asauber commented May 10, 2023

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 10, 2023
@jrajahalme jrajahalme added the affects/v1.12 This issue affects v1.12 branch label May 10, 2023
@michi-covalent michi-covalent added the affects/v1.13 This issue affects v1.13 branch label May 10, 2023
@michi-covalent michi-covalent merged commit 05b6d82 into main May 10, 2023
@michi-covalent michi-covalent deleted the pr/asauber/link-local-host branch May 10, 2023 17:52
@thorn3r thorn3r mentioned this pull request May 10, 2023
2 tasks
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 labels Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
Status: Released
6 participants