-
Notifications
You must be signed in to change notification settings - Fork 770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CHECK support for linux plugins #264
Conversation
@mccv1r0 I mentioned this in a private email, but we'll need to split out the Linux CHECK code that depends on netlink into _linux.go files with "//+build linux" at the top, and a stubbed out implementation in a "_windows.go" file with "//+build windows". That should fix the windows build errors. EDIT: acutally nevermind for now, I don't think we even need to worry about it for host-local. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial thoughts
pkg/utils/utils.go
Outdated
return fmt.Errorf("Invalid IP Version %v for interface %v", ips.Version, ifName) | ||
} | ||
|
||
gwy, err := netlink.RouteListFiltered(family, findGwy, routeFilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know for sure that all configurations will have a gateway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested all main plugins (except windows) and all had a gateway. But this might be a result of Issue 273? I'm pushing v2 with other comments addressed. We can come back to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in #273 I asked if how/if it is possbile to add such a route to the bridge plugin.
ip route add 192.168.10.153/32 dev eth0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all the stylistic things I complained about are fixed.
There are still some open comments from Dan, some commented-out code, etc., so not clicking the "approve" button yet, but it is looking good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after some small clean-up where there are comments.
Good job!
…evice main plugins host-local and static ipam plugins tuning, bandwidth and portmap meta plugins Utility functions created for common PrevResult checking Fix windows build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm now
Add check support for:
bridge, ipvlan, macvlan, p2p, vlan and host-device main plugins
host-local and static ipam plugins
tuning, bandwidth and portmap meta plugins
Utility functions created for common PrevResult checking
Fixes #263