Support CIDR-dependent L4 egress policies#3835
Merged
aanm merged 12 commits intocilium:masterfrom May 10, 2018
Merged
Conversation
houndci-bot
reviewed
Apr 20, 2018
0aaff12 to
45191ba
Compare
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. |
45191ba to
74d214a
Compare
houndci-bot
reviewed
Apr 22, 2018
74d214a to
e3f7941
Compare
Member
Author
|
Clicked the wrong button. |
jrfastab
approved these changes
May 3, 2018
Contributor
jrfastab
left a comment
There was a problem hiding this comment.
Looks reasonable to me.
b715372 to
f01c1cd
Compare
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>
f01c1cd to
c5ff7a8
Compare
Member
Author
|
test-me-please |
Member
|
@joestringer please add release note |
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? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support the combination of
toCIDR/toCIDRSetandtoPortsin a single egress rule, so this rule will only allow traffic that matches both of these conditions.Current limitations:
Related: #1684
Related dependent PRs:
Short-term todos:
cilium identity listcilium bpf policy getOnIPIdentityCacheChange()to generate prefixesLonger term, unlikely to be addressed by this PR:
This change is