bridge: add isDefaultGateway field #50
Conversation
| for i, route := range result.IP4.Routes { | ||
| if defaultNet.String() == route.Dst.String() { | ||
| if route.GW != nil && !route.GW.Equal(result.IP4.Gateway) { | ||
| return errors.New(fmt.Sprintf( |
|
I think it's a reasonable solution but I need to sleep on this. There are some implications for integrating it with |
|
I'd like to point out again that the default behaviour hasn't been changed, it does require a configuration change to make this happen. Only if |
|
@steveej I guess you're right. However there's also fact that this is almost sugar on top of IPAM routes. You could do the same with |
|
I see your point. I followed the already existing So, I would like a solution of that moves the gateway attributes to IPAM and allows for better flexibility. |
|
The first address is only assumed if IPAM did not provide a gateway address (https://github.com/appc/cni/blob/master/plugins/main/bridge/bridge.go#L195). The "gateway" field in the IPAM result JSON is optional, so there is a problem that if IPAM returns some routes but not the gateway, it can get messed up. Maybe we can issue a warning or something in this case. |
|
@eyakubovich: my understanding on this is that we'll move the gateway attributes to IPAM and leave the behavior as is, but add a convenience option to set the default GW as already implemented in this PR. Please confirm and I'll update the implementation accordingly. |
steveej
added
reviewed/needs rework
kind/feature request
missing/tests
needs review
and removed
kind/question
needs rebase
reviewed/needs rework
labels
| @@ -196,6 +201,7 @@ func cmdAdd(args *skel.CmdArgs) error { | |||
| return err | |||
| } | |||
|
|
|||
| // TODO: make this optional when IPv6 is supported | |||
There was a problem hiding this comment.
After inclusion of IPv6 next line should be extended with && result.IP6 == nil so this comment is hmmm... not precise.
edit: and then error in next line should be similarly extended.
There was a problem hiding this comment.
It's a hint that there's going to be multiple options, logically IPv4 || IPv6
|
LGTM |
|
LGTM. As discussed on IRC, it's worth waiting for #211 and rebasing, for the tests, before merging. |
When isDefaultGateway is true it automatically sets isGateway to true. The default route will be added via the (bridge's) gateway IP. If a default gateway has been configured via IPAM in the same configuration file, the plugin will error out.
Use Go modules instead of vndr
When isDefaultGateway is true it automatically sets isGateway to true.
The default route will be added via the (bridge's) gateway IP.
If a default gateway has been configured via IPAM in the same configuration file, the plugin will error out.
Fixes #27.
Fixes #184.