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

[brlbil]

I've initiated this discussion to explore incorporating driver-specific types within the library, with a particular interest in Netkit. However, the approach could extend to others such as Vxlan and Bond. Currently, driver-specific data is managed through LinkInfo.Data and/or LinkInfo.SlaveData as byte slices. To define concrete types for these drivers, we need a new mechanism. I propose several approaches and have prepared POC branches to illustrate these ideas, though I'm open to further suggestions for the most effective implementation. Your input would be invaluable to refine these proposals.

1. Interface Integration within LinkInfo: Adding a new interface within LinkInfo to handle driver types based on the Kind dynamically. This approach maintains backward compatibility. Example POC.

2. Revision of Data Fields as Interfaces: Transforming LinkInfo.Data and SlaveData into interfaces. This method would necessitate a major version update due to its backward-incompatible nature. Example POC.

3. Driver Types in LinkAttributes: Embedding driver types directly within LinkAttributes. This method could provide operational benefits, such as compatibility checks. For example, Netkit does not support setting ethernet address, in this way, we could check this condition Example POC.

[jsimonetti]

Thank you very much for bringing this up.

I have had earlier talks about something similar, however it never really got the attention it deserves.

I had a look at your POC's, and I must say I like optie 2 the most, even though that means a breaking change.

However, I would like to seperate all the drivers in a seperate 'driver' subpackage, to keep things a little cleaner.
At the same time, I would like it to be easy for people to inject/register their own custom drivers.

Thinking out loud, this boils down to something similar to:

type LinkDriver interface {
	// Encode the driver data into the netlink message attribute
	Encode(*netlink.AttributeEncoder, *LinkMessage) error 

	// Decode the driver data from the netlink message attribute
	Decode(*netlink.AttributeDecoder, *LinkMessage) error 

	// Return the driver kind as string, this will be matched with the LinkInfo.Kind to find a driver to decode the data
	Kind() string                           
}


// registeredDrivers is the global map of registered drivers
var registeredDrivers = make(map[string]Driver)

// getDriver returns the driver instance for the given kind
// it returns the default (LinkData) driver if the driver is not found
func getDriver(kind string) Driver {
	if d, ok := registeredDrivers[kind]; ok {
		return d
	}

	return &LinkData{}
}

// RegisterDriver registers a driver with the link service
// This allows the driver to be used to encode/decode the link data
func (l *LinkService) RegisterDriver(d Driver) error {
	if _, ok := registeredDrivers[d.Kind()]; ok {
		return fmt.Errorf("driver %s already registered", d.Kind())
	}
	registeredDrivers[d.Kind()] = d
}

All the regular drivers (such as netkit, vxlan, bonc, etc) can still be part of rtnetlink and could even be registered at Init() time, but it would still allow for people to develop their own drivers seperately, should they desire to.

By also passing the LinkMessage to the Driver, we can still make operational checks, as suggested in option 3.

Is this something that would suit your needs and would be willing to contribute?

[brlbil]

I am willing to contribute to this effort.

One thing is not very clear in your suggestion. In the proposed interface we are passing *LinkMessage as well.
But the driver's data is under LinkMessage.Attributes.Info so ideally we need to call the Encode and Decode function under the LinkAttribute's encode and decode functions where *LinkMessage is not available. We could call them up in the stack like MarshalBinary and UnmarshalBinary functions of *LinkMessage, but in this case, it would be very complicated to put the data in or read it from the right place in the byte slice. Or we might modify the *LinkAttribute encoding and decoding functions to pass *LinkMessage as well, since we do not care about the backward compatibility.

[jsimonetti]

I am in doubt here. I am unsure if a LinkDriver would need *LinkMessage.

perhaps just passing *LinkAttributes would be enough for a Driver. Do you have any strong feelings about this?

[brlbil]

I am unsure if any driver besides Netkit uses any LinkMesaage fields. Netkit driver required LinkMessage's Flags and Change fields to be the same for the peer interface, we cannot check this if we did not have *LinkMessage at least in Encoding func. There might be other drivers in the future that also require access to LinkMessage fields.

Another thing. Since we are considering a major library update, is there any other future we might want to implement? I was thinking about NetNamespace.

[jsimonetti]

So, I am thinking about changing the decode and encode function signatures.
However, the problem is, that it is possible not everything is unmarshalled by the time the Driver kicks in. The same would apply for the marshalling.

To make the Driver flexible enough I think we should do this:

type LinkDriver interface {
	// Encode the driver data into the netlink message attribute
	Encode(*netlink.AttributeEncoder) error 

	// Decode the driver data from the netlink message attribute
	Decode(*netlink.AttributeDecoder) error 

	// Return the driver kind as string, this will be matched with the LinkInfo.Kind to find a driver to decode the data
	Kind() string

	// After will perform any final checks. It will be called at the very end, after all unmarshalling of a LinkMessage has happened.
	After(*LinkMessage)

	// Before will perform any pre-check on the LinkMessage and LinkAttributes. It will be called before Marshalling a LinkMessage.
	Before(*LinkMessage)
}

func (m *LinkMessage) MarshalBinary() ([]byte, error) {
	if _, ok := m.Attributes.(LinkDriver); ok {
		m.Attributes.Before(m)
	}

	b := make([]byte, unix.SizeofIfInfomsg)
	...
}

func (m *LinkMessage) UnmarshalBinary(b []byte) error {
	...

	if _, ok := m.Attributes.(LinkDriver); ok {
		m.Attributes.After(m)
	}

	return nil
}

About Namespaces, I believe this library already works correctly, if you dial a rtnetlink.Conn into a specific namespace by setting the appropriate config option:

rtnetlink.Dial(&netlink.Config{NetNS: 1234})

Though, to be fair, I have never actually tried if this works or not.

[brlbil]

I have been looking into known drivers and have not seen a need for an After function to check LinkMessage related things. I have a picture to start implementing. I can push a draft PR and we can continue the discussion there if needed.

One thing seems problematic though. Creating a separate driver package would cause an import cycle. The driver package needs to import the rtnetlink package to use the LinkMessage struct and the rtnetlink needs to import the driver package to register all implemented packages.

[lmb]

Drive by comment: there could be a public rtnetlink.RegisterDriver which is invoked explicitly by the user or from an init function in the driver package.

[brlbil]

This would work. Thanks @lmb

[jsimonetti]

I have a picture to start implementing. I can push a draft PR and we can continue the discussion there if needed.

I am looking forward to it.