Skip to content

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

Merged
joestringer merged 6 commits intocilium:masterfrom
christarazi:pr/christarazi/enable-ingress-from-cidr-to-ports
Jul 15, 2020
Merged

policy: Enable ingress CIDR-dependent L3 policy (FromCIDR + ToPorts)#12482
joestringer merged 6 commits intocilium:masterfrom
christarazi:pr/christarazi/enable-ingress-from-cidr-to-ports

Conversation

@christarazi
Copy link
Copy Markdown
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
Copy Markdown
Member Author

christarazi commented Jul 9, 2020

test-me-please

@christarazi christarazi requested review from a team and joestringer 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
Copy Markdown

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
Copy Markdown
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.

Comment thread pkg/policy/api/ingress.go Outdated
Comment thread test/k8sT/Policies.go Outdated
Comment thread test/k8sT/Policies.go Outdated
christarazi
christarazi commented Jul 9, 2020
View reviewed changes
Comment thread test/k8sT/Policies.go Outdated
@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
Copy Markdown
Member Author

christarazi commented Jul 10, 2020

test-me-please

@christarazi
Copy link
Copy Markdown
Member Author

christarazi commented Jul 10, 2020

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
Copy Markdown
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
Copy Markdown
Member Author

christarazi commented Jul 10, 2020

test-me-please

pchaigno
pchaigno reviewed Jul 10, 2020
View reviewed changes
Copy link
Copy Markdown
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!

Comment thread pkg/policy/api/rule_validation.go Outdated
Comment thread test/k8sT/Policies.go Outdated
Comment thread test/k8sT/Policies.go Outdated
Comment thread test/k8sT/Policies.go Outdated
Comment thread test/k8sT/Policies.go Outdated
Comment thread test/k8sT/manifests/cnp-ingress-from-cidr-to-ports.yaml Outdated
@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jul 10, 2020

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
Copy Markdown
Member Author

christarazi commented Jul 13, 2020

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
Comment thread pkg/policy/api/ingress.go Outdated
@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
Copy Markdown
Member

pchaigno commented Jul 15, 2020

test-me-please

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jul 15, 2020

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