Add multi IP support for SBR #623
Conversation
Move default table routes which match the ipCfg config
This allows SBR plugin to accommodate for multi-ip interfaces
Fixes containernetworking#581
Signed-off-by: Anurag Dwivedi <[email protected]>
|
hey @plwhite can you give this a look-over? Thanks! |
|
Sorry, I've been out of office for a couple of days, but will get to this in the next day or so. |
|
You need to fix gofmt, FYI. |
plugins/meta/sbr/sbr_linux_test.go
Outdated
| Table: 101, | ||
| LinkIndex: expNet1.Routes[i].LinkIndex}) | ||
| } else { | ||
| // All 192.168.1.x routes expected in tabele 100 |
There was a problem hiding this comment.
Trivial typo: "tabele" should be "table"
plugins/meta/sbr/sbr_linux_test.go
Outdated
| Table: 101, | ||
| LinkIndex: expNet1.Routes[0].LinkIndex}) | ||
|
|
||
| // 2 Rules will ve created for each IP address. (100, 101) |
There was a problem hiding this comment.
Trivial typo: "ve" should be "be"
|
|
||
| // Use a different table for each ipCfg | ||
| table++ |
There was a problem hiding this comment.
In the other places where a table ID is generated there are a few lines of code to make sure that the table isn't already in use. I think you strictly ought to do that here - i.e. take the little bit of code around firstTableID and turn it into a simple subroutine so it does the same checks in both places.
This is unlikely to go wrong, but if it does it would be horrible to have to figure out.
|
Sorry for the delay - generally looks good to me. I've added a few notes, mostly about comments but one minor code issue. I think this merits updating the docs at https://www.cni.dev/plugins/current/meta/sbr/ - just a couple of lines explaining what the expected behaviour for routes and rules is with multiple IP addresses on an interface. |
Check tableID not in use for every ipCfg
This allows SBR plugin to accommodate for multi-ip interfaces
Fixes containernetworking#581
Signed-off-by: Anurag Dwivedi <[email protected]>
|
Hi, @squeed and @plwhite , I have updated the review. Please review at your convenience. Post confirmation I can add a few lines in the doc about multi-ip scenario (The behavior remains consistent with the current definition, just a footnote about separating the routes meant for different IPs on a single interface into different routing tables as well. ) |
|
LGTM - thanks for the updates. |
|
/lgtm (assuming CI passes) |
|
Thanks! |
|
@anuragensemble @plwhite @squeed, seems like an issue with this check in. I have a Pod with 2 interfaces. [root@c-2-server-01 /]# ip -6 addr Each of the IPs here has a default gateway, rightly eth0 is added to the default gateway [root@c-2-server-01 /]# ip -6 route Each IP subnet for interface eth1, has a default gateway and rightly there are two additional routing tables, that is what your change is fixing. [root@c-2-server-01 /]# ip -6 rule [root@c-2-server-01 /]# ip -6 route show table 100 [root@c-2-server-01 /]# ip -6 route show table 101 The issue here is that for table 100 where the source lookup is fd74:ca9b:3a09:8660:c0:a8:1:70, it should not have a route entry for fd74:ca9b:3a09:868e::/64 subnet, same is the case with table 101. I think traffic will still flow fine, but it is not correct that the directly connected networks are spanning both tables. I stand corrected if this is expected behavior since the interface does have the subnet. Regards |
|
@svallala , Can you please share the network-attachment-definition spec for the above case. In the recent update we are performing subnet matching on the routes added by the previous plugin (+ by interface creation) and moving them into their respective routing tables. Requesting the full NAD spec to make a more informed comment about the validity of the logic. |
|
@anuragensemble, please find the NAD. If you are looking for the IP addresses allocated, you might not get that information. Sending you the IPAM response that the plugin would have received. apiVersion: k8s.cni.cncf.io/v1
IPAM Response{"cniVersion": "0.3.1", "dns": {}, "ips": [{"version": "6", "address": "fd74:ca9b:3a09:8687:00c0:00a8:0001:0089/64", "gateway": "fd74:ca9b:3a09:8687:00c0:00a8:0001:0001"}, {"version": "6", "address": "fd74:ca9b:3a09:868e:00c0:00a8:000b:009c/64", "gateway": "fd74:ca9b:3a09:868e::0001"}]} |
|
@anuragensemble I don't see the subnet check in the code before adding the routes. Can you point me to the snippet where the subnet check is performed. I still see following behavior with the latest code 4: eth0@if15806: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP [root@sbr-1-9899bcc95-bhb9j pktgen]# ip -6 route [root@sbr-1-9899bcc95-bhb9j pktgen]# ip -6 route show table 100 [root@sbr-1-9899bcc95-bhb9j pktgen]# ip -6 route show table 101 As you can see the last two tables have a route entry that should not be present. |
[sbr]: Use different tableID for every ipCfg
Move default table routes which match the ipCfg config
This allows SBR plugin to accommodate for multi-ip interfaces
Fixes #581
Signed-off-by: Anurag Dwivedi [email protected]