Add support for references to CiliumCIDRGroup inside FromCIDRSet for ingress rules in CNPs #24638

pippolo84 · 2023-03-29T16:55:09Z

Add support for cidrGroupRef in CIDR ingress rules in Cilium Network Policies.

The new rule allows to reference a list of CiliumCIDRGroup objects that contain the CIDRs from which we want to allow traffic from.

An example of the newly introduced CiliumCIDRGroup CRD is the following:

apiVersion: cilium.io/v2alpha1
kind: CiliumCIDRGroup
metadata:
  name: test-group-1
spec:
  externalCIDRs:
    - "172.19.0.1/32"

The feature is implemented as a wrapper to the existing FromCIDR Ingress rule. Each cidrGroupRef field is translated to the equivalent fromCIDRSet block with the CIDRs from the referenced CiliumCIDRGroup object.

To do that, we use a single handler for CiliumNetworkPolicy, CiliumClusterwideNetworkPolicy and CiliumCIDRGroup in the k8s watcher.

Every time the referenced CiliumCIDRGroups are updated, the CNP rules are recalculated, in order to append to the Ingress.FromCIDR field the updated lists of CIDRs.

In case the lists of CIDRs in the referenced CiliumCIDRGroups are all empty, the policy will have an Ingress rule with all fields equal to nil, except the cidrGroupRef one. That would result in the same behavior as an empty FromCIDR rule, which means default deny.

This PR also adds some metrics to get info about the new Ingress rule impact and usage. Specifically:

number of events related to CiliumCIDRGroup received and processed
time taken to translate the cidrGroupRef field into CIDRs
number of CNPs and CCNPs that are referencing at least a CiliumCIDRGroup object

Fixes #10349

Add new CRD CiliumCIDRGroup which allows users to define external CIDRs to be used within a CiliumNetworkPolicy, outside of the CiliumNetworkPolicy, via fromCIDRSet using cidrGroupRef

pkg/k8s/watchers/cilium_network_policy.go

pkg/policy/cidrgroup/cell.go

pkg/k8s/watchers/cilium_network_policy.go

pkg/k8s/apis/cilium.io/client/crds/v2/ciliumclusterwidenetworkpolicies.yaml

pkg/k8s/shared_resources.go

pkg/k8s/watchers/cilium_network_policy.go

joestringer

LGTM, just one super minor nit. Great work. The splitting up of the logic for the refactor vs. the new feature also made the PR much easier to reason about 👏

pkg/k8s/watchers/watcher.go

christarazi · 2023-04-13T23:37:20Z

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-8px9w

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-5.4/1754/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

Job 'Cilium-PR-K8s-1.16-kernel-4.19' hit: #24697 (88.40% similarity)

Edit:

4.19 hit CI: K8sDatapathConfig Host firewall With native routing and endpoint routes #24700
5.4 hit CI: K8sDatapathConfig Host firewall With VXLAN: Failed to reach #16122
runtime hit CI: Suite-runtime.RuntimeDatapathPrivilegedUnitTests Run Tests (pkg/datapath/linux: panic: test timed out after 10m0s) #24342

pkg/metrics/metrics.go

This generates the CiliumCIDRGroup type, to reference a group of CIDRs as a single entity in CiliumNetworkPolicies. Co-authored-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Embed `cidrGroupRef` field in api.CIDRRule to reference CiliumCIDRGroup objects in Cilium Network Policies. The field can be used to allow ingress traffic from a set of CIDRs listed in the referenced CiliumCIDRGroup object. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

CNP and CCNP events handling is based on the same code, since each CNP and CCNP is first translated to an intermediate types.SlimCNP representation. To support the handling of CNP/CCNP updates, there are two separate caches, one for CNP and one for CCNP. This commit unify the events handling and the caches, relying on the two event channels to avoid any additional locking mechanism. This sets the stage for the fromCIDRGroupRef feature, where we need to reference another k8s object (CiliumCIDRGroup) to get the CIDRs the policies are going to allow when filtering ingress traffic. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>

Add support for cidrGroupRef field in Ingress rules for Cilium Network Policies. The new field allows to reference a list of CiliumCIDRGroup objects that contain the CIDRs from which we wants to allow traffic from. The feature is implemented as a wrapper to the existing FromCIDRSet Ingress rule. Each policy with a reference to a cidr group is translated in an equivalent "FromCIDRSet policy" after looking up the CIDRs in the referenced CiliumCIDRGroup objects. To do that, we use a single handler for CiliumNetworkPolicy, CiliumClusterwideNetworkPolicy and CiliumCIDRGroup in the k8s watcher, in order to avoid further locking when accessing the CNP/CCNP cache or the CIDRGroup one. Every time the referenced CiliumCIDRGroups are updated, the CNP rules are recalculated, in order to append to the Ingress.FromCIDRSet field the updated lists of CIDRs. In case the referenced CiliumCIDRGroups k8s objects are all empty or deleted, the policy behaviour is default-deny. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>

policy_change_total metric counts all the policies change events and should take into account Cilium Clusterwide Network Policied events too. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Inspired from the CNP watcher. Co-authored-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>

The NoOp* variables are used in pkg/metrics to define metrics that can be disabled. All of those metrics are grouped together as a slice of prometheus.Collector and then registered to the prometheus register. To add a metric of type prometheus.Histogram that can also be disabled, a type that satisfies both `prometheus.Collector` and `prometheus.Observer` is needed, so the NoOpHistogram of type prometheus.histogram is added. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Add a metric to measure the time taken to translate the FromCIDRGroupRef field in CNPs and CCNPs. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Add a gauge metric to count the current number of CNPs and CCNPs that reference at least a CiliumCIDRGroup object. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Assign the following files: - k8s/pkg/watcher/cilium_network_policy.go - k8s/pkg/watcher/cilium_cidr_group.go - k8s/pkg/watcher/cilium_cidr_group_test.go containing the shared handler for CNP, CCNP and CCG objects to sig-policy group. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

christarazi · 2023-04-14T06:55:12Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig MonitorAggregation Checks that monitor aggregation restricts notifications

Failure Output

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/916/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

christarazi

Fantastic work!

pkg/k8s/watchers/cilium_network_policy.go

pippolo84 · 2023-04-14T09:23:25Z

4.19 hit #24840

pippolo84 · 2023-04-14T09:23:38Z

/test-1.16-4.19

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 29, 2023

This comment was marked as resolved.

Sign in to view

pippolo84 force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch 4 times, most recently from e7ecffd to 7a0f675 Compare March 31, 2023 20:31

christarazi force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch from 7a0f675 to c6b6633 Compare March 31, 2023 20:52

christarazi force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch 2 times, most recently from d9763d4 to 1b055b0 Compare March 31, 2023 20:58

christarazi requested changes Mar 31, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch 2 times, most recently from dce042a to 10615c4 Compare April 2, 2023 17:48

christarazi reviewed Apr 3, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch 6 times, most recently from 4d15cc5 to de686ff Compare April 3, 2023 15:42

This comment was marked as outdated.

Sign in to view

christarazi reviewed Apr 4, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

pippolo84 force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch from de686ff to 74873ee Compare April 4, 2023 09:16

joestringer approved these changes Apr 13, 2023

View reviewed changes

pkg/k8s/watchers/watcher.go Show resolved Hide resolved

christarazi reviewed Apr 14, 2023

View reviewed changes

pkg/metrics/metrics.go Outdated Show resolved Hide resolved

pippolo84 and others added 2 commits April 13, 2023 23:48

CRDs: Add CiliumCIDRGroup CRD and scaffolding

d4e42e0

This generates the CiliumCIDRGroup type, to reference a group of CIDRs as a single entity in CiliumNetworkPolicies. Co-authored-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

christarazi force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch from daa64f1 to 39f51d6 Compare April 14, 2023 06:48

christarazi force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch from 39f51d6 to d30af4e Compare April 14, 2023 06:53

pippolo84 and others added 7 commits April 13, 2023 23:54

watchers: Report policy changed event for CCNP too

2a58a81

policy_change_total metric counts all the policies change events and should take into account Cilium Clusterwide Network Policied events too. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

watchers: Add basic metrics for CCG

6015da6

Inspired from the CNP watcher. Co-authored-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>

watchers: Add a metric for CIDRGroupRef translation in CNPs

3d159c4

Add a metric to measure the time taken to translate the FromCIDRGroupRef field in CNPs and CCNPs. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

watchers: Add a metric to count policies referencing a CIDRGroup

f134ac7

Add a gauge metric to count the current number of CNPs and CCNPs that reference at least a CiliumCIDRGroup object. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

christarazi force-pushed the pr/pippolo84/from-cidr-group-refs-policy branch from d30af4e to 60506a7 Compare April 14, 2023 06:54

christarazi approved these changes Apr 14, 2023

View reviewed changes

pkg/k8s/watchers/cilium_network_policy.go Show resolved Hide resolved

christarazi changed the title ~~Add support for FromCIDRGroupRefs Ingress network policies~~ Add support for references to CiliumCIDRGroup inside FromCIDRSet for ingress rules in CNPs Apr 14, 2023

joestringer merged commit da37f7e into cilium:master Apr 17, 2023
57 checks passed

joestringer mentioned this pull request May 1, 2023

CFP for High-Scale IPcache feature cilium/design-cfps#7

Open

joestringer mentioned this pull request Jul 5, 2023

Add support for user-defined entities #3553

Closed

pippolo84 mentioned this pull request Oct 3, 2023

pkg/k8s: use a deep copy of CNP in UpdateStatus to avoid race condition #28364

Merged

joestringer mentioned this pull request Nov 21, 2023

Document CiliumCIDRGroup CRD #25123

Open

illrill mentioned this pull request Jan 24, 2024

CFP: Ability to defined Custom Entities in CiliumNetworkPolicy #21832

Closed

julianwiedmann mentioned this pull request Feb 2, 2024

CiliumCIDRGroup not work for egress rules #30597

Closed

2 tasks

This was referenced Feb 23, 2024

policy/k8s: Add support for CIDRGroupRef in IngressDeny and EgressDeny #30933

Merged

Migrate CiliumCIDRGroup watcher to ipcache async metadata APIs #30992

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for references to CiliumCIDRGroup inside FromCIDRSet for ingress rules in CNPs #24638

Add support for references to CiliumCIDRGroup inside FromCIDRSet for ingress rules in CNPs #24638

pippolo84 commented Mar 29, 2023 •

edited by christarazi

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

joestringer left a comment

christarazi commented Apr 13, 2023 •

edited

Test Name

Failure Output

christarazi commented Apr 14, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

christarazi left a comment

pippolo84 commented Apr 14, 2023

pippolo84 commented Apr 14, 2023

Add support for references to CiliumCIDRGroup inside FromCIDRSet for ingress rules in CNPs #24638

Add support for references to CiliumCIDRGroup inside FromCIDRSet for ingress rules in CNPs #24638

Conversation

pippolo84 commented Mar 29, 2023 • edited by christarazi

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

joestringer left a comment

Choose a reason for hiding this comment

christarazi commented Apr 13, 2023 • edited

Test Name

Failure Output

christarazi commented Apr 14, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

christarazi left a comment

Choose a reason for hiding this comment

pippolo84 commented Apr 14, 2023

pippolo84 commented Apr 14, 2023

pippolo84 commented Mar 29, 2023 •

edited by christarazi

christarazi commented Apr 13, 2023 •

edited

christarazi commented Apr 14, 2023 •

edited by maintainer-s-little-helper bot