Network policies #2294

ormergi · 2025-05-19T07:54:11Z

What this PR does / why we need it:
In a scenario where a cluster networking has been hardened and futures a default deny-all network policy that affects CNAO operator pods, it will not be able to operate properly.

This PR introduce network-policies for CNAO operator allowing it to operate, complying with restrictions in form of deny all network-policy.

Special notes for your reviewer:
In order to have coverage for the introduced network-policies, and additional deny-all network-policies is installed as part of the cluster-sync flow.
Having a deny-all network-policy installed, allow the introduced network-policies to be tested in a development and test environments, locally and CI.

Since not all CNAO component provide network-policy, the deny-all network policy is configured to affect CNAO operator pods only.
Once all relevant components provide network-policies, we can change the deny-all network-policy to affect all pods in CNAO namespace.

The following work will be done in follow-up PRs:

Network-policies for CNAO components will be done in follow-up PRs, including kubemacpool, ~~kubesecondarydns~~ and kubvirt-ipam-controller.
Adjust CNAO (operator) network-policies to support OCP.

Release note:

Introduce network-policies allowing CNAO to operate in restricted env

kubevirt-bot · 2025-05-19T07:54:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign oshoval for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ormergi · 2025-05-21T11:58:54Z

Changes: rebase

oshoval · 2025-05-21T12:04:56Z

tools/manifest-templator/manifest-templator.go

@@ -254,6 +269,9 @@ func main() {
 	kubevirtIpamControllerImage := flag.String("kubevirt-ipam-controller-image", components.KubevirtIpamControllerImageDefault, "The kubevirtipamcontroller-image managed by CNA")
 	dumpOperatorCRD := flag.Bool("dump-crds", false, "Append operator CRD to bottom of template. Used for csv-generator")
 	inputFile := flag.String("input-file", "", "Not used for csv-generator")
+	clusterDNSNamespace := flag.String("cluster-dns-namespace", "kube-system", "The namespace name where the cluster DNS endpoint reside")


/hold

i am against it, as discussed offline

we have IsOpenShift all over for binary values (values that can be either k8s set of values or OpenShift)
please see render functions

this kind of change involves more repositories and makes the code less self contained, and then we have 2 ways of doing stuff we are doing for long time in one way

EDIT - fwiw i might have more comments, but imho this issue should first reach agreement this way or other please

I removed this part for now, so that this PR only adds network-policies for CNAO.
Supporting other platformes (OCP) will be done in a follow up.
PTAL

Thanks,

btw about the previous comment, cnao must stay dual as standalone
having that other method require a layer that will customize it, which also means this is undesired
for example either cluster-sync or even worse, deploy via manifests would stop being dual

oshoval · 2025-05-21T12:47:07Z

/hold cancel

will add it to my Q, i do have comments please

oshoval · 2025-05-21T12:58:50Z

some of the comments here kubevirt/kubesecondarydns#99 affect this PR as well, would you kindly consider them ?

if required i will amend them here, but once i reach it

i would even say they affect the other PRs potentially, as most of those have stuff in common

oshoval · 2025-05-21T13:01:59Z

pkg/components/networkpolicy.go

+	"k8s.io/utils/ptr"
+)
+
+func GetNetworkPolices(


lets amend please templates/cluster-network-addons/VERSION/operator.yaml.in
with this manifest
no need to go gen it

@oshoval why do you want to use template.in instead of the components.go? Mostly curious.

Manifests are easier to maintain and read,
Maybe it should exists on other manifest yaml file if thats what you mean, the interesting thing here is that it should be in a manifest imo

fwiw i didnt finish to review yet
need to check for example if we need to make it dual (k8s / OpenShift), and then to think deeper, unless it is already dual

oshoval · 2025-05-21T13:07:07Z

Since not all CNAO component provide network-policy, the deny-all network policy is configured to affect CNAO operator pods only.
Once all relevant components provide network-policies, we can change the deny-all network-policy to affect all pods in CNAO namespace.

maybe just add it at the end at once ?
stuff might change in between in case we find adjustments
meanwhile we will be protected on each repo by its own
and at the end wrap it on CNAO itself

EDIT - you can keep it as now as well, but indeed best to add the rest at once as you wrote and not step by step

oshoval · 2025-05-21T14:02:51Z

pkg/components/networkpolicy.go

+	"k8s.io/utils/ptr"
+)
+
+func GetNetworkPolices(


note that now we install it always, we need a condition please for OpenShift as there it is not relevant with those values, else it means we always deploy something that doesnt fit

the right way is to not just have if OpenShift then not install, but to just install the right one please

I removed this code following this comment #2294 (comment)

ormergi · 2025-05-21T14:25:41Z

some of the comments here kubevirt/kubesecondarydns#99 affect this PR as well, would you kindly consider them ?

if required i will amend them here, but once i reach it

Please post your comment here so discussion will take place in this PR changes context.

i would even say they affect the other PRs potentially, as most of those have stuff in common

If you meant for adding network-policy in kubemacpool, and ipam-ext they are already merged.

ormergi · 2025-05-21T14:28:38Z

Since not all CNAO component provide network-policy, the deny-all network policy is configured to affect CNAO operator pods only.
Once all relevant components provide network-policies, we can change the deny-all network-policy to affect all pods in CNAO namespace.
maybe just add it at the end at once ? stuff might change in between in case we find adjustments meanwhile we will be protected on each repo by its own and at the end wrap it on CNAO itself

EDIT - you can keep it as now as well, but indeed best to add the rest at once as you wrote and not step by step

I think we better have the deny-all np (affecting cnao pods) installed, it prevents the next dev to remove or change the network-policies in a way it wont work as expected.
Once we have all components consumed in this repo, the deny-all selector can be removed add affect all the pods in CNAO namespace.

ormergi · 2025-05-21T14:29:22Z

Changes: resolve #2294 (review) #2294 (review)
@oshoval PTAL

ormergi · 2025-05-21T14:51:12Z

Changes: rebase

ormergi · 2025-05-21T16:29:34Z

/tes pull-e2e-cluster-network-addons-operator-monitoring-k8s

ormergi · 2025-05-21T16:48:25Z

Changes: simplify install deny-all network-policy script

oshoval · 2025-05-22T08:42:40Z

templates/cluster-network-addons/VERSION/operator.yaml.in

+    - to:
+      - namespaceSelector:
+          matchLabels:
+            kubernetes.io/metadata.name: kube-system


this is problematic, it wont be dual right ?
same on L59 and whatever parameters that need to be dual

(this is partial review, but it seems the PR is not ready yet as it is now if so)

as long as there is no chicken and egg, best that CNAO will be able to be dualic and wont need any other repo for that,
but worth to understand if CNAO can create this NP, or someone else that is already whitelisted should, because CNAO cant do anything before it is created and then we can think deeper
In such case i would say best to do as done with KSD, but didnt think deeper yet (EDIT - not needed here, because it is not API, but rather static automated detection)
another option in case of deadlock is pretty like you wanted, but with one parameter, as we done with allow- multus param (but then need to check deeper on easy dual deployment via manifests as good as possible please)

this is problematic, it wont be dual right ?
same on L59 and whatever parameters that need to be dual

Yep, those manifests must have placeholders to allow the templator to set the right values, to make these manifest work in other platforms. Similar to previous iteration but more simple.

In a scenario where a cluster networking has been hardened and futures a default deny-all network policy that affects KSD namespace, CNAO will not be able to operate properly. Provide network-policies in order to comply with such restrictions, it should allow CNAO operate when a deny-all network-policy is in place. This commit add the following network-polices: 1. Allow egress to cluster DNS Enable interaction with cluster DNS, w/o this policy the CNAO operator cannot establish connection with the cluster API. Affect pods with name=cluster-network-addons-operator label. 2. Allow egress to kube-apiserver Enable interaction with the cluster API, w/o this policy CNAO operator cannot r/w VMs/Pods etc... Affect pods with name=cluster-network-addons-operator label. 3. Allow ingress to metrics endpoint Enable interacting with CNAO metric endpoint, w/o this policy its impossible to collect CNAO metrics. Affect pods with name=cluster-network-addons-operator label. The metric number is taken from the selected pod "metrics" port number (8443). This commit includes generated files following 'make gen-manifests'. Testing the network-policies will be done in follow-up commit. Signed-off-by: Or Mergi <ormergi@redhat.com>

The project network-policies (NP) are installed as part of cluster-sync flow. In order to have coverage for these network-policies, the tested env should simulate network restrictions, such as deny-all network-policy. Install deny-all network-policy as part of the cluster-sync flow, blocking egress/ingress traffic affecting CNAO operator pods. Having the deny-all NP installed as part of the sync flow enable coverage both in development env and tests, locally and CI. Once all components provider network-policies the installed NP can be set to affect all pods in CNAO namespace. Signed-off-by: Or Mergi <ormergi@redhat.com>

ormergi · 2025-05-22T12:16:03Z

Changes: resolve #2294 (comment)

Set placeholders in network-policies, and have the templator fill them.

@oshoval @RamLavi PTAL

sonarqubecloud · 2025-05-22T12:16:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

oshoval · 2025-05-22T12:21:20Z

tools/manifest-templator/manifest-templator.go

+		ClusterDNSPlacement: components.ClusterDNSPlacement{
+			Namespace:  *clusterDNSNamespace,
+			LabelKey:   *clusterDNSLabelKey,
+			LabelValue: *clusterDNSLabelValue,
+		},


before we continue with this approach

Do we know there is chicken and egg issue for sure ?
is there any way to fix it nicely if so (Render style, without external params as this ones), not sure but maybe you can think of

If chicken and egg issue is real:

We don't need 3 params, one is enough, isOpenShift, and internally the one that consume those vars can set the needed fields

Need to think please about additional manifest to release on releases page,
maybe one for OpenShift, because the one we release now will have the default values.

Need to make sure cluster-sync can get a parameter that allow easily to deploy it on KCLI and such please

(partial review)

another option is that on OpenShift, HCO is the one that need to deploy the NP, not CNAO
Please check out about this option
it will reduce lot of hassle, and important to consider

cc @RamLavi

Do we know there is chicken and egg issue for sure ? is there any way to fix it nicely if so (Render style, without external params as this ones), not sure but maybe you can think of

If chicken and egg issue is real:

1. We don't need 3 params, one is enough, isOpenShift, and internally the one that consume those vars can set the needed fields

The templator tool run locally and not as part of the controller in runtime, it doesnt interact with the cluster-api, its merely generate yamls.

2. Need to think please about additional manifest to release on releases page, maybe one for OpenShift, because the one we release now will have the default values.

It seem inevitable to introduce additional manifest for OCP.

3. Need to make sure cluster-sync can get a parameter that allow easily to deploy it on KCLI and such please

In case we have an additional manifest for OCP, we can introduce a parameter to cluster-sync to make it work and deploy the OCP manifest.

The concerns you raise in (2) and (3) are true for any project that need to provider network-policies and support OCP (due to the fact that the manifest containers env specific details).

I hope we can address (2) and (3) in a follow up PR.

we should not rush it
HCO can be a valid option, and not sure we can postpone stuff to follow-up (it is a regression and i am against it), but step by step please
if we see it is not valid, we can back to discuss this option deeper please (if have quite few things to tell about them if we reach it)

cc @nunnatsa we might need to discuss this deeper please

/hold
we need to make sure about all the options and their impact
EDIT - design doc might help here

oshoval · 2025-05-26T09:38:35Z

Since the NP is required for OpenShift best practices
We can omit it from the manifests on the upstream release page.

This solves one of the major problems, allowing CNAO to stay independent repository and easy deployable in both modes.
We can just document how to generate those in case needed (or to have a yaml example in one of the docs).

We still need to take care about cluster-sync, and so on of course.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 19, 2025

kubevirt-bot requested review from oshoval and phoracek May 19, 2025 07:54

kubevirt-bot added the size/L label May 19, 2025

ormergi force-pushed the network-policies branch 5 times, most recently from 3299e75 to f42238c Compare May 21, 2025 11:58

oshoval reviewed May 21, 2025

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2025

ormergi force-pushed the network-policies branch from f42238c to b01b4bd Compare May 21, 2025 12:41

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2025

oshoval reviewed May 21, 2025

View reviewed changes

ormergi force-pushed the network-policies branch from b01b4bd to 1ad782d Compare May 21, 2025 14:23

ormergi force-pushed the network-policies branch from 1ad782d to 8c6ced7 Compare May 21, 2025 14:49

ormergi force-pushed the network-policies branch from 8c6ced7 to b3e324c Compare May 21, 2025 16:47

kubevirt-bot removed the size/L label May 21, 2025

kubevirt-bot added the size/M label May 21, 2025

oshoval reviewed May 22, 2025

View reviewed changes

ormergi added 2 commits May 22, 2025 14:58

ormergi force-pushed the network-policies branch from b3e324c to 263f920 Compare May 22, 2025 12:15

kubevirt-bot added size/L and removed size/M labels May 22, 2025

oshoval reviewed May 22, 2025

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2025

ormergi marked this pull request as draft June 12, 2025 11:29

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2025

Network policies #2294

Are you sure you want to change the base?

Network policies #2294

Uh oh!

Conversation

ormergi commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubevirt-bot commented May 19, 2025

Uh oh!

ormergi commented May 21, 2025

Uh oh!

oshoval May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oshoval May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oshoval commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oshoval commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oshoval commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oshoval May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormergi commented May 21, 2025

Uh oh!

ormergi commented May 21, 2025

Uh oh!

ormergi commented May 21, 2025

Uh oh!

ormergi commented May 21, 2025

Uh oh!

ormergi commented May 21, 2025

Uh oh!

ormergi commented May 21, 2025

Uh oh!

oshoval May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormergi May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormergi commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented May 22, 2025

Quality Gate passed

Uh oh!

oshoval May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oshoval May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ormergi commented May 19, 2025 •

edited

Loading

oshoval May 21, 2025 •

edited

Loading

oshoval May 21, 2025 •

edited

Loading

oshoval commented May 21, 2025 •

edited

Loading

oshoval commented May 21, 2025 •

edited

Loading

oshoval commented May 21, 2025 •

edited

Loading

oshoval May 21, 2025 •

edited

Loading

oshoval May 22, 2025 •

edited

Loading

ormergi May 22, 2025 •

edited

Loading

ormergi commented May 22, 2025 •

edited

Loading

oshoval May 22, 2025 •

edited

Loading

oshoval May 22, 2025 •

edited

Loading

oshoval May 22, 2025 •

edited

Loading

oshoval commented May 26, 2025 •

edited

Loading