Support namespaced {Sriov,SriovIB,OVS}Networks #894

zeeke · 2025-05-12T11:43:04Z

SriovNetwork,IbSriovNetwork,OVSNetwork object (XNetwork resources from now on) must be created in the operator's namespace and the .Spec.NetworkNamespace field defines where the
controller should create the NetworkAttachmentDefinition resource. This constraint can be a problem
in clusters where applications are managed by non cluster administrators.

These changes makes the genericNetworkReconciler to accept XNetwork resources in namespaces different than
the operator's one. In such cases, the field .Spec.NetworkNamespace must be empty, and a validating webhook
ensure this constraint.

Webhook validation for networks .Spec.NetworkNamespace field

Move [sriov] operator No SriovNetworkNodePolicy ... test cases to its
own test file.

Implement test case to verify controller and webhook logic

github-actions · 2025-05-12T11:43:13Z

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2025-05-12T13:01:15Z

Pull Request Test Coverage Report for Build 17231697992

Details

73 of 141 (51.77%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 61.864%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/generic_network_controller.go	68	79	86.08%
pkg/webhook/validate_networks.go	5	23	21.74%
pkg/webhook/webhook.go	0	39	0.0%

Totals
Change from base Build 16749504687:	-0.1%
Covered Lines:	8656
Relevant Lines:	13992

💛 - Coveralls

Copilot

Pull Request Overview

This PR enhances the support for namespaced XNetwork resources by allowing network objects (SriovNetwork, SriovIBNetwork, OVSNetwork) to be created in namespaces other than the operator’s, enforcing constraints via a validating webhook and related tests. Key changes include:

Refactoring network node policy tests by moving them to a dedicated file.
Adding webhook validation and tests for the .Spec.NetworkNamespace field on network objects.
Updating controller reconciliation logic to handle resources in non-operator namespaces and improve finalizer handling.
Minor adjustments in logging and manifests for webhook configuration.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/conformance/tests/test_sriov_operator.go	Removed redundant “No SriovNetworkNodePolicy” tests to be replaced by a separate file.
test/conformance/tests/test_no_policy.go	Added dedicated tests for validating network policies and namespace constraints.
pkg/webhook/webhook.go	Implemented validation cases for the three network types using json.Unmarshal.
pkg/webhook/validate_networks*.go, validate_networks_test.go	Introduced functions and tests for enforcing .Spec.NetworkNamespace constraints.
controllers/generic_network_controller.go	Updated reconciliation logic to fetch objects from both operator and resource namespaces, update finalizers appropriately, and set owner references when applicable.
controllers/sriovnetwork_controller_test.go	Added new test cases to verify proper behavior when network objects are created in non-operator namespaces.
cmd/webhook/main.go & bindata/manifests/operator-webhook/003-webhook.yaml	Adjusted logging initialization and expanded webhook configuration for new resource types.

Comments suppressed due to low confidence (1)

test/conformance/tests/test_no_policy.go:182

The parameter name 'webookEnabled' appears to be a typo; consider renaming it to 'webhookEnabled' for improved clarity.

DescribeTable("should gracefully restart quickly", func(webookEnabled bool) {

controllers/generic_network_controller.go

SchSeba

great work!

I left some small comments

SchSeba · 2025-06-12T12:52:25Z

controllers/generic_network_controller.go

+	operatorNamespacedName := req.NamespacedName
+	operatorNamespacedName.Namespace = vars.Namespace
+
+	err = r.Get(ctx, operatorNamespacedName, instance)


why we need to also check in the operator namespace?
can you please add a comment to explain why so I will remember next time I look at this code :P

this is because we reconcile NetworkAttachmentDefinitions updates, and here we have to fetch the source object.

the cases are

|| Reconciled object || source object || | netAttachDef(nsX/net1) | SriovNetwork(operatorNs/net1) with networkNamespace: nsX | | netAttachDef(nsX/net1) | SriovNetwork(nsX/net1) | | SriovNetwork(operatorNs/net1) | SriovNetwork(operatorNs/net1) | | SriovNetwork(nsX/net1) | SriovNetwork(nsX/net1) |

adding a comment

can we handle this case in an eventHandler registered in SetupWithManager ?

so instead of using handler.EnqueuRequestForObject{} we would implement our own with handler.EnqueueRequestFromMapFn there we can try to get the "source" object from current net-attach-def namespace or from the operator ns.

then Reconcile would get the correct object it needs to reconcile, WDYT ?

controllers/sriovnetwork_controller_test.go

pkg/webhook/validate_networks.go

SchSeba · 2025-06-12T13:14:07Z

pkg/webhook/validate_networks.go

+
+func validateNetworkNamespace(cr controllers.NetworkCRInstance) error {
+	if cr.GetNamespace() != vars.Namespace && cr.NetworkNamespace() != "" {
+		return fmt.Errorf(".Spec.NetworkNamespace field can't be specified if the resource is not in the %s namespace", vars.Namespace)


also do we want to block the creation there is already a network from the operator namespace with the same name?

or if there already a net-attach-def on the requested namespace?

That's a little bit debatable: having two or more objects contending the same NetAttachDef is handled in

networks: Add Owner annotation to NetAttachDefs #897

It's the same problem, viewed from the perspective of the webhook, and we already have the same with an SriovNetwork + SriovIBNetwork pointing to the same object.

I prefer to address it in #897 or in another subsequent PR, after discussing in a community meeting. WDYT?

works for me

test/conformance/tests/test_no_policy.go

SchSeba · 2025-06-12T13:16:44Z

test/conformance/tests/test_no_policy.go

+			})
+		})
+
+		DescribeTable("should gracefully restart quickly", func(webookEnabled bool) {


This is the one that I removed so can you leave it in the original place?

To leave it where it was, I have to put back all the No SriovNetworkPolicy test tree. We'll get a conflict in any case and I'll solve it. Let's merge

remove leader election functional test #901

before this

now that 901 is merged I think we can remove this one

SchSeba · 2025-06-12T13:17:14Z

test/conformance/tests/test_no_policy.go

+		})
+
+		Context("Namespaced network objects", func() {
+			BeforeAll(func() {


nit: can you remove this empty one?

SchSeba · 2025-06-12T13:19:28Z

test/conformance/tests/test_no_policy.go

+			)
+		})
+	})
+})


can you add a check here that if network in the operator namespace exist it should win always ones created by the user on it's own namespace

This case is handled in

networks: Add Owner annotation to NetAttachDefs #897

but no such priority has been implemented yet. That PR handles these collisions by making the first object win.

for example, if a namespaced network is created, then the created netattachdef has the owner annotation. When the operatornamespaced network is created, it finds the target netattachdefs already owned by another object and the controller does nothing.

sounds good

adrianchiris · 2025-07-03T12:21:51Z

pkg/webhook/validate_networks_test.go

+	defer func(previous string) { vars.Namespace = previous }(vars.Namespace)
+	vars.Namespace = "operator-namespace"
+
+	validNetworkNamespaces := []controllers.NetworkCRInstance{


any chance to have a "[]tcase" (list of test case struct) which contains the test name, the NetworkCRInstance and a shouldFail boolean ? then the test would iterate over entries , run validateNetworkNamespace and compare to expected result ?

adrianchiris · 2025-07-03T12:31:22Z

controllers/generic_network_controller.go

+	if instance.NetworkNamespace() != "" && instance.GetNamespace() != vars.Namespace {
+		reqLogger.Error(
+			fmt.Errorf("bad value for NetworkNamespace"),
+			".Spec.NetworkNamespace can't be specified if the resource belongs to a namespace other than the operator's",


nit do we want to specify "jsonpath" style field refs ?

e.g .spec.networkNamespace, .metadata.namespace ?

yep, that would be better for the user point of view

adrianchiris · 2025-07-03T12:47:08Z

controllers/generic_network_controller.go

+	operatorNamespacedName := req.NamespacedName
+	operatorNamespacedName.Namespace = vars.Namespace
+
+	err = r.Get(ctx, operatorNamespacedName, instance)


can we handle this case in an eventHandler registered in SetupWithManager ?

so instead of using handler.EnqueuRequestForObject{} we would implement our own with handler.EnqueueRequestFromMapFn there we can try to get the "source" object from current net-attach-def namespace or from the operator ns.

then Reconcile would get the correct object it needs to reconcile, WDYT ?

SchSeba

from my side LGTM.

to me it's not an hard request to have the object retreval in the handler, but it's nice to have.

zeeke · 2025-07-07T11:01:04Z

I added the handleNetAttachDef function and it actually made the logic a little straightforward.

@adrianchiris @SchSeba please take a look

SchSeba

I link this new approach!

I learned something new :)

LGTM

controllers/generic_network_controller.go

SchSeba · 2025-07-22T10:54:03Z

Hi @zeeke,

our QE did the following test on this PR

Create sriovNetwork/net1 in ns1
Operator creates NAD/net1 in ns1
Update sriovNetwork/net1 with new resourceName
NAD/net1 is also updated (but no changes in spec as expected)

And on the last step when he check the NAD/net1 the owner reference was removed. So we probably have an issue with the update.

e0ne · 2025-07-22T12:21:56Z

controllers/generic_network_controller.go

+		return reconcile.Result{}, nil
+	}
+
+	if instance.NetworkNamespace() != "" && instance.GetNamespace() != vars.Namespace {


do we want to add error field into *NetworkStatus objects?

It's a good idea, but I would discuss it after

design proposal for conditions in operator CRDs #918

Maybe the conditions on CRDs are enough. What do you think?

zeeke · 2025-07-30T09:40:23Z

Hi @zeeke,

our QE did the following test on this PR

Create sriovNetwork/net1 in ns1

Operator creates NAD/net1 in ns1

Update sriovNetwork/net1 with new resourceName

NAD/net1 is also updated (but no changes in spec as expected)

And on the last step when he check the NAD/net1 the owner reference was removed. So we probably have an issue with the update.

fixed.

@adrianchiris @e0ne @SchSeba please, take another look

zeeke · 2025-08-18T15:04:20Z

@adrianchiris @e0ne can you please take another look at this PR?

adrianchiris

final nits otherwise LGTM

adrianchiris · 2025-08-21T14:51:52Z

controllers/generic_network_controller.go

 			// Request object not found, could have been deleted after reconcile request.
 			// Owned objects are automatically garbage collected. For additional cleanup logic use finalizers.
 			// Return and don't requeue
+			reqLogger.Error(err, "XXX2")


please remove debug prints ::)

sure! sorry for that leftover

adrianchiris · 2025-08-21T14:51:56Z

controllers/generic_network_controller.go

+		// Request object not found, could have been deleted after reconcile request.
+		// Owned objects are automatically garbage collected. For additional cleanup logic use finalizers.
+		// Return and don't requeue
+		reqLogger.Info("XXX3")


please remove debug prints ::)

adrianchiris · 2025-08-21T14:58:55Z

controllers/sriovnetwork_controller_test.go

+				Expect(k8sClient.Create(context.Background(), nsBlue)).ToNot(HaveOccurred())
+				DeferCleanup(func() {
+					By("deleting ns-blue")
+					err := k8sClient.Delete(ctx, nsBlue)


with envtest you cant really delete namespace, it will just stay dangling.

https://book.kubebuilder.io/reference/envtest#namespace-usage-limitation

I didn't know that, thanks!

I added a cleanNetworksInNamespace function to remove all the resources after the test

adrianchiris · 2025-08-21T15:01:08Z

controllers/sriovnetwork_controller_test.go

+
+				Eventually(func(g Gomega) {
+					netAttDef = &netattdefv1.NetworkAttachmentDefinition{}
+					err = util.WaitForNamespacedObject(netAttDef, k8sClient, "ns-blue", cr.GetName(), util.RetryInterval, util.Timeout)


nit: no need to call this wait func, object already exists, you can just call g.Expect(dynclient.Get(....)).To(Succeed())
its all wrapped in Eventually block anyway

adrianchiris · 2025-08-21T15:01:33Z

controllers/sriovnetwork_controller_test.go

+
+				// Delete the SriovNetwork
+				err = k8sClient.Delete(ctx, &cr)
+				Expect(err).NotTo(HaveOccurred())


lets wait for the underlying net-attach-def to be deleted ?

adrianchiris · 2025-08-21T15:03:39Z

controllers/sriovnetwork_controller_test.go

 				Should(Succeed())
 		})
+
+		Context("When the SriovNetwork namespace is not equal to the operator one", func() {


can you add a case where the user requests to create nad in a different ns than the current one ?
then check nad doesnt get created with a Consistently block ?

adrianchiris · 2025-08-21T15:08:24Z

controllers/generic_network_controller.go

+	instance := r.controller.GetObject()
+	nadNamespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
+
+	log.Log.WithName("XXX").Info("handleNetAttDef", "nadNamespacedName", nadNamespacedName, "vars.Namespace", vars.Namespace)


nit: remove debug logs (XXX/XXX11 etc)

adrianchiris · 2025-08-24T06:29:35Z

controllers/sriovnetwork_controller_test.go

+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+
+	k8sClient.DeleteAllOf(ctx, &netattdefv1.NetworkAttachmentDefinition{}, client.InNamespace(namespace))
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())


can you also wait for both objects to be gone from the kubernetes api ?

sure, I put the delete and list calls in an eventually block

adrianchiris · 2025-08-26T05:56:05Z

controllers/sriovnetwork_controller_test.go

+	ctx := context.Background()
+	EventuallyWithOffset(1, func(g Gomega) {
+		err := k8sClient.DeleteAllOf(ctx, &sriovnetworkv1.SriovNetwork{}, client.InNamespace(namespace))
+		g.Expect(1, err).NotTo(HaveOccurred())


g.Expect(err) ? here and below

adrianchiris · 2025-08-26T05:56:26Z

lgtm, lets have UT pass

SriovNetwork,IbSriovNetwork,OVSNetwork object (XNetwork resources from now on) must be created in the operator's namespace and the `.Spec.NetworkNamespace` field defines where the controller should create the NetworkAttachmentDefinition resource. This constraint can be a problem in clusters where applications are managed by non cluster administrators. These changes makes the `genericNetworkReconciler` to accept XNetwork resources in namespaces different than the operator's one. In such cases, the field `.Spec.NetworkNamespace` must be empty, and a validating webhook ensure this constraint. Signed-off-by: Andrea Panattoni <[email protected]>

Signed-off-by: Andrea Panattoni <[email protected]>

to avoid panic: ``` panic: /tmp/go-build1586768671/b001/exe/webhook flag redefined: alsologtostderr goroutine 1 [running]: flag.(*FlagSet).Var(0xc0000781c0, {0x3daf2d0, 0x5566969}, {0x394a710, 0xf}, {0x39e87ee, 0x49}) /usr/lib/golang/src/flag/flag.go:1029 +0x3b9 k8s.io/klog/v2.InitFlags.func1(0xc000234990?) /home/apanatto/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:447 +0x31 flag.(*FlagSet).VisitAll(0xc0003f4c40?, 0xc000785de0) /usr/lib/golang/src/flag/flag.go:458 +0x42 k8s.io/klog/v2.InitFlags(0x3dc1aa0?) /home/apanatto/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:446 +0x3c main.init.0() /home/apanatto/dev/github.com/k8snetworkplumbingwg/sriov-network-operator/cmd/webhook/main.go:27 +0x15 exit status 2 ``` Signed-off-by: Andrea Panattoni <[email protected]>

Move `[sriov] operator No SriovNetworkNodePolicy ...` test cases to its own test file. Implement test case to verify controller and webhook logic Signed-off-by: Andrea Panattoni <[email protected]>

adrianchiris

LGTM, feel free to merge this one @zeeke !

zeeke force-pushed the us/namespaced-networks branch 2 times, most recently from 000da88 to 53198d5 Compare May 12, 2025 12:49

zeeke force-pushed the us/namespaced-networks branch 2 times, most recently from a169538 to cd2efd9 Compare May 13, 2025 10:34

zeeke force-pushed the us/namespaced-networks branch from cd2efd9 to fe1f3a0 Compare May 30, 2025 14:17

zeeke mentioned this pull request Jun 6, 2025

networks: Add Owner annotation to NetAttachDefs #897

Merged

SchSeba requested review from SchSeba and Copilot June 10, 2025 13:07

Copilot AI reviewed Jun 10, 2025

View reviewed changes

controllers/generic_network_controller.go Outdated Show resolved Hide resolved

SchSeba reviewed Jun 12, 2025

View reviewed changes

zeeke force-pushed the us/namespaced-networks branch from fe1f3a0 to 1706fc6 Compare June 13, 2025 10:27

zeeke requested a review from SchSeba June 25, 2025 14:45

adrianchiris reviewed Jul 3, 2025

View reviewed changes

SchSeba approved these changes Jul 6, 2025

View reviewed changes

zeeke force-pushed the us/namespaced-networks branch from 1706fc6 to d38c8dc Compare July 7, 2025 09:49

SchSeba approved these changes Jul 7, 2025

View reviewed changes

adrianchiris reviewed Jul 14, 2025

View reviewed changes

controllers/generic_network_controller.go Show resolved Hide resolved

adrianchiris reviewed Jul 14, 2025

View reviewed changes

controllers/generic_network_controller.go Outdated Show resolved Hide resolved

e0ne reviewed Jul 22, 2025

View reviewed changes

zeeke force-pushed the us/namespaced-networks branch 2 times, most recently from b61c76b to c0347db Compare August 5, 2025 13:11

zeeke requested review from adrianchiris and e0ne August 11, 2025 08:57

adrianchiris reviewed Aug 21, 2025

View reviewed changes

zeeke force-pushed the us/namespaced-networks branch from c0347db to 8e133f4 Compare August 22, 2025 10:59

adrianchiris reviewed Aug 24, 2025

View reviewed changes

zeeke force-pushed the us/namespaced-networks branch from 8e133f4 to d260c44 Compare August 25, 2025 08:34

adrianchiris reviewed Aug 26, 2025

View reviewed changes

adrianchiris approved these changes Aug 26, 2025

View reviewed changes

zeeke added 4 commits August 26, 2025 09:52

Webhook validation for networks .Spec.NetworkNamespace field

5b30784

Signed-off-by: Andrea Panattoni <[email protected]>

Namespaced network object end2end tests

ada185c

Move `[sriov] operator No SriovNetworkNodePolicy ...` test cases to its own test file. Implement test case to verify controller and webhook logic Signed-off-by: Andrea Panattoni <[email protected]>

zeeke force-pushed the us/namespaced-networks branch from d260c44 to ada185c Compare August 26, 2025 07:52

adrianchiris approved these changes Aug 26, 2025

View reviewed changes

zeeke merged commit 35e7319 into k8snetworkplumbingwg:master Aug 27, 2025
13 of 14 checks passed

Support namespaced {Sriov,SriovIB,OVS}Networks #894

Support namespaced {Sriov,SriovIB,OVS}Networks #894

Uh oh!

Conversation

zeeke commented May 12, 2025

Uh oh!

github-actions bot commented May 12, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17231697992

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

SchSeba 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!

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!

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!

SchSeba left a comment

Choose a reason for hiding this comment

Uh oh!

zeeke commented Jul 7, 2025

Uh oh!

SchSeba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SchSeba commented Jul 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeke commented Jul 30, 2025

Uh oh!

zeeke commented Aug 18, 2025

Uh oh!

coveralls commented May 12, 2025 •

edited

Loading