Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

datapath: link-local unicast addresses can be "host" #25298

Merged
merged 1 commit into from May 10, 2023

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 This issue will prevent the release of the next version of Cilium. backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. sig/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 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
@asauber asauber added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label May 10, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 May 10, 2023
@asauber asauber removed backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels 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
57 checks passed
@michi-covalent michi-covalent deleted the pr/asauber/link-local-host branch May 10, 2023 17:52
@asauber asauber added the release-blocker/1.13 This issue will prevent the release of the next version of Cilium. label May 10, 2023
@thorn3r thorn3r mentioned this pull request May 10, 2023
2 tasks
@thorn3r thorn3r added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels May 10, 2023
@thorn3r thorn3r added this to Backport pending to v1.13 in 1.13.4 May 17, 2023
@thorn3r thorn3r removed this from Needs backport from main in 1.13.3 May 17, 2023
@qmonnet qmonnet moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.4 Jun 9, 2023
@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 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 10, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.13 in 1.13.3 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 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-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.13.3
Backport done to v1.13
1.13.4
Backport done to v1.13
6 participants