Skip to content

policy: Enable ingress CIDR-dependent L3 policy (FromCIDR + ToPorts) #12482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 15, 2020
Merged

policy: Enable ingress CIDR-dependent L3 policy (FromCIDR + ToPorts) #12482

merged 6 commits into from
Jul 15, 2020

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Jul 9, 2020 •
edited
Loading

See commit msgs.

Updates: #4129

Note to backporters: If there are conflicts with the tests, then test coverage commits do not need to be backported; only enabling the policy.

@christarazi christarazi added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. kind/enhancement This would improve or streamline existing functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 9, 2020
@christarazi
Copy link
Member Author

test-me-please

@christarazi christarazi requested review from joestringer and a team July 9, 2020 22:04
@christarazi christarazi marked this pull request as ready for review July 9, 2020 22:07
@christarazi christarazi requested review from a team as code owners July 9, 2020 22:07
@christarazi christarazi mentioned this pull request Jul 9, 2020
Closed
@coveralls
Copy link

coveralls commented Jul 9, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 36.962% when pulling 45ed873 on christarazi:pr/christarazi/enable-ingress-from-cidr-to-ports into b0610a4 on cilium:master.

joestringer
joestringer requested changes Jul 9, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Nice work, just few minor nits.

christarazi
christarazi commented Jul 9, 2020
View reviewed changes
@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch from 26d0956 to 84b7a97 Compare July 10, 2020 00:09
@christarazi
Copy link
Member Author

test-me-please

@christarazi
Copy link
Member Author

CI failures look legit; seems like these tests can be flakey. Looking into making them more deterministic.

@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch from 84b7a97 to 2b51192 Compare July 10, 2020 17:35
@christarazi
Copy link
Member Author

christarazi commented Jul 10, 2020 •
edited
Loading

Reworked some of the tests a bit to make them more deterministic. The main changes from the original code are:

  1. Utilize Eventually and Consistently constructs in testConnectivity()
    • The rationale behind this one is it's possible that by the time we test connectivity, either the pod or the policy aren't ready, which may cause the first few attempts to fail. This is why we "poll" that the connectivity eventually converges on the desired result "eventually". Once that convergence happens, we expect the connectivity to consistently give the same result.
  2. Count the number of policy verdict logs from cilium monitor
    • The rationale here is that because we've introduced the possibility of many requests from (1), and we need to count the number of requests that achieved their expected result (allowed or denied). We count because we want to validate that we see the same count of policy verdicts. This helps give us a more deterministic view, e.g. we see 10 requests succeed as expected, we should see 10 policy verdicts that say "allowed".

@christarazi christarazi requested a review from joestringer July 10, 2020 17:35
@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch 2 times, most recently from a4bccbb to af4e675 Compare July 10, 2020 19:30
@christarazi
Copy link
Member Author

test-me-please

pchaigno
pchaigno reviewed Jul 10, 2020
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

A few nits and questions below. LGTM overall!

It looks like implementing the same test for the host firewall is going to be fairly easy now 🎉 Thanks!

@pchaigno
Copy link
Member

Marking this for backport because I think we will need it at least in 1.8, at least for the host firewall.

@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch from af4e675 to 6a1f685 Compare July 13, 2020 05:00
@christarazi
Copy link
Member Author

test-me-please

@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch from 6a1f685 to 174fa10 Compare July 13, 2020 05:04
@christarazi christarazi mentioned this pull request Jul 14, 2020
Merged
@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch from 38951c0 to 7094430 Compare July 14, 2020 23:00
joestringer
joestringer reviewed Jul 14, 2020
View reviewed changes
@christarazi
policy: Allow CIDR-dependent L4 support on ingress
95c75dc 
This allows policies such as FromCIDR + ToPorts on ingress.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
test/helpers: Add MonitorEndpointStart
2c671d7 
This is useful for monitoring specific endpoints.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
test/helpers: Add adding/deleting route wrappers
caee40b 
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
test: Factor out importing policy function
ddf1713 
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
test/helpers: Add merge cfg Cilium deploy variant
bbe50c3 
This helper is useful for situations where you'd want to preserve the
previous Cilium configuration when redeploying. This helper abstracts
away the need to manually create a copy of the new merged configuration
before calling RedeployCilium.

Example usage:

```
daemonCfg := map[string]string{ ... }

(...)

RedeployCiliumWithMerge(..., daemonCfg, map[string]string{
  "flag-previously-set-in-daemonCfg": "1",
  "new-flag-also":                    "abc",
})
```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
test: Add policy test for ingress FromCIDR+ToPorts
45ed873 
This test validates that the new ingress FromCIDR+ToPorts policy
(CIDR-dependent L4) allows ingress traffic from an allowed IP address
range, to specific ports.

The setup of this test requires a third node (k8s3) which doesn't have
Cilium installed on it by making use of the NO_CILIUM_ON_NODE
environment variable of the test suite.

The test validates the policy by checking that ingress traffic is:
  1) Allowed before any policies
  2) Disallowed by applying a default deny ingress policy
  3) Allowed again after applying "cnp-ingress-from-cidr-to-ports"

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/enable-ingress-from-cidr-to-ports branch from 7094430 to 45ed873 Compare July 14, 2020 23:22
@pchaigno pchaigno requested a review from joestringer July 15, 2020 06:17
@pchaigno
Copy link
Member

test-me-please

@joestringer
Copy link
Member

I've been providing the feedback for @cilium/api and Paul (as well as me to a secondary degree) have been reviewing on behalf of @cilium/ci so all codeowners should be covered. Tests are passing, reviews are in. Merging.

@joestringer joestringer merged commit 94eb491 into cilium:master Jul 15, 2020
@christarazi christarazi deleted the pr/christarazi/enable-ingress-from-cidr-to-ports branch July 15, 2020 20:59
@joestringer joestringer mentioned this pull request Jul 15, 2020
Closed
7 tasks
@christarazi christarazi mentioned this pull request Jul 15, 2020
Closed
christarazi added a commit to christarazi/cilium that referenced this pull request Jul 20, 2020
@christarazi
doc: Remove reference to policy limitations
0393282 
This comes as a follow up to
cilium#12482.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi mentioned this pull request Jul 20, 2020
Merged
christarazi added a commit that referenced this pull request Jul 20, 2020
@christarazi
doc: Remove reference to policy limitations
0cb6c1f 
This comes as a follow up to
#12482.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
pchaigno pushed a commit that referenced this pull request Jul 21, 2020
@christarazi @pchaigno
doc: Remove reference to policy limitations
929a00b 
[ upstream commit 0cb6c1f ]

This comes as a follow up to
#12482.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno mentioned this pull request Jul 21, 2020
Merged
rolinh pushed a commit that referenced this pull request Jul 21, 2020
@christarazi @rolinh
doc: Remove reference to policy limitations
d6ed26e 
[ upstream commit 0cb6c1f ]

This comes as a follow up to
#12482.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno mentioned this pull request Dec 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb left review comments

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
kind/enhancement This would improve or streamline existing functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@christarazi @coveralls @pchaigno @joestringer @brb