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.

Sign up for GitHub

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

Jump to bottom

egressgw: add support for excludedCIDRs #23448

Merged
merged 12 commits into from Feb 20, 2023
Merged

egressgw: add support for excludedCIDRs #23448

merged 12 commits into from Feb 20, 2023

Conversation

jibi
Copy link
Member

@jibi jibi commented Jan 30, 2023 •
edited

Add support to the egress gateway manager for the new excludedCIDRs
policy property.

The implementation is based on the idea of adding, for each excluded
CIDR, "zero" entries to the egress policy map (i.e. entries with the
gateway IP set to 0.0.0.0). When traffic is then matched against these
entries in the datapath, it will skip altogether all the egress gateway
logic.

Fixes: #23002

@jibi jibi added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. feature/egress-gateway Impacts the egress IP gateway feature. sig/agent Cilium agent related. labels Jan 30, 2023
@jibi jibi force-pushed the pr/jibi/egressgw-exclude-cidr branch 2 times, most recently from d4f752e to 19a750b Compare January 30, 2023 14:23
@jibi
Copy link
Member Author

jibi commented Jan 30, 2023

/test-only --focus="K8sDatapathEgressGatewayTest.*" --k8s_version=1.26 --kernel_version=net-next

@jibi jibi force-pushed the pr/jibi/egressgw-exclude-cidr branch from 19a750b to 6e77966 Compare January 30, 2023 17:29
@jibi jibi changed the title egressgw: add support for excludeCIDRs egressgw: add support for excludedCIDRs Jan 30, 2023
@benesch
Copy link

benesch commented Jan 31, 2023

Really excited to see this, @jibi! 🎉

@jibi jibi force-pushed the pr/jibi/egressgw-exclude-cidr branch from 6e77966 to d340566 Compare January 31, 2023 11:03
@jibi jibi marked this pull request as ready for review January 31, 2023 11:04
@jibi jibi requested review from a team as code owners January 31, 2023 11:04
@jibi
Copy link
Member Author

jibi commented Jan 31, 2023 •
edited

Couple of TODO items:

  • add BPF unit tests. As for these I need first a generic test for redirection (we only test the SNAT path for now) based on how long it takes for this PR to get reviewed I may add them directly to this PR or open a new one
  • backport 7b2a750 to ensure we don't try to forward traffic to 0.0.0.0 gateways in case the agent is downgraded to an older version

@jibi jibi requested a review from pchaigno January 31, 2023 11:08
pchaigno
pchaigno approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Nice pull request (as usual 😃)!

My only concern is with the ip rules and routes as I'm not sure I understand the impact there.

Regarding downgrade, should we ensure we get the commit into v1.13 before we merge this? Or alternatively, create a release blocker issue to ensure this PR doesn't end up in v1.14 before v1.13 is ready (in case something comes up and we forget to backport that commit)?
On the same topic, is this something we want in v1.13? (In which case we'll have to backport the commit to v1.12.)

pkg/k8s/apis/cilium.io/client/crds/v2/ciliumegressgatewaypolicies.yaml Outdated Show resolved Hide resolved
pkg/egressgateway/manager.go Outdated Show resolved Hide resolved
pkg/egressgateway/manager.go Outdated Show resolved Hide resolved
pkg/egressgateway/policy.go Show resolved Hide resolved
test/k8s/egress.go Outdated Show resolved Hide resolved
pkg/egressgateway/manager.go Show resolved Hide resolved
@pchaigno pchaigno self-requested a review January 31, 2023 14:47
@jibi jibi force-pushed the pr/jibi/egressgw-exclude-cidr branch 4 times, most recently from 2341c95 to 6610e66 Compare January 31, 2023 21:37
pchaigno
pchaigno approved these changes Feb 8, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Only minor comments from me, but the main commit (egressgw: add support for excludedCIDRs) could use some work to make it easier to follow. It's actually fairly simple changes if you already understand the main idea, but it's currently still hard to follow as is.

pkg/egressgateway/policy.go Show resolved Hide resolved
pkg/egressgateway/manager.go Show resolved Hide resolved
pkg/egressgateway/policy.go Outdated Show resolved Hide resolved
pkg/egressgateway/manager.go Show resolved Hide resolved
Documentation/network/egress-gateway.rst Show resolved Hide resolved
qmonnet
qmonnet approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

pkg/egressgateway/manager_privileged_test.go Outdated Show resolved Hide resolved
@lmb lmb mentioned this pull request Feb 15, 2023
Closed
4 tasks
@jibi jibi force-pushed the pr/jibi/egressgw-exclude-cidr branch from ab22fa1 to e77dd83 Compare February 20, 2023 09:58
@jibi
k8s: cegp: add excludedCIDRs field
305337f 
ExcludedCIDRs can be used in a CiliumEgressGatewayPolicy to express a
list of destination CIDRs which will be excluded from the redirection
and SNAT logic.

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
bpf: egressgw: handle excluded CIDRs case
cd377bb 
Handle the case where an egress policy has a gateway IP set to 0:
any packet matching such policy should not go through egress gateway
redirection/SNAT.

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
ip: export PartitionCIDR function
64d55a5 
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
egressgw: policy: parse excludedCIDRs from CEGP
49102a9 
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
egressgw: policy: add destinationMinusExcludedCIDRs helper
c9accb0 
which returns the actual destination CIDRs space for a given policy,
which is defined by the destination CIDRs minus the excluded ones.

A practical example may be useful here. Let's say a policy specifies:

* destination CIDRs: 1.1.1.0/24
* excluded CIDRs: 1.1.1.1/2

The effective CIDR space for such policy will be the difference between
the two 1.1.1.0/24 and 1.1.1.1/24 CIDRs, so:

	1.1.1.0/28
	1.1.1.16/30
	1.1.1.20/31
	1.1.1.23/32
	1.1.1.24/29
	1.1.1.32/27
	1.1.1.64/26
	1.1.1.128/25

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
egressgw: policy: add excluded CIDRs iterating/matching helpers
4d32c2d 
This commit renames the forEachEndpointAndDestination method to
forEachEndpointAndCIDR, to make it more clear that it iterates over all
(destination and excluded) CIDRs of the receiver policy.

Next, it introduces 2 new helpers that allow iterating and matching over
all the endpoints and destination CIDRs minus the excluded ones:

* forEachEndpointAndDestinations: works like forEachEndpointAndCIDR, but
  iterates over destinationCIDRs minus excludedCIDRs
* matchesMinusExcludedCIDRs: works like matches but matches over
  destinationCIDRs minus excludedCIDRs

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
egressgw: add support for excludedCIDRs
9403c76 
Add support to the egress gateway manager for the new excludedCIDRs
policy property.

The implementation is based on the idea of adding, for each excluded
CIDR, "zero" entries to the egress policy map (i.e. entries with the
gateway IP set to 0.0.0.0). When traffic is then matched against these
entries in the datapath, it will skip altogether all the egress gateway
logic.

Fixes: #23002
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
egressgw: manager: cosmetic cleanup
c5ef07e 
In the callback we use in removeUnusedIpRulesAndRoutes() to match an IP
rule to an egress gateway policy, separate the preconditions from the
actual matching predicate.

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
bpf: tests: add unit test for egress gateway redirection
110688b 
And rename existing test file to tc_egressgw_snat.c

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
bpf: tests: add unit test for egress gateway excluded CIDRs
7ae8554 
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
bpf: tests: use EGRESS_PREFIX_LEN macro
a51cc87 
to initialize the egress gateway policies' lpm keys.

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi
docs: egressgw: add section for excludedCIDRs
6cf9629 
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi jibi force-pushed the pr/jibi/egressgw-exclude-cidr branch from e77dd83 to 6cf9629 Compare February 20, 2023 11:45
@jibi
Copy link
Member Author

jibi commented Feb 20, 2023

/test

julianwiedmann
julianwiedmann approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@jibi
Copy link
Member Author

jibi commented Feb 20, 2023

/test-1.16-4.19

@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 Feb 20, 2023
@pchaigno pchaigno merged commit aaeb260 into master Feb 20, 2023
@pchaigno pchaigno deleted the pr/jibi/egressgw-exclude-cidr branch February 20, 2023 18:17
@mhofstetter mhofstetter mentioned this pull request Feb 21, 2023
Merged
@pchaigno pchaigno mentioned this pull request Feb 21, 2023
Merged
@tklauser tklauser added affects/v1.13 This issue affects v1.13 branch affects/v1.12 This issue affects v1.12 branch labels Mar 29, 2023
@jibi jibi mentioned this pull request Apr 13, 2023
Merged
@M4t7e M4t7e mentioned this pull request Aug 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@nebril nebril nebril approved these changes

@qmonnet qmonnet qmonnet approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch feature/egress-gateway Impacts the egress IP gateway feature. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/agent Cilium agent related. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Egress Gateway destinationCIDRs exclusions
8 participants
@jibi @benesch @nathanjsweet @nebril @pchaigno @qmonnet @julianwiedmann @tklauser