-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
lib: fix binding to a vrf #8644
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -994,25 +994,50 @@ const char *vrf_get_default_name(void) | |
return vrf_default_name; | ||
} | ||
|
||
int vrf_bind(vrf_id_t vrf_id, int fd, const char *name) | ||
int vrf_bind(vrf_id_t vrf_id, int fd, const char *ifname) | ||
{ | ||
int ret = 0; | ||
struct interface *ifp; | ||
struct vrf *vrf; | ||
|
||
if (fd < 0) | ||
return -1; | ||
|
||
if (vrf_id == VRF_UNKNOWN) | ||
return -1; | ||
|
||
/* can't bind to a VRF that doesn't exist */ | ||
vrf = vrf_lookup_by_id(vrf_id); | ||
if (!vrf_is_enabled(vrf)) | ||
return -1; | ||
pguibert6WIND marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (ifname && strcmp(ifname, vrf->name)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have an issue #7432 Test Steps: Configure eBGP session between Sonic-1 and Sonic2 in default vrf using Peer-group. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I tested that the above scenario works with your fixes in vrf.c and vrf.h. I will defer my pull request (#7432) in favor of this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 1014: From netdevice creation point of view, how is the VRF interface different from a physical interface ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I get this question and its relevance to this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this code, when ifname is interface (say Ethernet64), we are processing it differently (by calling if_lookup_by_name() and checking inside if_name_head database). But if ifname is vrf (e.g. Vrf-Red), we will not check inside the database. I was wondering why we have this difference in code. From zebra perspective, both look same as below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done, because the code flow in the daemons is the following:
If we try to look for an interface on the second step, we always fail, because we didn't receive the interface registration message yet. It's enough to check for the VRF existence if the ifname is the VRF name. |
||
/* binding to a regular interface */ | ||
|
||
/* can't bind to an interface that doesn't exist */ | ||
ifp = if_lookup_by_name(ifname, vrf_id); | ||
if (!ifp) | ||
return -1; | ||
pguibert6WIND marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
/* binding to a VRF device */ | ||
|
||
/* nothing to do for netns */ | ||
if (vrf_is_backend_netns()) | ||
return 0; | ||
|
||
/* nothing to do for default vrf */ | ||
if (vrf_id == VRF_DEFAULT) | ||
return 0; | ||
|
||
ifname = vrf->name; | ||
} | ||
|
||
if (fd < 0 || name == NULL) | ||
return fd; | ||
/* the device should exist | ||
* otherwise we should return | ||
* case ifname = vrf in netns mode => return | ||
*/ | ||
ifp = if_lookup_by_name(name, vrf_id); | ||
if (!ifp) | ||
return fd; | ||
#ifdef SO_BINDTODEVICE | ||
ret = setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, name, strlen(name)+1); | ||
ret = setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, ifname, | ||
strlen(ifname) + 1); | ||
if (ret < 0) | ||
zlog_debug("bind to interface %s failed, errno=%d", name, | ||
errno); | ||
zlog_err("bind to interface %s failed, errno=%d", ifname, | ||
errno); | ||
#endif /* SO_BINDTODEVICE */ | ||
return ret; | ||
} | ||
|
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.
Suppose at this step, vrf is not enabled. Meaning zebra has not yet processed this message and has not created this netdevice. So the code returns from here without binding the socket-id to the VRF (or interface).
Later, zebra creates the netdevice. Now, after creation of netdevice, will this socket-id be rebinded to the VRF (or interface) ? (i.e. will this code get called again ? )
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 actually up to the daemon to correctly process the VRF disable/enable events and recreate its sockets when the VRF is enabled.
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 agree with @idryzhov