SDN-5519: OVN-K: Localnet API support #1750

ormergi · 2025-02-04T16:11:38Z

Introduce API for Localnet topology networks.

This proposal outlines the Localnet API utilizing OVN-Kubernetes ClusterUserDefinedNetwork (CUDN) CRD.
And extending the CUDN CRD to allow creating localnet topology networks.

https://issues.redhat.com/browse/SDN-5519

openshift-ci · 2025-02-04T16:11:46Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ormergi · 2025-02-06T17:33:36Z

/cc @maiqueb @ricardomaraschini

maiqueb

Minor changes requested.

From my perspective, you can start fishing for an SDN reviewer, and an API reviewer.

maiqueb · 2025-02-07T08:37:48Z

enhancements/network/ovn-k-localnet-api.md

+  - "@ormergi"
+reviewers:
+  - "@maiqueb"
+  - TBD


start networking this PR in the SDN team, and find an API reviewer from them (I'd suggest @npinaeva ) and an approver.

We also want someone involved from the API team asap. We don't want them getting involved too late.

maiqueb · 2025-02-07T08:39:00Z

enhancements/network/ovn-k-localnet-api.md

+
+## Proposal
+
+### Summary


another summary ?

I tried to express the proposal in few sentences to enable reviewers realize whats the proposed solution idea.

maiqueb · 2025-02-07T08:40:32Z

enhancements/network/ovn-k-localnet-api.md

+        bridge-mappings:
+        - localnet: physnet  <---- has to match the NAD config.spec.name. OR 
+                                   the NAD config.spec.physicalNetworkName
+          bridge: br-ex <--------- OVN-K switch


Suggested change

bridge: br-ex <--------- OVN-K switch

bridge: br-ex <--------- OVS switch

You're not forced to use br-ex. You can use nmstate to create and configure whatever other switch you want.

maiqueb · 2025-02-07T08:41:29Z

enhancements/network/ovn-k-localnet-api.md

+
+As of today the CUDN CRD controller does not allow mutating `spec.network`.
+In order to comply with NAD spec mutation support, the CUDN CRD controller should allow spec.network mutation for 
+localnet topologies, specifically MTU and VLAN.


plus the physicalNetworkName and ignored subnets.

maiqueb · 2025-02-07T08:42:29Z

enhancements/network/ovn-k-localnet-api.md

+
+The CUDN `spec.network` follows the  [discriminated-union](https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#discriminated-unions),
+convention. 
+The `spec.network.topology` serves as the union discriminator, it should accept `Locannet` option.


Suggested change

The `spec.network.topology` serves as the union discriminator, it should accept `Locannet` option.

The `spec.network.topology` serves as the union discriminator, it should accept `Localnet` option.

maiqueb · 2025-02-07T08:44:04Z

enhancements/network/ovn-k-localnet-api.md

+| VLAN                | The network VLAN ID.                                                                                                                                                                                                                                                                                  | Yes      |
+| Subnets             | List of CIDRs used for the pod network across the cluster.Dual-stack clusters may set 2 subnets (one for each IP family), otherwise only 1 subnet is allowed. The format should match standard CIDR notation (for example, "10.128.0.0/16"). This field must be omitted if `ipam.mode` is `Disabled`. | Yes      |
+| ExcludeSubnets      | List of CIDRs removed from the specified CIDRs in `subnets`.The format should match standard CIDR notation (for example, "10.128.0.0/16"). This field must be omitted if `subnetes` is unset or `ipam.mode` is `Disabled`.                                                                            | Yes      |
+| IPAM                | Contains IPAM-related configuration for the network. When ipam.lifecycle=Persistent enable workloads have persistent IP addresses. For example: Virtual Machines will have the same IP addresses along their lifecycle (stop, start migration, reboots).                                              | Yes      |


I'm conflicted. Should we actually move the subnets + excludesubnets attributes to IPAM ? ...

It kind of makes sense ... but would make it diverge from L2.

I suggested changing IPAM config in recent IPAM config API changes to have subents but unfortunately it wasn't accepted.

Its like you said if we change it for localnet topology configuration, it diverge from other typologies. I think we should avoid that 😬

we definitely want all network types to be consistent, and we discussed this (more than once :D) before.
I think one of the reason to leave it separate was our plan to use subnets field for egress setup but with disabled ipam, so subnets isn't necessarily a part of ipam

maiqueb · 2025-02-07T08:47:48Z

enhancements/network/ovn-k-localnet-api.md

+  - When Subnets consist of IPv6 CIDR, minimum MTU should be 1280.
+- `VLAN`:
+  - VLAN IDs `0` (untagged),`1` (default), highest VLAN ID is `4095`. 
+    Minimum: 2, Maximum: 4095.


I think the highest is 4094 actually. 4095 is reserved. Check for instance https://networkengineering.stackexchange.com/questions/24404/vlan-0-1-and-4095-are-reserved-what-are-they-reserved-for

Thanks, fixed. Also added ref.

maiqueb · 2025-02-07T08:48:27Z

enhancements/network/ovn-k-localnet-api.md

+        "topology": "localnet",
+        "name": "cluster.udn.test-net",
+        "physicalNetworkName: "tenantblue",
+	    "mtu": 1500,


nit: could you fix the formatting ?

ormergi · 2025-02-09T12:28:50Z

Changes Address #1750 (review)

maiqueb

Requesting reviews from our peers .

maiqueb · 2025-02-10T10:09:17Z

enhancements/network/ovn-k-localnet-api.md

+  - "@maiqueb"
+  - TBD
+approvers:
+  - TBD


@JoelSpeed could you review this API ? We've tried to keep it really small and focused on the API itself.

Or find someone on your team that could lend a hand.

maiqueb · 2025-02-10T10:10:02Z

enhancements/network/ovn-k-localnet-api.md

+  - "@ormergi"
+reviewers:
+  - "@maiqueb"
+  - TBD


@npinaeva you're my go-to API person in the SDN team.

Can we beg you for some reviews ?... Or has tried to keep this really short.

maiqueb · 2025-02-10T10:11:29Z

@ormergi you need to fix the markdown linter ...

npinaeva

makes sense!

npinaeva · 2025-02-10T10:48:47Z

enhancements/network/ovn-k-localnet-api.md

+- `VLAN`: <br/>
+  According to [dot1q (IEEE 802.1Q)](https://ieeexplore.ieee.org/document/10004498), 
+  VID (VLAN ID) is 12-bits field, providing 4096 values; 0 - 4095. <br/>
+  The VLAN IDs `0` and `4095` are reserved, `1` is the default. <br/> 


what does "1 is the default" mean? I thought default option was unspecified?

VLAN ID 1 is also reserved, it is used for management and usually the default. Sorry for confusion.
Fixed.

npinaeva · 2025-02-10T10:55:53Z

enhancements/network/ovn-k-localnet-api.md

+// +kubebuilder:validation:XValidation:rule="!has(self.ipam) || !has(self.ipam.mode) || self.ipam.mode != 'Disabled' || !has(self.subnets)", message="Subnets must be unset when ipam.mode is Disabled"
+// +kubebuilder:validation:XValidation:rule="!has(self.ipam) || !has(self.ipam.mode) || self.ipam.mode != 'Disabled' || self.role == 'Secondary'", message="Disabled ipam.mode is only supported for Secondary network"
+// +kubebuilder:validation:XValidation:rule="!has(self.subnets) || !has(self.mtu) || !self.subnets.exists_one(i, isCIDR(i) && cidr(i).ip().family() == 6) || self.mtu >= 1280", message="MTU should be greater than or equal to 1280 when IPv6 subent is used"
+// +kubebuilder:validation:XValidation:rule="!has(self.excludeSubnets) || has(self.excludeSubnets) && !has(self.subnets)", message="ExcludeSubnets must be unset when subents is unset"


I don't know if we want to review CEL validation here or in the implementation PR, but I think this should be
!has(self.excludeSubnets) || has(self.subnets) not sure if we want to add IP family validation, may be costly for 25 items

I removed CEL validation rules for now, we can revisit them in the implementation PR.

Thanks for the suggestion on this rule, noted 🙂

npinaeva · 2025-02-10T10:58:23Z

enhancements/network/ovn-k-localnet-api.md

+    // The format should match standard CIDR notation (for example, "10.128.0.0/16").
+    // This field must be omitted if `subnetes` is unset or `ipam.mode` is `Disabled`.
+    //
+    // +optional


this will need its own type, DualStackCIDRs has MaxItems=2

Good catch!
I changed it to []CIDR.

npinaeva · 2025-02-10T10:59:20Z

enhancements/network/ovn-k-localnet-api.md

+
+    // VLAN is the VLAN ID.
+    //
+    // +kubebuilder:validation:Minimum=2


So what happened to vlan=1?

VLAN ID 1 is also reserved by the standard, used for management.
The bridge (the bridging entity, e.g.: switch, linux-bridge) defines each port with VLAN 1if not spesifed.

npinaeva · 2025-02-10T11:02:06Z

enhancements/network/ovn-k-localnet-api.md

+| VLAN                | The network VLAN ID.                                                                                                                                                                                                                                                                                  | Yes      |
+| Subnets             | List of CIDRs used for the pod network across the cluster.Dual-stack clusters may set 2 subnets (one for each IP family), otherwise only 1 subnet is allowed. The format should match standard CIDR notation (for example, "10.128.0.0/16"). This field must be omitted if `ipam.mode` is `Disabled`. | Yes      |
+| ExcludeSubnets      | List of CIDRs removed from the specified CIDRs in `subnets`.The format should match standard CIDR notation (for example, "10.128.0.0/16"). This field must be omitted if `subnetes` is unset or `ipam.mode` is `Disabled`.                                                                            | Yes      |
+| IPAM                | Contains IPAM-related configuration for the network. When ipam.lifecycle=Persistent enable workloads have persistent IP addresses. For example: Virtual Machines will have the same IP addresses along their lifecycle (stop, start migration, reboots).                                              | Yes      |


we definitely want all network types to be consistent, and we discussed this (more than once :D) before.
I think one of the reason to leave it separate was our plan to use subnets field for egress setup but with disabled ipam, so subnets isn't necessarily a part of ipam

npinaeva · 2025-02-10T11:03:14Z

enhancements/network/ovn-k-localnet-api.md

+    // Secondary network is only assigned to pods that use `k8s.v1.cni.cncf.io/networks` annotation to select given network.
+    //
+    // +kubebuilder:validation:Required
+    // +kubebuilder:validation:XValidation:rule="self != 'Secondary'", message="Localnet topology is supported for secondary networks only."


I think it makes sense to leave role here explicitly, but is it ever possible to enable this for primary network?

We have no plans, and IMHO, the use-case is pointless.

It doesn't need egress, it will not have SNAT to the node's IP addresses, etc ...

I don't see it happening.

ormergi

@npinaeva PTAL

ormergi · 2025-02-12T01:35:08Z

enhancements/network/ovn-k-localnet-api.md

+- `VLAN`: <br/>
+  According to [dot1q (IEEE 802.1Q)](https://ieeexplore.ieee.org/document/10004498), 
+  VID (VLAN ID) is 12-bits field, providing 4096 values; 0 - 4095. <br/>
+  The VLAN IDs `0` and `4095` are reserved, `1` is the default. <br/> 


VLAN ID 1 is also reserved, it is used for management and usually the default. Sorry for confusion.
Fixed.

ormergi · 2025-02-12T01:45:47Z

enhancements/network/ovn-k-localnet-api.md

+// +kubebuilder:validation:XValidation:rule="!has(self.ipam) || !has(self.ipam.mode) || self.ipam.mode != 'Disabled' || !has(self.subnets)", message="Subnets must be unset when ipam.mode is Disabled"
+// +kubebuilder:validation:XValidation:rule="!has(self.ipam) || !has(self.ipam.mode) || self.ipam.mode != 'Disabled' || self.role == 'Secondary'", message="Disabled ipam.mode is only supported for Secondary network"
+// +kubebuilder:validation:XValidation:rule="!has(self.subnets) || !has(self.mtu) || !self.subnets.exists_one(i, isCIDR(i) && cidr(i).ip().family() == 6) || self.mtu >= 1280", message="MTU should be greater than or equal to 1280 when IPv6 subent is used"
+// +kubebuilder:validation:XValidation:rule="!has(self.excludeSubnets) || has(self.excludeSubnets) && !has(self.subnets)", message="ExcludeSubnets must be unset when subents is unset"


I removed CEL validation rules for now, we can revisit them in the implementation PR.

Thanks for the suggestion on this rule, noted 🙂

ormergi · 2025-02-12T01:48:27Z

enhancements/network/ovn-k-localnet-api.md

+    // The format should match standard CIDR notation (for example, "10.128.0.0/16").
+    // This field must be omitted if `subnetes` is unset or `ipam.mode` is `Disabled`.
+    //
+    // +optional


Good catch!
I changed it to []CIDR.

ormergi · 2025-02-12T01:50:33Z

enhancements/network/ovn-k-localnet-api.md

+
+    // VLAN is the VLAN ID.
+    //
+    // +kubebuilder:validation:Minimum=2


VLAN ID 1 is also reserved by the standard, used for management.
The bridge (the bridging entity, e.g.: switch, linux-bridge) defines each port with VLAN 1if not spesifed.

ormergi · 2025-02-12T02:16:28Z

Changes: address #1750 (review)

ormergi · 2025-02-12T02:17:31Z

@trozet could you please have a look? 🙏

everettraven · 2025-02-12T18:02:58Z

enhancements/network/ovn-k-localnet-api.md

+    // +required
+    Network NetworkSpec `json:"network"`


Is spec.network already a required field today?

Yes, please see CUDN CRD definition here https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/cudn.go#L37

everettraven · 2025-02-12T18:05:55Z

enhancements/network/ovn-k-localnet-api.md

+```go
+// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="Network spec is immutable"
+```
+This validation should be changed to allow mutation localnet topology configurations, at least MTU and VLAN.


Are you saying that spec.network should not be immutable only if the spec.network.topology is set to Localnet?

How do you plan to implement that change to the validation?

Yes, and let me add some context to allowing spec mutations for localnet topology:
Allowing spec mutation for localnet motivation followed by recorded customer cases describing the current API limitations flaws (e.g.: immutable spec) and impact on UX (bad) https://issues.redhat.com/browse/PLMCORE-10896.
For starters we want to allow spec mutation mostly for the following fields: VLAN, MTU, excludeSubnets and physicalNetworkName.
Having it localnet topology settings mutable improve the UX because it save re-creating CRs in case of misconfiguration, and allow changes to the topology settings on day2.

Regarding to CEL rule validations required changes, what I had in mind is inverting the spec mutation validation we have today on spec.network as follows:

Remove mutation restrictions on spec.network.

Add CEL rule validations to restrict mutations of spec.network.topology, spec.network.layer2, and spec.network.layer3.

Add CEL rule validations to restrict mutation of localnet topology settings: spec.network.localnet.role, spec.network.localnet.subnetes and spec.network.topology.ipam.

Depending on OVN-K maintainers feedback we can, do broader change and allow spec mutations for other topologies as well (layer2 and layer3), naturally it will simplify the required CEL rule validations.

I though we better start with hardest restrictions we can that make sense, and upon necessity remove them later on, keeping things backward compatible.
WDYT?

I think that approach sounds reasonable. I do think it would be worth explicitly including this approach in the enhancement.

Added text about allowing spec mutations for localnet configurations, at the API level.

everettraven · 2025-02-12T18:12:17Z

enhancements/network/ovn-k-localnet-api.md

+    //
+    // +kubebuilder:validation:Required
+    // +required
+    Role NetworkRole `json:"role"`


Is the NetworkRole a type that already exists today? Does it have the appropriate enum tags to limit the allowed values to only Secondary?

Yes, NetworkRole type exist and limited to Primary and Secondary,
please see https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L170.

Are you intending for this field to also allow setting Primary and Secondary?

Please see #1750 (comment)

For localnet typologies we dont see it happen in the near future, hence Secondary only for now.
Since we want to maintain similar semantics to exiting typologies, this field should remain required.
Make sense?

Clarifying here: there is no use case for having a primary UDN whose topology is localnet.

@everettraven what does the openshift API guidelines say about this scenario ?... What is more important ? Consistency across the API (forcing the user to specify always the same value) or make the user provide less input ?

My specific concern here is that you specify that a user can only set Secondary in the Godoc, but the type you are using has validation that allows setting both Primary and Secondary. There is no additional validation that I see enforces that this must only be set to Secondary. Without that explicit validation in place your Godoc is not accurate.

I added additional validation to ensure only Secondary value is accepted at the LocalnetConfig NetworkRole definition,
please see https://github.com/openshift/enhancements/compare/03fbe107f688b60a3e8fb8dda6d2f4f002b86746..1db8ce327af3d896fdbda53de337e098b8874d80#diff-7512349752d7cfc290273d99b4b60b705813d12aaf6d4db24cad883a630d7f9aR432

everettraven · 2025-02-12T18:14:08Z

enhancements/network/ovn-k-localnet-api.md

+    // +kubebuilder:validation:Required
+    // +required


Only use the +required tag.

Additionally, ensure it is clearly stated in the Godoc that the field is required. Users using something like oc explain ... can't see the tags.

everettraven · 2025-02-12T18:17:10Z

enhancements/network/ovn-k-localnet-api.md

+Localnet topology configuration type 
+```go
+type LocalnetConfig struct {
+    // Role describes the network role in the pod.


References to fields in godoc should use the serialized field name as per OpenShift API conventions: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-json-field-names-in-godoc

Suggested change

// Role describes the network role in the pod.

// role describes the network role in the pod.

What is a "network role" ? Is this something that users are expected to already be familiar with? Why would they care about setting this value? What does it let them achieve?

These are all questions that I would expect to have answered when reading the godoc for this field.

For more guidance on some things to consider to write user readable documentation in godoc, see https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc

What is a "network role" ? Is this something that users are expected to already be familiar with? Why would they care about setting this value? What does it let them achieve?

Network role control whether the pod interface will act as the primary or secondary. primary used as the default gateway for the pod. Its up to the user to deciede what kind of network they like to create.

Added text to godoc.

everettraven · 2025-02-12T18:49:33Z

enhancements/network/ovn-k-localnet-api.md

+
+    // IPAM section contains IPAM-related configuration for the network.
+    //
+    // +optional


Explicitly mention that this field is optional in the godoc.

everettraven · 2025-02-12T18:51:57Z

enhancements/network/ovn-k-localnet-api.md

+    // +optional
+    IPAM *IPAMConfig `json:"ipam,omitempty"`
+
+    // MTU is the maximum transmission unit for a network.


Suggested change

// MTU is the maximum transmission unit for a network.

// mtu is the maximum transmission unit for a network.

Why does a user care about setting this field? What does it allow them to achieve?

It enable users align the MTU used across the stack, fitting the MTU of the pod network interface to the MTU set on the node interface.

In a scenario physicalNetworkName points to a network interface who has certain MTU settings, e.g: 1600, the pod interface should have the exact same MTU as the node network interface.
Otherwise, the MTU wont be aligned across the stack (pod has MTU X node has MTU Y), it could result in network disruptions and bad performance.

Added text to godoc

everettraven · 2025-02-12T18:54:25Z

enhancements/network/ovn-k-localnet-api.md

+    // +kubebuilder:validation:Minimum=576
+    // +kubebuilder:validation:Maximum=65536


Explicitly mention the minimum and maximum values in the Godoc so users using something like oc explain ... can see the minimum and maximum constraints.

You mentioned previously in the enhancement that this should have a higher minimum value if the subnets are IPv6 - is there any way to enforce that at admission time? if not, how are planning to enforce it?

Building on this a bit more, if you aren't able to enforce this increased minimum based on subnet types (IPv4 vs IPv6) at admission time is it because of the API structure? is there a change you could make to the API structure to more easily enforce that at admission time?

You mentioned previously in the enhancement that this should have a higher minimum value if the subnets are IPv6 - is there any way to enforce that at admission time? if not, how are planning to enforce it?

It is enforced using CEL validation rule on Layer2 and Layer3 configuration types, please see
https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L93
https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L28

The proposed Localnet topology configuration type should have similar validation.

Added text to godoc about min/max constrains

everettraven · 2025-02-12T18:55:36Z

enhancements/network/ovn-k-localnet-api.md

+    // +optional
+    MTU int32 `json:"mtu,omitempty"`
+
+    // VLAN is the VLAN ID.


Suggested change

// VLAN is the VLAN ID.

// vlan is the VLAN ID.

Why does a user care about setting this field? What does it allow them to achieve?

In a scenerio physicalNetworkName points to a network who has some VLAN applied, it should be configured in the spec.
Otherwise pods connected to the subject network will not be able to communicate with other hosts on the network.

Added text to godoc

everettraven · 2025-02-12T18:55:54Z

enhancements/network/ovn-k-localnet-api.md

+    // +kubebuilder:validation:Minimum=2
+    // +kubebuilder:validation:Maximum=4094
+    // +optional


Explicitly mention these constraints in the godoc.

ormergi

@everettraven thank you so much for the feedback, I am working on resolving you comments.

ormergi · 2025-02-12T20:13:48Z

enhancements/network/ovn-k-localnet-api.md

+    // +required
+    Network NetworkSpec `json:"network"`


Yes, please see CUDN CRD definition here https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/cudn.go#L37

ormergi · 2025-02-12T20:41:31Z

enhancements/network/ovn-k-localnet-api.md

+```go
+// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="Network spec is immutable"
+```
+This validation should be changed to allow mutation localnet topology configurations, at least MTU and VLAN.


Yes, and let me add some context to allowing spec mutations for localnet topology:
Allowing spec mutation for localnet motivation followed by recorded customer cases describing the current API limitations flaws (e.g.: immutable spec) and impact on UX (bad) https://issues.redhat.com/browse/PLMCORE-10896.
For starters we want to allow spec mutation mostly for the following fields: VLAN, MTU, excludeSubnets and physicalNetworkName.
Having it localnet topology settings mutable improve the UX because it save re-creating CRs in case of misconfiguration, and allow changes to the topology settings on day2.

Regarding to CEL rule validations required changes, what I had in mind is inverting the spec mutation validation we have today on spec.network as follows:

Remove mutation restrictions on spec.network.

Add CEL rule validations to restrict mutations of spec.network.topology, spec.network.layer2, and spec.network.layer3.

Add CEL rule validations to restrict mutation of localnet topology settings: spec.network.localnet.role, spec.network.localnet.subnetes and spec.network.topology.ipam.

Depending on OVN-K maintainers feedback we can, do broader change and allow spec mutations for other topologies as well (layer2 and layer3), naturally it will simplify the required CEL rule validations.

I though we better start with hardest restrictions we can that make sense, and upon necessity remove them later on, keeping things backward compatible.
WDYT?

ormergi · 2025-02-12T20:58:15Z

enhancements/network/ovn-k-localnet-api.md

+    // Role describes the network role in the pod.
+    //
+    // Allowed value is "Secondary".
+    // Secondary network is only assigned to pods that use `k8s.v1.cni.cncf.io/networks` annotation to select given network.


The network will be defined as a secondary network, pods will need to specify Mutltus networks annotation in other to connect to that network. For example: given CR with metadata.name mynet, pod should have k8s.v1.cni.cncf.io/networks: mynet annotation.

What makes it work is the fact the CUDN controller creates a NAD at the target namespace, allowing pods to connect to it.
The corresponding NAD meta.name will be the same as the CUDN CR meta.name.

ormergi · 2025-02-12T21:00:38Z

enhancements/network/ovn-k-localnet-api.md

+    //
+    // +kubebuilder:validation:Required
+    // +required
+    Role NetworkRole `json:"role"`


Yes, NetworkRole type exist and limited to Primary and Secondary,
please see https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L170.

ormergi · 2025-02-12T21:05:11Z

enhancements/network/ovn-k-localnet-api.md

+    // +required
+    Role NetworkRole `json:"role"`
+
+    // PhysicalNetworkName the OVS bridge-mapping's network-name configured in the nodes.


The JSON tag name should be PhysicalNetworkName, sorry for the confusion I will fix it.

There is not interaction with other fields in this API.
There is an interaction with Kubernetes-nmsate NodeNetworkConfigurationPolicy (NNCP) API though:
the CUDN spec.network.localnet.physicalNetworkName should point to the NNCP spec.desiredState.ovn.bridge-mappings as mentioned earlier in the doc.

ormergi · 2025-02-12T21:54:23Z

enhancements/network/ovn-k-localnet-api.md

+    // +optional
+    ExcludeSubnets []CIDR `json:"excludeSubnets,omitempty"`
+
+    // IPAM section contains IPAM-related configuration for the network.


The IPAM stands for OVN-K IPAM configuration for the subject network, its implemented for Layer2 topology configuration type, please see [1] it has prerry nice godoc explaining what it does 🙂

The proposal is to have Localnet topology configuration type has similar ipam cofiguration type as Layer2 has.

ormergi · 2025-02-12T21:58:28Z

enhancements/network/ovn-k-localnet-api.md

+    // +kubebuilder:validation:Minimum=576
+    // +kubebuilder:validation:Maximum=65536


You mentioned previously in the enhancement that this should have a higher minimum value if the subnets are IPv6 - is there any way to enforce that at admission time? if not, how are planning to enforce it?

It is enforced using CEL validation rule on Layer2 and Layer3 configuration types, please see
https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L93
https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L28

The proposed Localnet topology configuration type should have similar validation.

ormergi · 2025-02-12T22:02:56Z

enhancements/network/ovn-k-localnet-api.md

+    // +optional
+    MTU int32 `json:"mtu,omitempty"`
+
+    // VLAN is the VLAN ID.


In a scenerio physicalNetworkName points to a network who has some VLAN applied, it should be configured in the spec.
Otherwise pods connected to the subject network will not be able to communicate with other hosts on the network.

ormergi · 2025-02-12T22:07:56Z

enhancements/network/ovn-k-localnet-api.md

+    // +optional
+    IPAM *IPAMConfig `json:"ipam,omitempty"`
+
+    // MTU is the maximum transmission unit for a network.


It enable users align the MTU used across the stack, fitting the MTU of the pod network interface to the MTU set on the node interface.

In a scenario physicalNetworkName points to a network interface who has certain MTU settings, e.g: 1600, the pod interface should have the exact same MTU as the node network interface.
Otherwise, the MTU wont be aligned across the stack (pod has MTU X node has MTU Y), it could result in network disruptions and bad performance.

ormergi · 2025-02-12T22:23:04Z

enhancements/network/ovn-k-localnet-api.md

+Localnet topology configuration type 
+```go
+type LocalnetConfig struct {
+    // Role describes the network role in the pod.


What is a "network role" ? Is this something that users are expected to already be familiar with? Why would they care about setting this value? What does it let them achieve?

Network role control whether the pod interface will act as the primary or secondary. primary used as the default gateway for the pod. Its up to the user to deciede what kind of network they like to create.

ormergi · 2025-02-18T14:26:33Z

Changes: resolve #1750 (review)

openshift-ci · 2025-02-18T14:29:32Z

[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 ask for approval from trozet. 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:

enhancements/network/OWNERS

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

npinaeva · 2025-02-18T14:41:12Z

Yes, you can mutate NADs. You only need to restart the workloads attached to it for them to consume the new version - assuming the network is in-sync.

thank you for clarifying! I think this part should go in the enhancement, considering it is the first time we allow to mutate spec. Otherwise /lgtm

@npinaeva its mentioned here, is it fine or you expect something else?

I mean the fact that you need to restart workloads to consume the new changes

ormergi · 2025-02-19T14:09:06Z

Changes: resolve #1750 (comment)

ormergi · 2025-02-19T14:10:00Z

I mean the fact that you need to restart workloads to consume the new changes

@npinaeva I have added text to clarify this point please see https://github.com/openshift/enhancements/compare/dca226ce6916f77252e10f97ad4e5a9093596245..c44363a24ffebf59b4d2cafd3584499da82afb06

everettraven · 2025-02-19T19:56:54Z

enhancements/network/ovn-kubernetes-localnet-api.md

+    // For Localnet topology only `Secondary` is allowed.
+    // Secondary network is only assigned to pods that use `k8s.v1.cni.cncf.io/networks` annotation to select given network.
+    // +required
+	// +kubebuilder:validation:Enum=Secondary


If NetworkRole already specifies an enum validation tag this won't work. To use an Enum tag this needs to be its own type alias to a string or a string. The only other path to restrict this further in this case would be a CEL expression.

using a CEL expression you would end up with a potentially misleading CRD schema though because Primary would show up as an allowed value in the schema but in reality it would be blocked by your XValidation

I think we can avoid it by moving the enum validation for NetworkRole definition to each topology config definition (where NetworkRole used), as follows:

Remove the enum validation exist on NetworkRole definition

On Layer2Config definition, add enum validation to role field allowing Primary or Secondary.

On Layer3Config definition, add enum validation to role field allowing Primary or Secondary.

Finally, in the proposed LocalnetConfig definition, add enum validation to role field allowing Secondary only.

@everettraven WDYT?

I think that sounds reasonable

Added text about required changes to topology role validations https://github.com/openshift/enhancements/compare/6fc5b82ece4e9bdaaedf6d7208f56681dc40bcdc..7caa6a8f94524aa3218f9ea2ef845bcc377e4893#diff-65ff4f11b53425ec17e6fbd228843c58470c309f5031c38c345010775bb378d9R508-R519

ormergi · 2025-02-20T15:06:47Z

Changes:

Fix excludeSubent validation
Add validations to physicalNetworkName, resolves SDN-5519: OVN-K: Localnet API support #1750 (comment)
Added text about required changes to topology role validations, resolves SDN-5519: OVN-K: Localnet API support #1750 (comment)

ormergi · 2025-02-20T15:08:49Z

@everettraven @maiqueb @npinaeva I think I resolved all comments, could you please take another look?

ormergi · 2025-02-20T15:33:38Z

Changes: fix indentations

maiqueb

Some comments, nothing big.

Real close to having the +1 again.

maiqueb · 2025-02-20T15:31:19Z

enhancements/network/ovn-kubernetes-localnet-api.md

+1. `name` 
+   The underlying network name.
+   - Should match the node OVS bridge-mapping network-name.
+   - In case Kubernetes-nmstate is used, should match the `NodeNetworkConfigurationPolicy` (NNCP) `spec.desiredState.ovn.bridge-mappings` value.


should actually match the localnet attribute of an item on that slice / array.

Added text to clarify that

maiqueb · 2025-02-20T15:31:49Z

enhancements/network/ovn-kubernetes-localnet-api.md

+2. `physicalNetworkName`
+   Points to the node OVS bridge-mapping network-name - the network-name mapped to the node OVS bridge that provides access to that network.
+   (Can be defined using Kubernetes-nmstate NNCP - `spec.desiredState.ovn.bridge-mappings`)
+   - Overrides the `name` attribute.


defined in .1 , for clarity.

maiqueb · 2025-02-20T15:36:57Z

enhancements/network/ovn-kubernetes-localnet-api.md

+As of today the CUDN CRD controller does not allow mutating `spec.network`.
+
+OVN-Kubernetes support NAD spec mutations on day 2.
+For the new the spec to take place, restarting the workloads is necessary.


nit:

Suggested change

For the new the spec to take place, restarting the workloads is necessary.

For the new network configuration (defined in the spec) to take place, the user must restart the workloads.

maiqueb · 2025-02-20T15:38:32Z

enhancements/network/ovn-kubernetes-localnet-api.md

+  network:
+    topology: Localnet
+    localnet:
+      name: "tenantblue"


I still prefer physicalNetworkName but, maybe just name is good enough.

maiqueb · 2025-02-20T15:40:06Z

enhancements/network/ovn-kubernetes-localnet-api.md

+##### Subnets
+The subnets and exclude-subnets should be in CIDR form, similar to Layer2 topology subnets.
+
+##### Persistent IPs


I miss explicitly indicating which is the default. Which I assume to be false.

Yes default is false. Added text to clarify that.

maiqueb · 2025-02-20T15:43:10Z

enhancements/network/ovn-kubernetes-localnet-api.md

+- `Role`:
+  - Required.
+  - Can be Secondary only when topology=Localnet.
+    - Having Role explicitly makes the API predictable and consistent with other topologies. In addition it enables extending localnet for Primary networks.


there's no use case for primary UDN in localnet. It's pointless.

... but I understand your point.

I changed the text to omit primary role

maiqueb · 2025-02-20T15:44:27Z

enhancements/network/ovn-kubernetes-localnet-api.md

+    localnet:
+      role: Secondary
+      physicalNetworkName: tenantblue
+      subnets: ["192.168.100.0/24", "2001:dbb::/64"]


for completeness, let's also exclude the GW IPs here (one for each subnet).

shouldn't this be a /32 ?

maiqueb · 2025-02-20T15:45:09Z

enhancements/network/ovn-kubernetes-localnet-api.md

+labels:
+  k8s.ovn.org/user-defined-network: ""


shouldn't this label be on the namespace ?...

The CRDs controller label NADs with this label making it easier to aggregate for maintenance by admins (view all UDN NAD, etc..).
The primary UDN label for namespace is the same.

OK, fair enough.

maiqueb · 2025-02-20T15:47:08Z

enhancements/network/ovn-kubernetes-localnet-api.md

+    // Allowed values are "Layer3", "Layer2" and "Localnet".
+    // Layer3 topology creates a layer 2 segment per node, each with a different subnet. Layer 3 routing is used to interconnect node subnets.
+    // Layer2 topology creates one logical switch shared by all nodes.
+    // Localnet topology attach to the overlay local network. Enables egress to the provider's physical network. 


I kind of always seen this the other way around; Localnet topology (which is an OVN term) attaches to the physical network .

I changed the text saying localnet attach to physical network

maiqueb · 2025-02-20T15:47:53Z

enhancements/network/ovn-kubernetes-localnet-api.md

+2. Add CEL rule validations to restrict mutations of:
+   `spec.network.topology`, `spec.network.layer2`, and `spec.network.layer3`.
+3. Add CEL rule validations to restrict mutation of localnet topology settings: 
+   `spec.network.localnet.role`, `spec.network.localnet.subnetes` and `spec.network.topology.ipam`.


Suggested change

`spec.network.localnet.role`, `spec.network.localnet.subnetes` and `spec.network.topology.ipam`.

`spec.network.localnet.role`, `spec.network.localnet.subnets` and `spec.network.topology.ipam`.

Thanks. Fixed.

Signed-off-by: Or Mergi <[email protected]>

ormergi · 2025-02-20T16:46:45Z

Changes: address #1750 (review)
@maiqueb PTAL 🙏

maiqueb · 2025-02-20T16:57:35Z

/lgtm

I just think you have a typo (using a /31 subnet instead of a /32)

openshift-ci · 2025-02-20T17:16:24Z

@ormergi: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2025

ormergi force-pushed the localnet-api-ext branch 7 times, most recently from f552b5c to 466d214 Compare February 5, 2025 13:37

ormergi marked this pull request as ready for review February 6, 2025 17:32

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2025

openshift-ci bot requested review from danwinship and dougbtv February 6, 2025 17:33

openshift-ci bot requested review from maiqueb and ricardomaraschini February 6, 2025 17:33

maiqueb suggested changes Feb 7, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

ormergi force-pushed the localnet-api-ext branch from 466d214 to 16775f8 Compare February 9, 2025 12:28

maiqueb reviewed Feb 10, 2025

View reviewed changes

npinaeva reviewed Feb 10, 2025

View reviewed changes

ormergi force-pushed the localnet-api-ext branch from 16775f8 to 03fbe10 Compare February 12, 2025 02:12

ormergi commented Feb 12, 2025

View reviewed changes

everettraven reviewed Feb 12, 2025

View reviewed changes

ormergi commented Feb 12, 2025

View reviewed changes

ormergi changed the title ~~OVN-K: Localnet API support~~ SDN-5634: OVN-K: Localnet API support Feb 13, 2025

ormergi force-pushed the localnet-api-ext branch from 03fbe10 to 1db8ce3 Compare February 18, 2025 14:25

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2025

ormergi force-pushed the localnet-api-ext branch 3 times, most recently from dca226c to c44363a Compare February 19, 2025 14:08

ormergi force-pushed the localnet-api-ext branch 2 times, most recently from 3bb9bc5 to 6fc5b82 Compare February 19, 2025 15:02

everettraven reviewed Feb 19, 2025

View reviewed changes

ormergi force-pushed the localnet-api-ext branch from 6fc5b82 to 7caa6a8 Compare February 20, 2025 15:03

ormergi force-pushed the localnet-api-ext branch from 7caa6a8 to b0ca553 Compare February 20, 2025 15:33

ormergi requested review from npinaeva, everettraven and maiqueb February 20, 2025 15:34

ormergi mentioned this pull request Feb 20, 2025

[WIP] crds,cudn: Support localnet topology ovn-kubernetes/ovn-kubernetes#5010

Draft

5 tasks

maiqueb reviewed Feb 20, 2025

View reviewed changes

OVN-K Localnet API support enhancement proposal

8236df8

Signed-off-by: Or Mergi <[email protected]>

ormergi force-pushed the localnet-api-ext branch from b0ca553 to 8236df8 Compare February 20, 2025 16:46

openshift-ci bot assigned maiqueb Feb 20, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2025

	bridge: br-ex <--------- OVN-K switch
	bridge: br-ex <--------- OVS switch

	The `spec.network.topology` serves as the union discriminator, it should accept `Locannet` option.
	The `spec.network.topology` serves as the union discriminator, it should accept `Localnet` option.

	// Role describes the network role in the pod.
	// role describes the network role in the pod.

	// MTU is the maximum transmission unit for a network.
	// mtu is the maximum transmission unit for a network.

		// +kubebuilder:validation:Minimum=576
		// +kubebuilder:validation:Maximum=65536

	For the new the spec to take place, restarting the workloads is necessary.
	For the new network configuration (defined in the spec) to take place, the user must restart the workloads.

	`spec.network.localnet.role`, `spec.network.localnet.subnetes` and `spec.network.topology.ipam`.
	`spec.network.localnet.role`, `spec.network.localnet.subnets` and `spec.network.topology.ipam`.

SDN-5519: OVN-K: Localnet API support #1750

Are you sure you want to change the base?

SDN-5519: OVN-K: Localnet API support #1750

Conversation

ormergi commented Feb 4, 2025 • edited Loading

openshift-ci bot commented Feb 4, 2025

ormergi commented Feb 6, 2025

maiqueb 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

This comment was marked as duplicate.

ormergi commented Feb 9, 2025

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb commented Feb 10, 2025

npinaeva 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

maiqueb Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

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

ormergi commented Feb 12, 2025

ormergi commented Feb 12, 2025

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

ormergi Feb 19, 2025 • edited Loading

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

ormergi commented Feb 4, 2025 •

edited

Loading

maiqueb Feb 10, 2025 •

edited

Loading

ormergi Feb 19, 2025 •

edited

Loading