Bug 1505266 - Validate node IP is local during sdn node initialization #17043
Conversation
openshift-ci-robot
added
the
size/L
openshift-merge-robot
added
the
approved
pravisankar
added
component/networking
sig/networking
and removed
approved
openshift-merge-robot
added
the
approved
|
@openshift/networking @danwinship PTAL |
pkg/network/node/node.go
Outdated
| @@ -144,36 +144,17 @@ func New(c *OsdnNodeConfig) (network.NodeInterface, error) { | |||
| return nil, nil | |||
| } | |||
|
|
|||
| if useConnTrack && c.ProxyMode != componentconfig.ProxyModeIPTables { | |||
There was a problem hiding this comment.
I feel like it's better to always log the "Initializing SDN" message... can you move things around so that happens before this? (And why did you move this before setNodeIP? Why does that matter?)
There was a problem hiding this comment.
Agree, it is always good to log this msg.
This proxy check doesn't depend on computing hostname/selfIP, so I just moved that logic a bit up in this method.
pkg/network/common/common.go
Outdated
|
|
||
| for _, addr := range addrs { | ||
| if addr.IP.String() == ip { | ||
| return link, addr.IPNet, nil |
There was a problem hiding this comment.
This is not the same as what findEgressLink() was doing. The netlink.Addr's embedded IPNet is an IP address (eg "172.17.0.3/24"), but eip.localEgressNet is supposed to be the subnet CIDR that includes it (eg "172.17.0.0/24"). Hence the re-parsing of it (because there's no public method to directly mask the IP address form into the subnet form).
Check proxy mode in node.New() before computing hostname/selfIP so that we can return early in case of incompatible proxy mode error.
pravisankar
force-pushed
the
validate-nodeip
branch
from
607e90b
to
75adfb4
Compare
|
@danwinship Updated, PTAL |
|
/retest |
|
/refresh |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, pravisankar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue. |
Automatic merge from submit-queue. Fix cross build #17043 broke the cross build by depending on netlink from pkg/network/common. This fixes that (by just moving the new function since it wasn't needed outside of pkg/network/node anyway).
Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1505266