Don't allow claiming node IP as egress IP #16779
Conversation
danwinship
added
component/networking
kind/bug
openshift-merge-robot
added
the
approved
openshift-ci-robot
added
the
size/S
|
/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 |
| @@ -245,6 +245,10 @@ func (eip *egressIPWatcher) deleteNamespaceEgress(vnid uint32) { | |||
| } | |||
|
|
|||
| func (eip *egressIPWatcher) claimEgressIP(egressIP, egressHex string) error { | |||
| if egressIP == eip.localIP { | |||
There was a problem hiding this comment.
That means if the master is not also configured as a node, the master IP will still be able to set as egressIPs?
There was a problem hiding this comment.
The code for claiming egress IPs is part of the node code; if the master isn't a node then it can't claim egress IPs at all.
There was a problem hiding this comment.
Sorry, I meant that as the bug described. I can add the master host IP as the egressIPs to node which will make the node to master connection broken.
The fix is going to check the node hosts IPs should not be added as egressIPs.
But for condition that the master only roles as master, but not node. Then the master host IP will still be able to add to nodes as egressIPs.
My env like:
host1: master
host2: node1
host3: node2
After the fix, the host1's IP will still be able to add to node1 or node2 since the host1 is not a node. But it will still break the node to master connection after that.
There was a problem hiding this comment.
Currently, we only validate EgressIP has valid IP (also IPv4 with #16807) but I think we should ensure it does not overlap with other cidrs (node/service/ingress/external...) and master IPs.
There was a problem hiding this comment.
@bmeng: we can't really validate that the IP address is totally unused anyway; there might be other machines on the same subnet that aren't nodes/master. egress-router has the same issue. But in both cases, only cluster admins can set those IPs anyway, so we can just tell them "be careful"
There was a problem hiding this comment.
@pravisankar the EgressIP has to be on the same subnet as the node's primary IP, which we have already verified doesn't overlap with the other CIDRs.
There was a problem hiding this comment.
@danwinship Where did we check 'EgressIP is on the same subnet as the node's primary IP'?
There was a problem hiding this comment.
ah, indeed, we don't; we add it to the primary interface, so it probably won't work right otherwise, but we don't actually check it. I'll fix that
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 16725, 16779, 16798, 16783, 16740). |
|
@danwinship: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
LGTM |
Automatic merge from submit-queue. Egress IP fixes 1. Further IP address validation, from #16779 (comment) 2. Fix OVS VXLAN ingress rule to not filter out remote node egress IP traffic, fixing https://bugzilla.redhat.com/show_bug.cgi?id=1501876
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1500203