-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
If vrf name #8610
If vrf name #8610
Conversation
Instead of relying on the ifp->vrf_id, pass in the vrf_name for lookup. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Use the vrf name for if_create_name instead of the vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
On creation of ifindex's use the vrf name instead of the vrf index. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments. Overall the idea seems to solve the problem of creating the interface in non-existing VRF but it adds a possibility of a memory leak.
Example for OSPF:
- create ospf router in non-existing vrf
- create virtual link in this ospf router
After that, the VRF is automatically created and never deleted even if the ospf config is deleted.
To correctly solve this issue we should actually allow creating the interface without the underlying VRF and associating the interface with a VRF after its instantiation.
vrf = vrf_lookup_by_name(vrf_name); | ||
assert(vrf); | ||
|
||
ifp = if_new(vrf_id); | ||
ifp = if_new(vrf->vrf_id != VRF_UNKNOWN ? vrf->vrf_id : VRF_DEFAULT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct.
- You can't count on the existence of the VRF here. It may not exist yet.
- Why do you associate the interface with the default VRF when the index is VRF_UNKNOWN? The VRF name is known and we should associate the interface with this VRF.
To summarize the above, I think we should do the following:
- pass both
vrf_id
(may be unknown) andvrf_name
(may be null) to this function, - pass both
vrf_id
andvrf_name
toif_new
, - inside the
if_new
store bothvrf_id
andvrf_name
in the interface structure for later use.
This allows calling this function from places where we know only one of the two arguments.
|
||
hook_call(if_add, ifp); | ||
return ifp; | ||
} | ||
|
||
struct interface *if_create_ifindex(ifindex_t ifindex, vrf_id_t vrf_id) | ||
struct interface *if_create_ifindex(ifindex_t ifindex, const char *vrf_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change this function. It is called only from the code where we know the real vrf_id
.
vrf = vrf_get(ifp->vrf_id, NULL); | ||
vrf = vrf_lookup_by_name(vrf_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must call vrf_get
here to always create the VRF if it doesn't exist.
Following my previous suggestion of saving both vrf_id
and vrf_name
in the interface structure, this should be changed to:
vrf = vrf_get(ifp->vrf_id, ifp->vrf_name);
One of the arguments may be NULL, this is already handled in vrf_get
.
The same change should be done in if_set_index
.
The ability to add interfaces to not-yet-known VRFs was added in f60a118. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
convert code to use the vrf name instead of the vrf id for interface creation. There are cases where we have vrf_id of unknown due to pre-creation of vrfs. Start allowing us to associate these interfaces with the vrfs when we know the name but not the id yet.