net, admitter: Move (all) network validation admitters under `network` pkg. #11698

EdDev · 2024-04-12T07:23:49Z

What this PR does

Move and when needed adjust network validation admitters to the network/admitter package.

The logic is now properly owned and organized for the sig-network.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

NONE

Validators that check that the network name is unique and that each network has a counterpart interface have been moved and adjusted to the `network/admitter` package. Signed-off-by: Edward Haas <edwardh@redhat.com>

Testing uniqueness of a port name between different interfaces is irrelevant because there can be only a single pod network. The existing validation checks the duplication for pod network interfaces only. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2024-04-12T15:55:20Z

/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-kind-sriov

ormergi

Thanks for this clean up PR, awesome work

Overall looks good to me please see my comments.
nit are not a must.

ormergi · 2024-04-16T14:25:27Z

pkg/network/admitter/netiface_test.go

+				},
+			},
+			{
+				Name: "default",


nit: I suggest to not duplicate the network test here, its a bit confusing becuase this test is about interfaces name duplication but it can also fail due to network name duplications.

It is not better and not worse than placing here a single network or two networks of a different name. In all scenarios we will get a combination or failures.

Therefore, I chose this one which seems simple enough.

ormergi · 2024-04-16T14:26:53Z

pkg/network/admitter/netiface_test.go

+			{
+				Name:                   "default",
+				InterfaceBindingMethod: v1.InterfaceBindingMethod{Bridge: &v1.InterfaceBridge{}},
+			},


nit: Mix and match using v1.DefaultBridgeNetworkInterface and having in-place interface definition is a bit confusing.
You can simulate duplication by using *v1.DefaultBridgeNetworkInterface(), twice.
If you like to reflect the interface names better, I think it better to have additional in place interface definition (as in lines 84-88).

ormergi · 2024-04-16T14:48:08Z

pkg/network/admitter/binding.go

+	if iface.Binding != nil {
+		if hasInterfaceBindingMethod(iface) {


nit: I think we can reduce if clauses here:

Suggested change

if iface.Binding != nil {

if hasInterfaceBindingMethod(iface) {

if iface.Binding != nil && hasInterfaceBindingMethod(iface) {

ormergi · 2024-04-16T14:52:01Z

pkg/network/admitter/binding.go

@@ -52,3 +62,22 @@ func hasInterfaceBindingMethod(iface v1.Interface) bool {
 		iface.InterfaceBindingMethod.Macvtap != nil ||
 		iface.InterfaceBindingMethod.Passt != nil
 }
+
+func validateMasqueradeBinding(fieldPath *field.Path, idx int, iface v1.Interface, net v1.Network) []metav1.StatusCause {


nit: I think checking the iface use masquerade binding can be asked once and reused.

Sorry, I do not understand what you mean by this.

The masquerade binding check, at lines 66 and 73.

ormergi · 2024-04-16T15:05:50Z

pkg/network/admitter/passt.go

+	v1 "kubevirt.io/api/core/v1"
+)
+
+func validatePasstBinding(


Since passt iface API is deprecated in favor of Passt network binding plugin, I think it will be nice if this validation function would reflect it, and maybe recommend using the plugin.
Changing it on a follow up PR may be a better fit.

This is part of the passt core binding check, not about the binding plugin which is not specific to passt or any other binding. When we remove passt core binding, we will remove these as well.

ormergi · 2024-04-16T15:29:04Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

+	//networkNameMap := vmispec.IndexNetworkSpecByName(spec.Networks)

 	// Make sure the port name is unique across all the interfaces
-	portForwardMap := make(map[string]struct{})
+	//portForwardMap := make(map[string]struct{})

 	// Validate that each interface has a matching network
 	for idx, iface := range spec.Domain.Devices.Interfaces {

-		networkData, networkExists := networkNameMap[iface.Name]
+		//networkData, networkExists := networkNameMap[iface.Name]

-		causes = append(causes, validatePortConfiguration(field, networkExists, &networkData, iface, idx, portForwardMap)...)
+		//causes = append(causes, validatePortConfiguration(field, networkExists, &networkData, iface, idx, portForwardMap)...)


Shouldn't the commented line be removed?

Looks like leftovers, thanks.
Done.

ormergi · 2024-04-16T15:35:27Z

pkg/network/admitter/macvtap.go

+		causes = append(causes, metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Message: "Macvtap feature gate is not enabled",
+			Field:   fieldPath.Child("domain", "devices", "interfaces").Index(idx).Child("name").String(),


nit: I think the field path string can be reused

In this case, we will loose context in favor of a negligible optimization.
I think it is more readable this way and it is consistent with how we use it all over.

ormergi · 2024-04-16T15:36:36Z

pkg/network/admitter/passt.go

+		causes = append(causes, metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Message: "Passt feature gate is not enabled",
+			Field:   fieldPath.Child("domain", "devices", "interfaces").Index(idx).Child("name").String(),


nit: I think the field path string can be reused

Same answer as before.

ormergi · 2024-04-16T15:37:55Z

pkg/network/admitter/netiface.go

+					field.Child("domain", "devices", "interfaces").Index(idx).Child("name").String(),
+					iface.MacAddress,
+				),
+				Field: field.Child("domain", "devices", "interfaces").Index(idx).Child("macAddress").String(),


nit: I think the field path string can be reused

Same answer as before.

ormergi · 2024-04-16T15:38:33Z

pkg/network/admitter/netiface.go

+			causes = append(causes, metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueDuplicate,
+				Message: fmt.Sprintf("Duplicate name of the port: %s", forwardPort.Name),
+				Field:   field.Child("domain", "devices", "interfaces").Index(idx).Child("ports").Index(portIdx).Child("name").String(),


nit: Field path string can be reused

Same answer as before

Signed-off-by: Edward Haas <edwardh@redhat.com>

Two masquerade binding validations, that check it is set with pod network and is not using the reserved mac address, have been moved to the `network/admitter` package. Signed-off-by: Edward Haas <edwardh@redhat.com>

The unit tests have been using a cluster-config-checker stub that was originally focused on SLIRP tests. Later it was used to just satisfy other scenarios which had nothing to do with SLIRP. As new cluster-config methods are needed for other scenarios, the current stub is generalized by name and location. Signed-off-by: Edward Haas <edwardh@redhat.com>

The validation that checks an interface with bridge binding set on a pod network is moved to the `network/admitter` package. Signed-off-by: Edward Haas <edwardh@redhat.com>

Signed-off-by: Edward Haas <edwardh@redhat.com>

The passt core binding has a validation that checks that it is defined without any other interface in the spec. We support only L2 bindings on the secondary interfaces which use bridging to ling the pod interface to the VM. Therefore, there is no real limitation to have additional interfaces to passt. In addition, passt is targeted for removal after declared as deprecated in favor of the binding plugin version of it. Therefore, this single interface validation is removed. Signed-off-by: Edward Haas <edwardh@redhat.com>

Signed-off-by: Edward Haas <edwardh@redhat.com>

Move interface fields validation to the `network/admitter` package and adjust them to the new location. The following validations have been moved in this change: - Interface name format. - Interface model. - Interface mac address. - Interface PCI address. Signed-off-by: Edward Haas <edwardh@redhat.com>

The interface port validations are moved to `network/admitter` and refactored there to use test tables. Signed-off-by: Edward Haas <edwardh@redhat.com>

Tests have been converted to test tables and duplicate tests removed. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2024-04-18T04:50:33Z

change: Answered review.

enp0s3 · 2024-04-25T07:39:35Z

/approve

kubevirt-bot · 2024-04-25T07:39:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [enp0s3]

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

ormergi

LGTM

ormergi · 2024-04-25T10:08:50Z

pkg/network/admitter/binding.go

@@ -52,3 +62,22 @@ func hasInterfaceBindingMethod(iface v1.Interface) bool {
 		iface.InterfaceBindingMethod.Macvtap != nil ||
 		iface.InterfaceBindingMethod.Passt != nil
 }
+
+func validateMasqueradeBinding(fieldPath *field.Path, idx int, iface v1.Interface, net v1.Network) []metav1.StatusCause {


The masquerade binding check, at lines 66 and 73.

ormergi · 2024-04-25T10:11:30Z

pkg/network/admitter/binding.go

+	if iface.Binding != nil && !config.NetworkBindingPlugingsEnabled() {
+		return []metav1.StatusCause{{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Message: "Binding plugins feature gate is not enabled",


The original is wrong, this is a good opportunity to fix.

kubevirt-commenter-bot · 2024-04-25T10:12:56Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

EdDev · 2024-04-25T20:05:33Z

/test pull-kubevirt-e2e-k8s-1.27-sig-compute

EdDev · 2024-04-26T06:15:35Z

/retest-required

EdDev · 2024-04-26T12:49:43Z

/test pull-kubevirt-e2e-k8s-1.27-sig-network

kubevirt-bot · 2024-04-26T12:52:28Z

@EdDev: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-conformance-arm64	`f42ccc0`	link	false	`/test pull-kubevirt-conformance-arm64`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

EdDev added 2 commits April 12, 2024 10:21

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/network labels Apr 12, 2024

kubevirt-bot requested review from enp0s3 and ormergi April 12, 2024 07:24

EdDev changed the title ~~Admitters net move part3~~ net, admitter: Move (all) network validation admitters under network pkg. Apr 12, 2024

ormergi reviewed Apr 16, 2024

View reviewed changes

EdDev added 12 commits April 17, 2024 20:52

net, admitter: Relocated and adjust interface name uniqueness

89f5979

Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move and adjust the check about ifaces assign to networks

c0db2b1

Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move masquerade binding validations

71325ca

Two masquerade binding validations, that check it is set with pod network and is not using the reserved mac address, have been moved to the `network/admitter` package. Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move and adjust bridge binding validation

5d817d3

The validation that checks an interface with bridge binding set on a pod network is moved to the `network/admitter` package. Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move macvtap core binding validators

5eba0e9

Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move passt core binding validations

ddbe092

Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move binding plugin feature gate validation

ece33ae

Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move and adjust interface port validations

15b0a3c

The interface port validations are moved to `network/admitter` and refactored there to use test tables. Signed-off-by: Edward Haas <edwardh@redhat.com>

net, admitter: Move and adjust interface DHCP options validations

f42ccc0

Tests have been converted to test tables and duplicate tests removed. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev force-pushed the admitters-net-move-part3 branch from 423befe to f42ccc0 Compare April 18, 2024 04:50

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2024

ormergi approved these changes Apr 25, 2024

View reviewed changes

kubevirt-bot assigned ormergi Apr 25, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024

kubevirt-bot merged commit 570e503 into kubevirt:main Apr 26, 2024
37 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net, admitter: Move (all) network validation admitters under `network` pkg. #11698

net, admitter: Move (all) network validation admitters under `network` pkg. #11698

EdDev commented Apr 12, 2024

EdDev commented Apr 12, 2024

ormergi left a comment

ormergi Apr 16, 2024

EdDev Apr 17, 2024

ormergi Apr 16, 2024

EdDev Apr 17, 2024

ormergi Apr 16, 2024

EdDev Apr 17, 2024

ormergi Apr 16, 2024

EdDev Apr 17, 2024

ormergi Apr 25, 2024

ormergi Apr 16, 2024

EdDev Apr 18, 2024

ormergi Apr 16, 2024

EdDev Apr 18, 2024

ormergi Apr 16, 2024

EdDev Apr 17, 2024

ormergi Apr 16, 2024

EdDev Apr 18, 2024

ormergi Apr 16, 2024

EdDev Apr 18, 2024

ormergi Apr 16, 2024

EdDev Apr 18, 2024

EdDev commented Apr 18, 2024

enp0s3 commented Apr 25, 2024

kubevirt-bot commented Apr 25, 2024

ormergi left a comment

ormergi Apr 25, 2024

ormergi Apr 25, 2024

kubevirt-commenter-bot commented Apr 25, 2024

EdDev commented Apr 25, 2024

EdDev commented Apr 26, 2024

EdDev commented Apr 26, 2024

kubevirt-bot commented Apr 26, 2024 •

edited

		if iface.Binding != nil {
		if hasInterfaceBindingMethod(iface) {

	if iface.Binding != nil {
	if hasInterfaceBindingMethod(iface) {
	if iface.Binding != nil && hasInterfaceBindingMethod(iface) {

net, admitter: Move (all) network validation admitters under network pkg. #11698

net, admitter: Move (all) network validation admitters under network pkg. #11698

Conversation

EdDev commented Apr 12, 2024

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

EdDev commented Apr 12, 2024

ormergi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Apr 18, 2024

enp0s3 commented Apr 25, 2024

kubevirt-bot commented Apr 25, 2024

ormergi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Apr 25, 2024

EdDev commented Apr 25, 2024

EdDev commented Apr 26, 2024

EdDev commented Apr 26, 2024

kubevirt-bot commented Apr 26, 2024 • edited

net, admitter: Move (all) network validation admitters under `network` pkg. #11698

net, admitter: Move (all) network validation admitters under `network` pkg. #11698

kubevirt-bot commented Apr 26, 2024 •

edited