Add MultiNetworkPolicy resource #1573

Anatw · 2024-01-08T14:26:39Z

Short description:

More details:

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewer:

Bug:

redhat-qe-bot · 2024-01-08T14:26:45Z

Report bugs in Issues

The following are automatically added:

Add reviewers from OWNER file (in the root of the repository) under reviewers section.
Set PR size label.
New issue is created for the PR. (Closed when PR is merged/closed)
Run pre-commit if .pre-commit-config.yaml exists in the repo.

Available user actions:

To mark PR as WIP comment /wip to the PR, To remove it from the PR comment /wip cancel to the PR.
To block merging of PR comment /hold, To un-block merging of PR comment /hold cancel.
To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
verified label removed on each new commit push.
To cherry pick a merged PR comment /cherry-pick <target branch to cherry-pick to> in the PR.
- Multiple target branches can be cherry-picked, separated by spaces. (/cherry-pick branch1 branch2)
- Cherry-pick will be started when PR is merged
To build and push container image command /build-and-push-container in the PR (tag will be the PR number).
To add a label by comment use /<label name>, to remove, use /<label name> cancel

Supported /retest check runs

/retest tox: Retest tox
/retest sonarqube: Retest sonarqube
/retest python-module-install: Retest python-module-install

Supported labels

hold
verified
wip
lgtm

ocp_resources/multi_network_policy.py

rnetser · 2024-01-09T09:34:45Z

ocp_resources/multi_network_policy.py

+            nad_name_for_policy (str): The name of the NetworkAttachmentDefinition to which the resources connected
+                the created policy will impact.
+            client (DynamicClient): Dynamic client for connecting to a remote cluster.
+            policy_types (list): one or both of the valid ip policies - "Ingress" and "Egress".


do not include api-supported values

ocp_resources/multi_network_policy.py

rnetser · 2024-01-09T09:43:25Z

ocp_resources/multi_network_policy.py

+            self.res["metadata"]["annotations"] = {
+                f"{MultiNetworkPolicy.api_group}/policy-for": f"{self.namespace}" f"/{self.nad_name_for_policy}"
+            }
+            updated_data = {


set directly in self.res["spec"]

I don't understand. This is part of the metadata, not the spec.

updated_data is set as spec
please see other resources to see how we set data under spec

ocp_resources/multi_network_policy.py

rnetser · 2024-01-09T10:09:52Z

ocp_resources/multi_network_policy.py

+            for policy in self.policy_types:
+                if policy == "Ingress":
+                    ingress = "ingress"
+                    if not self.ingress:


this can be an empty list - see Deny ingress from all pods in all namespaces section

rnetser · 2024-01-09T10:10:20Z

ocp_resources/multi_network_policy.py

+                }
+            }
+            self.res["spec"] = {}
+            for policy in self.policy_types:


if a user sends self.ingress or self.egress; add it to the spec

rnetser · 2024-01-11T12:27:23Z

ocp_resources/multi_network_policy.py

+            self.res["metadata"]["annotations"] = {
+                f"{MultiNetworkPolicy.api_group}/policy-for": f"{self.namespace}" f"/{self.nad_name_for_policy}"
+            }
+            updated_data = {


updated_data is set as spec
please see other resources to see how we set data under spec

myakove · 2024-01-11T12:29:37Z

@Anatw Please do not resolve comments without pushing new code.

myakove

None of the comments was addressed.

rnetser · 2024-01-18T15:00:33Z

ocp_resources/multi_network_policy.py

+            policy_types (list): One or both of the valid ip policies - "Ingress" and "Egress".
+            ingress (list): list containing a dictionary specifying the allowed "from" parameters.
+            egress (list): list containing a dictionary specifying the allowed "to" parameters.
+            pod_selector (dict): Map a label to match.


please add optional to those that are not mandatory

rnetser · 2024-01-18T15:01:27Z

ocp_resources/multi_network_policy.py

+                raise ValueError("network_name is required")
+
+            self.res["metadata"]["annotations"] = {
+                f"{self.api_group}/policy-for": f"{self.namespace} /{self.network_name}"


Suggested change

f"{self.api_group}/policy-for": f"{self.namespace} /{self.network_name}"

f"{self.api_group}/policy-for": f"{self.namespace}/{self.network_name}"

redundant space?

rnetser · 2024-01-18T15:01:51Z

ocp_resources/multi_network_policy.py

+                f"{self.api_group}/policy-for": f"{self.namespace} /{self.network_name}"
+            }
+            self.res["spec"] = {}
+            self.res["spec"].update({


assign directly in self.res["spec"]

rnetser · 2024-01-18T15:04:13Z

ocp_resources/multi_network_policy.py

+            }
+            self.res["spec"] = {}
+            self.res["spec"].update({
+                "podSelector": {"matchLabels": self.pod_selector} if self.pod_selector else {},


user should send the whole dict; empty dict will block all
this is mandatory

rnetser · 2024-01-18T15:09:48Z

ocp_resources/multi_network_policy.py

+                "podSelector": {"matchLabels": self.pod_selector} if self.pod_selector else {},
+                "policyTypes": self.policy_types,
+            })
+            if self.ingress:


can be empty list (deny all) - this condition will not apply what is requested by the user

if the user want to deny all he should send an empty list.

If you tested it, and it's working, then I guess t is OK.
Did you test it?

any update on this?

Yes, the user can send an empty list

I'm not sure about it.

In [1]: ingress = [] In [2]: if ingress: ...: print("ingress") ...: In [3]:

How your code going to set [] if it will never get in here if the user send []?
What do I miss?

rnetser · 2024-01-18T15:10:13Z

ocp_resources/multi_network_policy.py

+            })
+            if self.ingress:
+                self.res["spec"].update({"ingress": self.ingress})
+            else:  # self.egress


please remove the inline comment

see comment on empty list above

if one of them is mandatory, please add a check. if not, please add a separate condition - according to the doc, one of them must exist

It is possible to create a policy without self.ingress or self.egress:
apiVersion: k8s.cni.cncf.io/v1beta1
kind: MultiNetworkPolicy
metadata:
annotations:
k8s.cni.cncf.io/policy-for: flat-overlay-ns/flat-l2-nad
name: flat-overlay-mnp
namespace: flat-overlay-ns
spec:
podSelector:
matchLabels:
kubevirt.io/vm: vma
policyTypes:

Egress

Can you please refer me to where in the documentation you saw that including at least one of them is mandatory?

I added a condition instead of the 'else'.

If you tested it, and it's working, then I guess t is OK.
Did you test it?
I can't respond to your original message so I'm responding here - I did test it and the policy is created successfully. If the documentation is specifically claiming otherwise I want to make sure this is not a bug.

sonarqubecloud · 2024-02-08T12:51:54Z

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Anatw · 2024-02-08T13:10:22Z

/verified

Anatw · 2024-02-11T10:46:26Z

/cherry-pick v4.15

Signed-off-by: Anat Wax <awax@redhat.com>

redhat-qe-bot2 · 2024-02-11T10:46:45Z

Cherry-picked PR Add MultiNetworkPolicy resource into v4.15

Signed-off-by: Anat Wax <awax@redhat.com> Co-authored-by: ‪Anat Wax <46938838+Anatw@users.noreply.github.com>

Signed-off-by: Anat Wax <awax@redhat.com>

redhat-qe-bot added size/M branch-main labels Jan 8, 2024

redhat-qe-bot requested review from myakove, oharan2, omrirh and rnetser January 8, 2024 14:27

Anatw force-pushed the add_multi_network_policy_resource branch from 2f9d662 to 7a4b48b Compare January 8, 2024 14:27

rnetser requested changes Jan 9, 2024

View reviewed changes

redhat-qe-bot added changes-requested-rnetser commented-Anatw labels Jan 9, 2024

rnetser requested changes Jan 11, 2024

View reviewed changes

redhat-qe-bot added the needs-rebase label Jan 16, 2024

Anatw force-pushed the add_multi_network_policy_resource branch from 7a4b48b to 31576cb Compare January 16, 2024 10:29

redhat-qe-bot removed changes-requested-rnetser commented-Anatw labels Jan 16, 2024

redhat-qe-bot requested a review from rnetser January 16, 2024 10:29

redhat-qe-bot added needs-rebase and removed needs-rebase labels Jan 16, 2024

myakove requested changes Jan 18, 2024

View reviewed changes

redhat-qe-bot added changes-requested-myakove commented-Anatw labels Jan 18, 2024

Anatw force-pushed the add_multi_network_policy_resource branch from 31576cb to 46b27f0 Compare January 18, 2024 14:35

redhat-qe-bot removed changes-requested-myakove commented-Anatw labels Jan 18, 2024

redhat-qe-bot requested a review from myakove January 18, 2024 14:35

redhat-qe-bot removed the needs-rebase label Jan 18, 2024

rnetser requested changes Jan 18, 2024

View reviewed changes

redhat-qe-bot added the changes-requested-rnetser label Jan 18, 2024

redhat-qe-bot2 added the commented-rnetser label Feb 8, 2024

redhat-qe-bot1 added the commented-Anatw label Feb 8, 2024

rnetser approved these changes Feb 8, 2024

View reviewed changes

redhat-qe-bot added the approved-rnetser label Feb 8, 2024

redhat-qe-bot2 added the needs-rebase label Feb 8, 2024

Merge branch 'main' into add_multi_network_policy_resource

103b429

redhat-qe-bot1 removed commented-rnetser approved-rnetser commented-Anatw labels Feb 8, 2024

redhat-qe-bot1 requested a review from rnetser February 8, 2024 12:51

redhat-qe-bot1 removed the needs-rebase label Feb 8, 2024

redhat-qe-bot2 added the verified label Feb 8, 2024

github-actions bot added Stale and removed Stale labels Feb 9, 2024

myakove approved these changes Feb 11, 2024

View reviewed changes

redhat-qe-bot2 added approved-myakove can-be-merged labels Feb 11, 2024

omrirh approved these changes Feb 11, 2024

View reviewed changes

redhat-qe-bot2 added the approved-omrirh label Feb 11, 2024

rnetser approved these changes Feb 11, 2024

View reviewed changes

redhat-qe-bot1 added the approved-rnetser label Feb 11, 2024

rnetser merged commit 81acadc into RedHatQE:main Feb 11, 2024

redhat-qe-bot2 pushed a commit that referenced this pull request Feb 11, 2024

Add MultiNetworkPolicy resource (#1573)

5c613eb

Signed-off-by: Anat Wax <awax@redhat.com>

redhat-qe-bot2 mentioned this pull request Feb 11, 2024

CherryPicked: [v4.15] Add MultiNetworkPolicy resource #1658

Merged

redhat-qe-bot2 added a commit that referenced this pull request Feb 11, 2024

Add MultiNetworkPolicy resource (#1573) (#1658)

eac8423

Signed-off-by: Anat Wax <awax@redhat.com> Co-authored-by: ‪Anat Wax <46938838+Anatw@users.noreply.github.com>

myakove pushed a commit that referenced this pull request Aug 11, 2025

Add MultiNetworkPolicy resource (#1573)

8edb835

Signed-off-by: Anat Wax <awax@redhat.com>

	f"{self.api_group}/policy-for": f"{self.namespace} /{self.network_name}"
	f"{self.api_group}/policy-for": f"{self.namespace}/{self.network_name}"

Add MultiNetworkPolicy resource #1573

Add MultiNetworkPolicy resource #1573

Uh oh!

Conversation

Anatw commented Jan 8, 2024

Short description:

More details:

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewer:

Bug:

Uh oh!

redhat-qe-bot commented Jan 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

myakove commented Jan 11, 2024

Uh oh!

myakove left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Anatw Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Anatw Jan 21, 2024 •

edited

Loading