user-defined-network-segmentation: add VRF field to CUDN #1730
Conversation
jcaamano
commented
Dec 19, 2024
•
jcaamano
commented
|
/assign @ormergi |
jcaamano
force-pushed
the
vrf-name
branch
2 times, most recently
from
764a35d to
72c9780
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: trozet 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| // 16 characters long and unique. If omitted, the ClusterUserDefinedNetwork name | ||
| // will be used as long as it is less than 16 characters long. Otherwise an internal | ||
| // name will be generated. | ||
| // +optional |
There was a problem hiding this comment.
If omitted, the ClusterUserDefinedNetwork name
// will be used as long as it is less than 16 characters long. Otherwise an internal
// name will be generated.
Why does the VRF name has to be configured implicitly if not specified in the spec?
Can we avoid forcing VRF name for CUDNs who doesnt specify it?
Please consider simplifying the name generation as much possible, it will make so it easier to work with and more predictable.
I would drop using the CUDN metadata.name, leaving two options: the spesifed name in the spec or generated one.
In case the VRF name is generated, I am not sure how it can be consumed because its not reported anywhere.
One would need to fetch the corresponding NAD and parse the spec to get the generated VRF name.
There was a problem hiding this comment.
Why does the VRF name has to be configured implicitly if not specified in the spec? Can we avoid forcing VRF name for CUDNs who doesnt specify it?
Please consider simplifying the name generation as much possible, it will make so it easier to work with and more predictable. I would drop using the CUDN metadata.name, leaving two options: the spesifed name in the spec or generated one.
Note that currently we are using a generated VRF name as mp<network-id>-udn-vrf which is not predictable and which is precisely what this proposal is trying to address.
Using metadata.name is what we should be doing, except for the fact that is not limited to 15 characters. Because of this, we offer the proposed optional extra field.
Generating a name is a fallback we do need just because of that same reason: metadata.name is not limited to 15 characters.
The only simplification we can do here is making the field required. But that is not backward compatible and will potentially lead to just replicating the metadata.name value in most cases.
| @@ -421,6 +421,7 @@ The cluster scoped CRD should have the following additional field: | |||
|
|
|||
| | Field name | Description | optional | | |||
| |-------------------|---------------------------------------------------------------------------------------------------------------|----------| | |||
| | VRF | A custom VRF device name to use | Yes | | |||
There was a problem hiding this comment.
Could you please elaborate/add user-stories?
| // VRF is an optional VRF device name to use for the network. It has to be less than | ||
| // 16 characters long and unique. If omitted, the ClusterUserDefinedNetwork name |
There was a problem hiding this comment.
// VRF is an optional VRF device name to use for the network. It has to be less than
// 16 characters long and unique.
Asking to understand, what will happen if VRF is specified?
Does it mean the pod is wired to some device in the node?
If so, does it fit localnet topology?
There was a problem hiding this comment.
No, it just means that name will be used instead of the currently generated one.
| @@ -658,6 +669,7 @@ kind: ClusterUserDefinedNetwork | |||
| metadata: | |||
| name: db-network | |||
| spec: | |||
| vrf: db-network | |||
There was a problem hiding this comment.
In case VRF field is introduced to provider some integration with other APIs, I suggest having dedicated example for it, including the other APIs it should be integrated with.
There was a problem hiding this comment.
I don't want to introduce other APIs that then I will have to explain. But I will come up with a user story.
| @@ -836,6 +848,11 @@ Having the prefix avoids conflicting with existing NADs who has the same `metada | |||
| For example: | |||
| Given the CR meta.name is `db-network`,the NAD metadata.name will be `cluster.udn.db-network`. | |||
|
|
|||
| If a VRF name is provided in the cluster-scope CRD instance, it will be set to an homonimous field in the NAD NetConf. If the | |||
| provided VRF name is already used in any other cluster-scope CRD instance, a failed status should be reported for the cluster-scope | |||
There was a problem hiding this comment.
Sounds like it"ll require additional caching, from what I know its something that should be avoided.
Could it be validated at the API level? using some CEL rules?
| @@ -836,6 +848,11 @@ Having the prefix avoids conflicting with existing NADs who has the same `metada | |||
| For example: | |||
| Given the CR meta.name is `db-network`,the NAD metadata.name will be `cluster.udn.db-network`. | |||
|
|
|||
| If a VRF name is provided in the cluster-scope CRD instance, it will be set to an homonimous field in the NAD NetConf. If the | |||
There was a problem hiding this comment.
The VRF field eventually translated to some ID and assigned to corresponding field in the NetConf (NAD spec.config) that have less restrictions on length.
What does the generated ID (e.g.: udn-mynet-vrf) represent? which entity consumes it?
Can the NetConf.Name be used instead?
It has some similar characteristics.
There was a problem hiding this comment.
The meaning and use of the VRF is not introduced in this proposal amendment, it was already introduced and explained on the original enhancement.
The generation of VRF names was assumed as an implementation detail and not specified in the enhancement.
This amendment just introduces and optional user provided name for it and surfaces the name generation that was already assumed.
Currently VRF names are generated in a way that can not be easily identified and correlated to the UDNs. Cluster admins may want predictable VFR names: * VFR-Lite day 0 configuration * Friendlier VRF matching of non auto non default target VRFs on route advertisements * Consistent VRF name configuration around the fabric * Same ease of use and level of customization as the default VRF Provide that through an optional field of the CUDN. If not specified, use the CUDN name if possible. Otherwise, use the internal genrated names we have been using up until now. Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
|
Added user story and clarified PR/commit message |
|
@jcaamano: 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. |
