network: Handle default route where gateway is empty#404
network: Handle default route where gateway is empty#404bergwolf merged 3 commits intokata-containers:masterfrom
Conversation
|
/test |
|
It's a simple enough fix but I still think this could do with a unit test. Since Travis is also failing... .. how about doing something like: func gatewayFromRoute(route *pb.Route) string {
// ...
} |
| // We do not modify the gateway in the list of routes provided, | ||
| // since we loop through the routes checking for the gateway again. | ||
| gateway := route.Gateway | ||
| if route.Dest == "" && route.Gateway == "" && route.Source == "" { |
There was a problem hiding this comment.
@amshinde
This looks weird to handle a case where everything is empty with a default behavior of having a gateway 0.0.0.0.
Why don't we try to solve this by pushing a patch to the ipvlan plugin directly? IMO the plugin should send set the gateway properly instead of being empty. Maybe I'm missing some context though.
There was a problem hiding this comment.
@sboeuf When the gateway is empty the implicit value for default gateway is 0.0.0.0. We get the empty gateway while scanning the network. Unfortunately we need to provide an explicit value of "0.0.0.0" to netlink. This is what iproute2 does as well when you run the command ip route add default dev eth0 as well. This is equivalent to calling ip route add default via 0.0.0.0 dev eth0 scope link
There was a problem hiding this comment.
@sboeuf this has to be done primarily because the netlink library has an explicit check
https://github.com/vishvananda/netlink/blob/1404979ff6478eb5af45f1c77fed8e8f249dfb3f/route_linux.go#L488
But given that
ip route add default dev enp0s3 scope link == ip route add default via 0.0.0.0 dev enp0s3 scope link and that satisfies the check in the go netlink library this fix will work.
However looking at the iproute2 code https://github.com/shemminger/iproute2/blob/master/ip/iproute.c#L699
Here the gw is not filled explicitly means that the kernel may accept the gateway not being present.
i.e. https://github.com/shemminger/iproute2/blob/master/ip/iproute.c#L750
The RTA_GATEWAY attribute does not need to be present.
If anything longer term we can push a fix to the upstream netlink library to allow leaving out RTA_GATEWAY
There was a problem hiding this comment.
Thanks for the feedback here!
As discussed with @amshinde offline, let's move forward with this PR for now, and submit some parallel code to the upstream netlink library.
Once the netlink code gets merged, let's revendor it, and get rid of this patch.
In case of ipvlan plugin, the default route has an empty gateway. Set this to default gateway value before adding this route with netlink. Fixes kata-containers#403 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
Introduce function called processRoute to reduce the cyclomatic complexity of updateRoute. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
|
/retest |
Test adds routes expected to be added while using ipvlan plugin in l3 mode. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
|
/retest |
Codecov Report
@@ Coverage Diff @@
## master #404 +/- ##
==========================================
+ Coverage 47.96% 48.04% +0.08%
==========================================
Files 17 17
Lines 2648 2656 +8
==========================================
+ Hits 1270 1276 +6
- Misses 1216 1217 +1
- Partials 162 163 +1 |
| // We do not modify the gateway in the list of routes provided, | ||
| // since we loop through the routes checking for the gateway again. | ||
| gateway := route.Gateway | ||
| if route.Dest == "" && route.Gateway == "" && route.Source == "" { |
In case of ipvlan plugin, the default route has an empty gateway.
Set this to default gateway value before adding this route with
netlink.
Fixes #403
Signed-off-by: Archana Shinde archana.m.shinde@intel.com