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.

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

policy: Support CIDRs in rules with zero length prefix #4458

Merged
merged 7 commits into from Jun 12, 2018

Conversation

Projects
None yet
3 participants
@joestringer
Contributor

joestringer commented Jun 11, 2018

There's no reason to disallow specifying 0.0.0.0/0 CIDRs in policies,
they're as valid as any other CIDR. Make the /0 check more specific to
deny non-contiguous masks, which is what we actually disallow.

Fixes: #3146
Fixes: #3945


This change is Reviewable

@joestringer

This comment has been minimized.

Contributor

joestringer commented Jun 11, 2018

test-me-please

@joestringer

This comment has been minimized.

Contributor

joestringer commented Jun 11, 2018

I believe I hit #4455:
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/4053/

/home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:373
Cannot curl from "testclient-7ps6q" to testds-service
Expected command: kubectl exec -n default testclient-7ps6q -- curl -s -D /dev/stderr --fail --connect-timeout 5 --max-time 5 http://testds-service.default.svc.cluster.local:80/ 
To succeed, but it failed:
Exitcode: 28 
Stdout:
 	 
Stderr:
 	 command terminated with exit code 28
	 

/home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/k8sT/Chaos.go:109

Multiple services are presented for the ports 80 / 10080:

$ cat *service_list* | jq '.[] | .spec."frontend-address"' | grep -B 1 80
  "ip": "10.108.97.4",
  "port": 10080,
--
  "ip": "10.108.157.62",
  "port": 10080,
--
  "ip": "10.103.47.100",
  "port": 10080,
--
  "ip": "10.111.78.6",
  "port": 80,
--
  "ip": "10.106.173.81",
  "port": 80,
--
  "ip": "10.110.94.102",
  "port": 80,
--
  "ip": "10.106.173.81",
  "port": 80,
--
  "ip": "10.110.94.102",
  "port": 80,
--
  "ip": "10.108.97.4",
  "port": 10080,
--
  "ip": "10.108.157.62",
  "port": 10080,
--
  "ip": "10.103.47.100",
  "port": 10080,
--
  "ip": "10.111.78.6",
  "port": 80,
$ cat *service_list* | jq '.[] | .spec."frontend-address".ip' | sort | uniq
"10.103.47.100"
"10.106.173.81"
"10.108.157.62"
"10.108.97.4"
"10.110.94.102"
"10.111.78.6"
"10.96.0.1"
"10.96.0.10"
$ cat svc.txt | jq '.items[] | .spec.clusterIP'
"10.96.0.1"
"10.108.97.4"
"10.110.94.102"
"10.96.0.10"

I'll re-run CI.

@joestringer

This comment has been minimized.

Contributor

joestringer commented Jun 11, 2018

Apparently last time I triggered two builds, and the other one passed:
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/4052/

@joestringer joestringer added pending-review and removed wip labels Jun 11, 2018

@tgraf

tgraf approved these changes Jun 11, 2018

@joestringer joestringer added wip and removed pending-review labels Jun 11, 2018

@tgraf

Joe is doing additional testing

joestringer added some commits Jun 11, 2018

policy: Make CIDRRule error more consistent
Provide more information when the parsed Cidr is in an invalid format.

Signed-off-by: Joe Stringer <joe@covalent.io>
policy: Support CIDRs in rules with zero length prefix
There's no reason to disallow specifying 0.0.0.0/0 CIDRs in policies,
they're as valid as any other CIDR. Make the /0 check more specific to
deny non-contiguous masks, which is what we actually disallow.

Fixes: #3146
Fixes: #3945

Signed-off-by: Joe Stringer <joe@covalent.io>
labels: Fix source for existing cidr tests
These tests assumed that labels are specified by source:key=value, but
for label arrays they're actually handled as source.key=value. Fix up
the test. This requires no functional changes.

Signed-off-by: Joe Stringer <joe@covalent.io>
labels: Resolve CIDR 0.0.0.0/0 to reserved:world
Previously, we would resolve 0.0.0.0/0 (or ::/0) to a set of labels
0.0.0.0/0, reserved:world. The policy handling for this would be
expected to be exactly the same as for reserved:world, however
previously this would lead to allocating a unique CIDR identity for this
combination of labels. To prevent an identity conflict between this and
resolved:world, drop the 0.0.0.0/0 (or ::/0) label during CIDR to labels
conversion.

Signed-off-by: Joe Stringer <joe@covalent.io>
ipcache: Don't push reserved identities to kvstore
Reserved identities such as the one for world shouldn't be pushed to the
kvstore, as they will always be handled locally as the daemon inserts
such IP->identity mappings into the local ipcache on startup.

Signed-off-by: Joe Stringer <joe@covalent.io>
policy: Allow 0/0 CIDR to match reserved:world
When generating the EndpointSelector for a CIDRRule, if the CIDR selects
0.0.0.0/0 or ::/0, then add also the selector for reserved:world as
these should be equivalent.

Signed-off-by: Joe Stringer <joe@covalent.io>
test: Add runtime policy test for 0.0.0.0/0
Add a runtime test to check that policies allowing 0.0.0.0/0 actually
allow connectivity to the outside world.

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

@joestringer joestringer force-pushed the joestringer:submit/zero-cidr-length branch from 8fe7571 to b05fa48 Jun 11, 2018

@joestringer joestringer requested review from cilium/ci as code owners Jun 11, 2018

@joestringer

This comment has been minimized.

Contributor

joestringer commented Jun 11, 2018

Turned out that it needed a scattering of other changes. They're all pretty small scope.

@joestringer

This comment has been minimized.

Contributor

joestringer commented Jun 11, 2018

test-me-please

@joestringer joestringer dismissed stale reviews from rlenglet and tgraf Jun 11, 2018

Added a bunch of extra changes.

@joestringer joestringer added pending-review and removed wip labels Jun 12, 2018

@joestringer

This comment has been minimized.

Contributor

joestringer commented Jun 12, 2018

Tests passed, and I did some manual testing as well for both ingress and egress CIDR /0 allows (plus exclusions).

@tgraf

tgraf approved these changes Jun 12, 2018

@tgraf tgraf merged commit 78c681e into cilium:master Jun 12, 2018

2 checks passed

Cilium-Ginkgo-Tests Build finished.
Details
Hound No violations found. Woof!

@joestringer joestringer deleted the joestringer:submit/zero-cidr-length branch Jun 12, 2018

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