Bump CIDR limit on older kernels. #3119

joestringer · 2018-03-13T01:19:12Z

Support a larger number of CIDR prefixes when running on older kernels. Now limited by the number of unique prefix lengths in the policies for an endpoint, which should be less than forty.

houndci-bot · 2018-03-13T01:19:23Z

pkg/policy/l3.go

 		} else {
-			m.IPv4Count++
+			m.IPv4PrefixCount[ones] += 1


should replace m.IPv4PrefixCount[ones] += 1 with m.IPv4PrefixCount[ones]++

houndci-bot · 2018-03-13T01:19:23Z

pkg/policy/l3.go

@@ -74,10 +74,16 @@ func (m *CIDRPolicyMap) Insert(cidr string, ruleLabels labels.LabelArray) int {
 	if _, found := m.Map[key]; !found {
 		m.Map[key] = &CIDRPolicyMapRule{Prefix: *ipnet, DerivedFromRules: labels.LabelArrayList{ruleLabels}}
 		if ipnet.IP.To4() == nil {
-			m.IPv6Count++
+			m.IPv6PrefixCount[ones] += 1


should replace m.IPv6PrefixCount[ones] += 1 with m.IPv6PrefixCount[ones]++

rlenglet · 2018-03-13T05:06:09Z

pkg/policy/api/rule_validation.go

+
+	// FIXME GH-1781 count coalesced CIDRs and restrict the number of
+	// prefix lengths based on the CIDRSet exclusions.
+	if l := len(prefixLengths); l > MaxCIDRPrefixLengths {


I think this is not particularly useful, because the number that matters is the number of CIDRs across all the rules relevant for each specific endpoint.
It's OK to check here anyway.

Do you think we should just drop this validation and rely on the generation errors propagating to endpoint log?

We can keep it here, there's also checking when L3Policy is Validate()d just before attempting to push it into BPF.

rlenglet · 2018-03-13T05:08:37Z

pkg/policy/l3.go

+		}
+
+		if len(m.IPv6PrefixCount) > api.MaxCIDRPrefixLengths {
+			log.Infof("Inserting CIDR with prefix length %d/%d, generation may fail",


Info level seems very noisy a priori. Change to Debug?

Can't you return an error and add expose that through the endpoint's log?

Not from here, this is during CIDRPolicyMap construction which is divorced from endpoints. I could signal it when we attempt to apply the policy to the endpoint and generate the BPF.

Please file a GH to clean this up. It's not ideal but I don't want to hold up this PR.

I removed this log.

rlenglet · 2018-03-13T05:10:45Z

pkg/policy/l3_test.go

@@ -0,0 +1,40 @@
+// Copyright 2016-2017 Authors of Cilium


This test needs rethinking with the upcoming expansion of CIDR limits. Remove it for now. We should be able to achieve this with just unit testing. Signed-off-by: Joe Stringer <joe@covalent.io>

Signed-off-by: Joe Stringer <joe@covalent.io>

On older kernels that do not support the BPF_MAP_TYPE_LPM, we have previously supported defining an array of up to about forty CIDRs for which the datapath would allow traffic. However, if a user wished to write CIDR policies with more than this number of possible CIDR matches, then the only option to support this would be to upgrade the kernel. This commit relaxes this constraint by replacing the array-based implementation in the BPF datapath with a hashtable-based approach instead. The entries in the hash table start with a 32-bit unsigned integer indicating the prefix length followed by the CIDR with a zeroed suffix. The implementation iterates through CIDR{4,6}_{E,IN}GRESS_PREFIXES to perform the lookup, so if there are many CIDRs but they all use the same prefix it will only require one lookup. This list of prefix lengths is expected to be ordered from longest prefix to shortest. With datapath complexity and clang-7.0 it appears that up to about ninety prefix lengths with any number of CIDRs (defined by LPM_MAP_SIZE) may be supported while also supporting the rest of the datapath logic. However, when upcoming CIDR-dependent L4 matching support is added this is expected to drop, so for the moment restrict configuration of CIDRs with more than 40 prefix lengths. This should cover all IPv4 cases, and the majority of IPv6 use cases for the moment. Based on outline from Jarno Rajahalme. Signed-off-by: Joe Stringer <joe@covalent.io>

joestringer · 2018-03-13T21:33:30Z

test-me-please

tgraf · 2018-03-14T01:12:56Z

pkg/policy/rule_test.go

+		Ingress:          []api.IngressRule{{FromCIDR: cidrs}},
+	}
+	err := apiRule1.Sanitize()
+	c.Assert(err, Not(IsNil))


I feel a bit uneasy about this behaviour but I understand given the current implementation.

joestringer added wip sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/bugtool Impacts gathering of data for debugging purposes. labels Mar 13, 2018

joestringer requested a review from a team as a code owner March 13, 2018 01:19

joestringer requested a review from a team March 13, 2018 01:19

joestringer requested review from a team as code owners March 13, 2018 01:19

joestringer requested a review from a team March 13, 2018 01:19

houndci-bot reviewed Mar 13, 2018

View reviewed changes

joestringer force-pushed the submit/cidr-limit-bump branch from 7fe1076 to 060e57f Compare March 13, 2018 03:39

joestringer added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. and removed area/bugtool Impacts gathering of data for debugging purposes. labels Mar 13, 2018

rlenglet approved these changes Mar 13, 2018

View reviewed changes

joestringer force-pushed the submit/cidr-limit-bump branch from 060e57f to 727b610 Compare March 13, 2018 19:37

joestringer added pending-review and removed wip labels Mar 13, 2018

joestringer force-pushed the submit/cidr-limit-bump branch from 727b610 to d507a4f Compare March 13, 2018 19:40

ianvernon approved these changes Mar 13, 2018

View reviewed changes

joestringer force-pushed the submit/cidr-limit-bump branch from d507a4f to ee69ab5 Compare March 13, 2018 19:58

joestringer added 4 commits March 13, 2018 14:32

test: Remove CIDR limit test

50dc3c4

This test needs rethinking with the upcoming expansion of CIDR limits. Remove it for now. We should be able to achieve this with just unit testing. Signed-off-by: Joe Stringer <joe@covalent.io>

policy: Add test for rule rejection conditions

268b109

Signed-off-by: Joe Stringer <joe@covalent.io>

policy: Return prefix length from CIDR.sanitize()

efdeb31

Signed-off-by: Joe Stringer <joe@covalent.io>

joestringer force-pushed the submit/cidr-limit-bump branch from ee69ab5 to 2af084f Compare March 13, 2018 21:33

tgraf approved these changes Mar 14, 2018

View reviewed changes

tgraf merged commit cc7a4cb into cilium:master Mar 14, 2018

joestringer deleted the submit/cidr-limit-bump branch March 14, 2018 01:46

joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump CIDR limit on older kernels. #3119

Bump CIDR limit on older kernels. #3119

joestringer commented Mar 13, 2018 •

edited

houndci-bot Mar 13, 2018

houndci-bot Mar 13, 2018

rlenglet Mar 13, 2018

joestringer Mar 13, 2018

joestringer Mar 13, 2018

rlenglet Mar 13, 2018

rlenglet Mar 13, 2018

joestringer Mar 13, 2018

tgraf Mar 14, 2018

joestringer Mar 14, 2018

rlenglet Mar 13, 2018

joestringer commented Mar 13, 2018

tgraf Mar 14, 2018

Bump CIDR limit on older kernels. #3119

Bump CIDR limit on older kernels. #3119

Conversation

joestringer commented Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joestringer commented Mar 13, 2018

Choose a reason for hiding this comment

joestringer commented Mar 13, 2018 •

edited