Skip to content

Add libraries for API CIDR string to labels / IPNet#3909

Merged
ianvernon merged 3 commits intocilium:masterfrom
joestringer:submit/cidr-labels
Apr 27, 2018
Merged

Add libraries for API CIDR string to labels / IPNet#3909
ianvernon merged 3 commits intocilium:masterfrom
joestringer:submit/cidr-labels

Conversation

@joestringer
Copy link
Copy Markdown
Member

@joestringer joestringer commented Apr 26, 2018

[Split out from #3835 for easier review]

This PR introduces the library code for #3835 which allows a CIDR rule or CIDRSet rule from the API to be converted into one of two formats:

  • Labels
  • []*net.IPNet

Labels

Allow conversion into a set of native Labels (pkg/labels/) that represent the CIDR and all CIDRs that contain it, ie ones with the same prefix but shorter prefix lengths. These are used to allocate an identity, which, when an EndpointSelector is created on the CIDR prefix, or any prefix that the CIDR falls under, will be selected. This works by, for instance, for 10.0.0.0/8 creating labels such as:

  • 10.0.0.0/8
  • 10.0.0.0/7
  • 8.0.0.0/6
  • ...
  • 0.0.0.0/0

Now, if an EndpointSelector is created to match 0.0.0.0/0, for instance, it will match this identity. This is what one might expect - the 0.0.0.0/0 CIDR matches all traffic.

IPv6 addresses are handled a bit specially. Labels are not allowed to contain :, however regular IPv6 formatting per RFC5952 typically contains this character. This PR represents such CIDRs using a label that replaces : with -. The - is converted back before formatting to string. Furthermore, label keys used for EndpointSelectors cannot begin or end with a -, so if the IPv6 CIDR label would otherwise begin or end with with -, eg ::/0 -> --/0, then insert a zero at the start or end to satisfy the EndpointSelector constraints. So ::/0 becomes cidr:0--0/0.

Finally, although the identities generated using these labels would contain labels for each CIDR Prefix that is in the rule plus every CIDR prefix that is shorter, it is both redundant and too verbose to print these regularly in output such as cilium identity get. This PR adds a patch to represent such CIDRs with just the most-specific CIDR when formatting. Using cilium identity get --json will provide the actual underlying information.

[]*net.IPNet

PR #3835 will also need to create CIDR -> Identity mappings in the ipcache. These are currently managed using net.IP as they only cover IP addresses. It makes sense to use the native go type *net.IPNet to represent prefixes there. Therefore this PR introduces libraries to also convert the API CIDR rule / CIDRSet rules into slices of *net.IPNet for insertion into the IPCache.


This change is Reviewable

@joestringer joestringer requested review from a team as code owners April 26, 2018 04:34
@joestringer joestringer requested a review from a team April 26, 2018 04:34
@joestringer
Copy link
Copy Markdown
Member Author

test-me-please

@joestringer joestringer added pending-review sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Apr 26, 2018
@joestringer
Copy link
Copy Markdown
Member Author

Unit tests failed because it relies on the types introduced in #3883, but this can still be reviewed.

Comment thread cilium/cmd/identity_get.go Outdated
Comment thread pkg/labels/labels.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we have "except" in the CIDR definitions for policies, is it possible we would return too few most-specific-CIDR ranges when using this for a policy/identity print? (It should be fine if used on an endpoint, I think). I think you mention this in #3835 but I'm not sure that I fully understand the interactions here and what is left to be pulled in.

If the above can happen, and is a problem, I think we can sort it out by making cidr a list, and then check element in it for ones > prefixLength and also cidr[i].Contains(ipnet). If it matches none add it to the list, if it matches one, override it (so, overwrite an entry in cidr if ipnet is more specific than that entry but add a new entry to cidr if ipnet is outside the range of all the current most specific ranges)

Copy link
Copy Markdown
Member Author

@joestringer joestringer Apr 26, 2018

Choose a reason for hiding this comment

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

My expectation (and maybe there's a place this can be left somewhere as a comment) is that CIDR exceptions will be expanded when rendering into labels, so you would end up with, for example,

One rule that uses CIDRSet with exception like:
CIDR: 0.0.0.0/0 Except 0.0.0.0/2

turns into two prefixes, with labels:

cidr:0.0.0.0/0, cidr:128.0.0.0/1

and

cidr:0.0.0.0/0, cidr:0.0.0.0/1, cidr:64.0.0.0/2

In these cases, when attempting to get the printable model, it's redundant to print 0.0.0.0/0 for any of these CIDRs, because of course all CIDRs fall under the default prefix. For the latter example, it's not useful to show 0.0.0.0/1 because cidr:64.0.0.0/2 is naturally within the 0.0.0.0/1, so it's more specific to show cidr:64.0.0.0/2.

Does that help to explain?

@joestringer joestringer force-pushed the submit/cidr-labels branch 5 times, most recently from 7a07389 to e8aff31 Compare April 26, 2018 22:18
Signed-off-by: Joe Stringer <joe@covalent.io>
Although identities may contain multiple CIDR labels, it's difficult to
read and provides no additional information beyond the longest CIDR in
the labels list. Filter through and only print the longest cidr.

Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
@joestringer
Copy link
Copy Markdown
Member Author

test-me-please

@joestringer joestringer added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed pending-review labels Apr 27, 2018
@ianvernon ianvernon merged commit 5f9dc6f into cilium:master Apr 27, 2018
@joestringer joestringer deleted the submit/cidr-labels branch April 27, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants