Skip to content
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

Add LinkDriver interface and Driver package #221

Merged
merged 5 commits into from
May 10, 2024

Conversation

brlbil
Copy link
Contributor

@brlbil brlbil commented May 2, 2024

This PR adds a LinkDriver interface that replaces the LinkInfo.Data and SlaveData fields and
adds Driver package with link drivers Bond, Netkit, and Veth that implement the LinkDriver interface.

Only these three drivers are implemented to demonstrate different kinds of link drivers and not complicate the PR.

Please take a look at the commit messages for individual changes.

Fixes: #216

@brlbil brlbil marked this pull request as draft May 2, 2024 08:16
@brlbil brlbil marked this pull request as ready for review May 2, 2024 08:16
@brlbil brlbil force-pushed the pr/brlbil/add_driver_pkg branch 3 times, most recently from 9a147fe to 9eefed8 Compare May 3, 2024 21:06
@jsimonetti
Copy link
Owner

First off, thank you. This is very nice!

I have a few nits which I will comment on while I review. (Please allow me a day or two).

One thing i would like to discuss is rtnetlink.RegisterDriver.

First off, this function is not threadsafe. So it should have a very clear comment and documentation that it is very unwise to use after you Dial. I prefer it this way, instead of adding mutexes to protect it, since that would add considerable amount of locking.

Second, I think we should make a choice for the driver package. Currently it registers the drivers on init(), which I believe is fine, since it is what most people want. However, that does block consumer from registering their own driver, if it conflicts with drivers registered already.
So, either we move the driver registration away from init() to an explicit driver.LoadAll() function, or we leave it like this and add a rtnetlink.UnregisterDriver. The latter has my preference, and we could even hold off on that untill there is actual demand for such a function.

What do you think?

@brlbil
Copy link
Contributor Author

brlbil commented May 6, 2024

Thanks for pointing these out.

Since rtnetlink.RegisterDriver relies on global variables, a global mutex would be needed for thread safety, which isn't ideal. Let's put a comment on this.

Initializing drivers with the second option makes more sense. We'll keep it this way and add rtnetlink.UnregisterDriver if necessary. Given the limited number of drivers, implementing most (or the most useful) should suffice, reducing the need for competing implementations. I am willing to work on adding other drivers later.

Another thing is that I have observed that the iproute2 tool does not send any attribute that is not initialized. I added the name behind a check because it was causing problems. It seems we could do the same for the values below. But I did not want to overcomplicate the PR. Am I missing anything here?

ae.Uint16(unix.IFLA_UNSPEC, 0)
if a.Name != "" {
	ae.String(unix.IFLA_IFNAME, a.Name)
}
ae.Uint32(unix.IFLA_LINK, a.Type)
ae.String(unix.IFLA_QDISC, a.QueueDisc)

I will make all of the changes after your review.

link.go Outdated Show resolved Hide resolved
link.go Outdated Show resolved Hide resolved
link.go Outdated Show resolved Hide resolved
link.go Outdated Show resolved Hide resolved
link.go Outdated Show resolved Hide resolved
link.go Outdated Show resolved Hide resolved
link.go Outdated Show resolved Hide resolved
driver/driver.go Show resolved Hide resolved
driver/driver.go Show resolved Hide resolved
driver/driver.go Outdated Show resolved Hide resolved
@jsimonetti
Copy link
Owner

I think I might have made your process a bit complicated by rebasing using github by accident. My appologies.

@jsimonetti
Copy link
Owner

Another thing is that I have observed that the iproute2 tool does not send any attribute that is not initialized. I added the name behind a check because it was causing problems. It seems we could do the same for the values below. But I did not want to overcomplicate the PR. Am I missing anything here?

ae.Uint16(unix.IFLA_UNSPEC, 0)
if a.Name != "" {
	ae.String(unix.IFLA_IFNAME, a.Name)
}
ae.Uint32(unix.IFLA_LINK, a.Type)
ae.String(unix.IFLA_QDISC, a.QueueDisc)

I believe you are correct. I have not had the need to address this though. I am fine with adding this to the PR

brlbil added 3 commits May 9, 2024 06:56
This commit removes unix.IFLA_UNSPEC and introduces checks for the interface name,
the link type and the queue disc fields.
Interface name validation is necessary to prevent an 'invalid argument' error
when creating a link with an empty name. Other checks were added to be consistent with the ip tools.

Signed-off-by: Birol Bilgin <birolbilgin@gmail.com>
This commit introduces the NetNS struct and integrates network namespace capabilities into link attributes.
This enhancement facilitates the creation of links, such as veth pairs, across different network namespaces
without the need to execute directly within those namespaces.

Signed-off-by: Birol Bilgin <birolbilgin@gmail.com>
This commit introduces a Driver interface and changes Data and SlaveData fields within the LinkInfo struct
as LinkDriver to accommodate driver-specific data encoding, addressing the limitation
where LinkInfo.Data and SlaveData fields were merely byte slices without support for specific data encoding.

Drivers are registered globally with the RegisterDriver function.
For un-registered drivers, the default LinkData driver is used.

Signed-off-by: Birol Bilgin <birolbilgin@gmail.com>
@brlbil brlbil force-pushed the pr/brlbil/add_driver_pkg branch from 4ea0bf5 to ea66878 Compare May 9, 2024 11:38
This commit introduces the 'driver' package, which contains specific implementations of the LinkDriver interface.
It also includes the implementation of the Linux bond driver as LinkDriver and the bond slave driver as LinkSlaveDriver.

Signed-off-by: Birol Bilgin <birolbilgin@gmail.com>
@brlbil brlbil force-pushed the pr/brlbil/add_driver_pkg branch from ea66878 to ede371c Compare May 9, 2024 11:44
@brlbil
Copy link
Contributor Author

brlbil commented May 9, 2024

While adding documentation to the drivers, I found a couple of issues and fixed them.

Copy link
Owner

@jsimonetti jsimonetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last nit I found. The rest is looking awesome!

driver/veth.go Outdated Show resolved Hide resolved
This commit adds Netkit and Veth drivers.

Signed-off-by: Birol Bilgin <birolbilgin@gmail.com>
@brlbil brlbil force-pushed the pr/brlbil/add_driver_pkg branch from ede371c to 22f75eb Compare May 9, 2024 18:09
@jsimonetti jsimonetti merged commit bd79d59 into jsimonetti:master May 10, 2024
8 checks passed
@jsimonetti
Copy link
Owner

I am willing to work on adding other drivers later.
I am very much looking forward to more drivers!

@brlbil Thank you once again for this contribution!
I have cut a new release containing these changes.

@brlbil brlbil deleted the pr/brlbil/add_driver_pkg branch May 10, 2024 07:53
@jsimonetti
Copy link
Owner

@brlbil I am keen to add some more drivers. Could you tell me how you found out what the layout of the linkinfo should be?
Perhaps point me at some docs you used, so I can look into other link types?

@brlbil
Copy link
Contributor Author

brlbil commented May 27, 2024

@jsimonetti It is good to hear that. I want to work on it but I do not have much spare time these days. I have looked at other Golang Netlink library that implements drivers. It helps but there are some differences because it is designed differently. I have looked at the iproute2 tool but later I learned that it should not be taken as a reference implementation, the net namespace implementation that later changes. Lastly, I have looked at the kernel source https://elixir.bootlin.com/linux/latest/source/drivers/net and kernel patch messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding driver specific Netkit type, and possibly others like Vxlan, Bond etc...
2 participants