feat: Add support for Linux VRFs #1744
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1744 +/- ##
==========================================
- Coverage 57.11% 55.69% -1.42%
==========================================
Files 618 379 -239
Lines 44654 27954 -16700
==========================================
- Hits 25502 15569 -9933
+ Misses 16192 10895 -5297
+ Partials 2960 1490 -1470
Flags with carried forward coverage won't be shown. Click here to find out more. |
| func (d *InterfaceDescriptor) enableL3MasterDev() error { | ||
| value, err := getSysctl(sysctlL3MDevVar) | ||
| if err != nil { | ||
| err = fmt.Errorf("could not read sysctl value for %s: %v", |
There was a problem hiding this comment.
It is better to use the format specifier with %w for error values, so they can be unwrapped by the caller(s) using Unwrap in the errors package.
quoted from fmt.Errorf docs:
If the format specifier includes a %w verb with an error operand, the returned error will implement an Unwrap method returning the operand. It is invalid to include more than one %w verb or to supply it with an operand that does not implement the error interface. The %w verb is otherwise a synonym for %v.
| @@ -68,6 +68,8 @@ type LinuxIfMetadata struct { | |||
| VPPTapName string // empty for VETHs | |||
| Namespace *linux_namespace.NetNamespace | |||
| HostIfName string | |||
| Vrf string | |||
| VrfDevRT int // only set for VRF_DEVICE | |||
There was a problem hiding this comment.
Agree that it should be and will change it.
I used int only because netlink package also uses it for the table ID, but negative values would not be valid, so...
| // VRF to which this interface should be assigned to. | ||
| // The field should reference the logical name of a VRF device (i.e. instance of this Interface model with | ||
| // VRF_DEVICE type). | ||
| string vrf = 10; |
There was a problem hiding this comment.
I'm not sure about the naming vrf here.. since we use namings with *_interface/*interfaces practically everywhere else where interface is referenced (route, arps, iptables..).
I think using vrf_interface is more consistent and possibly clearer to the users that this should be interface and not name/ID of VRF/routing table.
There was a problem hiding this comment.
What about vrf_master_interface or shorter variant vrf_master_if?
Just to make it more clear that what is being referenced is not some random interface inside a VRF, but the interface implementing VRF as a whole.
(master/slave terminology is not very popular these days but that's how they call it in Linux - ip link set dev eth1 master vrf-blue is said to enslave eth1 under vrf-blue interface/device)
There was a problem hiding this comment.
Yea, I think that's okay anything better than just vrf because it's interface and not routing table/VRF ID like in case of VPP VRFs.
Signed-off-by: Milan Lenco <milan.lenco@pantheon.tech>
|
PR updated after the review. |
| Table: routingTable, | ||
| } | ||
| if err := netlink.LinkAdd(link); err != nil { | ||
| return errors.Wrapf(err, "failed to add VRF device: LinkAdd (vrf=%s, rt=%d)", |
There was a problem hiding this comment.
The part with failed to add VRF device: should not be added here, because it is already added by the caller, so it is redundant and would make the error look like:
...: creating X interface failed: failed to add VRF device X (rt: Z): failed to add VRF device: LinkAdd (vrf=Y, rt=Z)
Usually, errors returned to caller should only add context info that caller does not know about.
In this case failed to **add VRF device** is not needed because caller already knows it called AddVRFDevice function. What it does not know that this function failed during call to LinkAdd so that part is adding useful context info to the error.
Signed-off-by: Milan Lenco <milan.lenco@pantheon.tech>
This PR contains these changes in proto:
VRF_DEVICEVrfMasterInterfaceto linux interfaceDocumentation for Linux VRF: