Skip to content

Support CIDR-dependent L4 egress policies#3835

Merged
aanm merged 12 commits intocilium:masterfrom
joestringer:submit/cidr-l4
May 10, 2018
Merged

Support CIDR-dependent L4 egress policies#3835
aanm merged 12 commits intocilium:masterfrom
joestringer:submit/cidr-l4

Conversation

@joestringer
Copy link
Copy Markdown
Member

@joestringer joestringer commented Apr 20, 2018

Support the combination of toCIDR/toCIDRSet and toPorts in a single egress rule, so this rule will only allow traffic that matches both of these conditions.

Current limitations:

  • Up to about 14 prefix lengths may be used with this PR as-is, eg, a x/1, y/2, ... z/10. Any number of CIDRs may be configured under that set of prefix lengths. Beyond we start to hit BPF instruction limits.
    • This includes CIDRSet "exceptions", eg if you have "allow CIDR /8" and "except this /16" then it might be slightly too complex.
    • When importing a policy, if the supported number of prefix lengths is likely to be exceeded, we will reject the policy (rather than waiting until compilation time)
  • Cannot select the host IP(s) via CIDR policy. If you want to filter on host IP, use "entity: host".

Related: #1684

Related dependent PRs:

Short-term todos:

  • Make sure that CIDR+L4 doesn't allow the entire CIDR as well as the specified l3+L4 range
  • Add cluster cidr to ipcache on startup
  • Improve documentation
  • Improve unit testing
  • Add basic ginkgo tests
  • Check the output of every tool that prints labels
    • cilium identity list
    • cilium bpf policy get
  • Fix up NPHDS OnIPIdentityCacheChange() to generate prefixes
    • Test CIDR-dependent L7
  • Shift regular l3 CIDR over to this model
  • Test, fix IPv6 CIDRs

Longer term, unlikely to be addressed by this PR:

  • LPM based BPF map for newer kernels
  • Enable Ingress CIDR-dependent L4(+L7)
    • Requires minor datapath changes
    • Remove CIDRPolicyMap and related logic

This change is Reviewable

@joestringer joestringer added wip area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/daemon Impacts operation of the Cilium daemon. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Apr 20, 2018
@joestringer joestringer requested a review from ianvernon as a code owner April 20, 2018 16:09
@joestringer joestringer requested a review from a team April 20, 2018 16:09
@joestringer joestringer requested a review from a team as a code owner April 20, 2018 16:09
@joestringer joestringer requested a review from a team April 20, 2018 16:09
@joestringer joestringer requested review from a team as code owners April 20, 2018 16:09
@joestringer joestringer requested review from a team April 20, 2018 16:09
Comment thread pkg/policy/api/cidr.go Outdated
Comment thread pkg/maps/ipcache/ipcache.go Outdated
Comment thread pkg/labels/cidr/doc.go Outdated
Comment thread pkg/labels/cidr.go Outdated
Comment thread pkg/labels/cidr.go Outdated
Comment thread pkg/ipcache/ipcache.go Outdated
@joestringer
Copy link
Copy Markdown
Member Author

One of the k8s tests, specifically k8s-1.10 failed, and I see segfaults from the proxy in it. Most likely unrelated. I'm working on ginkgo tests before I request a more thorough review.

@joestringer joestringer requested review from a team as code owners April 22, 2018 23:04
Comment thread pkg/policy/l4.go Outdated
Comment thread pkg/policy/l4.go Outdated
@joestringer
Copy link
Copy Markdown
Member Author

Clicked the wrong button.

@joestringer joestringer reopened this Apr 23, 2018
@tgraf tgraf mentioned this pull request Apr 23, 2018
36 tasks
@cilium cilium deleted a comment from houndci-bot Apr 24, 2018
Copy link
Copy Markdown
Contributor

@jrfastab jrfastab left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

joestringer added 12 commits May 9, 2018 15:30
Extend the IPIdentityPair to support IP masks so that we can propagate
CIDR->ID mappings into the KVstore.

Signed-off-by: Joe Stringer <joe@covalent.io>
Implement prefix length tracking in the ipcache so that the daemon can
request the list of prefixes during BPF code generation.

Signed-off-by: Joe Stringer <joe@covalent.io>
For each unique CIDR that is configured via policy, we allocate an
Identity. These CIDR -> Identity mappings are then stored in the ipcache
in the kvstore so that we can perform LPM CIDR -> Identity mapping.

With these things in place, the core of the Cilium userspace and
datapath code can use simple IP (CIDR) -> Identity mapping, then
Identity-based policy for allowing or denying traffic to/from those
CIDRs. This greatly simplifies the handling of CIDRs throughout the
codebase.

Signed-off-by: Joe Stringer <joe@covalent.io>
This magic patch enables ingress / egress rules to select CIDRs via
EndpointSelectors. Bam. CIDR-dependent L4.

Signed-off-by: Joe Stringer <joe@covalent.io>
For fully specified prefixes such as ".../32" for IPv4 or ".../128" for
IPv6, the daemon's ipcache garbage collector would potentially delete
the corresponding datapath entry if there was a host in the ipcache but
not a fully specified prefix. Resolve this by attempting to look up
either of them from the garbage collector, and if either can be found,
leave the corresponding datapath entry alone.

Signed-off-by: Joe Stringer <joe@covalent.io>
Shift this stuff out into its own function to shrink the size of d.init().

Signed-off-by: Joe Stringer <joe@covalent.io>
On startup, we need to push the cluster IP (v4 and v6) and default CIDR
mappings into the ipcache, bypassing the kvstore. We already do this for
hosts, expand it for the other special identities.

Signed-off-by: Joe Stringer <joe@covalent.io>
Also, document the behaviour.

Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
At the moment, there's a limitation in the BPF codepath where on older
kernels, the LPM implementation in use for ipcache CIDR lookups results
in many instructions generated per unique CIDR prefix length. If more
than about 16 of these are used (with ~15 instructions / prefix length),
then the BPF code we generate will exceed the kernel verification limit
for a single program, which leads endpoints to fall into a bad state.

Since we need to count the CIDRs in use anyway via the ipcache to
generate these lengths, take these limits into account when importing
the policy, and reject a policy if the resultant set of CIDRs would
generate BPF programs that are too big.

This limit is best-effort, since other things such as the number of
ports forwarded to L7 may also affect this instruction limit and it's
difficult to model the code generation ahead of time, but for basic
testing this makes it fairly obvious where the limits are for some basic
cases.

Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
Previously, the arguments forced the callers to be aware of the details
of how the IPIdentityPair represents a host->Identity mapping. This
commit hides those details within the implementation, then provides an
accessor to determine whether this is the case, for users that care.

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 May 10, 2018
@aanm
Copy link
Copy Markdown
Member

aanm commented May 10, 2018

@joestringer please add release note

@joestringer
Copy link
Copy Markdown
Member Author

@aanm I was under the impression that the tag by itself will pick up the title of the PR as the release note, is that not true or does the title not sufficiently describe the feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/daemon Impacts operation of the Cilium daemon. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. upgrade-impact This PR has potential upgrade or downgrade impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants