Correct egress IP assignment for egress node IPs #1668

alexanderConstantinescu · 2020-09-07T21:29:04Z

When writing e2e tests for Egress IP (upcoming PR soon), I realized a mistake made on my part in the assignment procedure, namely: when specifying an egress IP which is a cluster node X's IP address, we might assign it to another node Y in the assignment procedure. This patch thus fixes this by implementing a check and making sure that if the egress IP specified is a cluster node's, that it is also assigned to exactly that node. If that node does not have the egress label, then we do not perform an assignment and notify the user by triggering an event.

Some of the tests in egressip_test.go were explicitly wrong and testing and verifying the complete opposite to what they should have....I obviously should have paid more attention to that and not let it slip by.

/assign @danwinship

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

coveralls · 2020-09-07T21:46:25Z

Coverage increased (+0.2%) to 56.552% when pulling 15bb754 on alexanderConstantinescu:egressip-node-fix into 71ea9e2 on ovn-org:master.

danwinship · 2020-09-08T17:53:54Z

when specifying an egress IP which is a cluster node X's IP address, we might assign it to another node Y in the assignment procedure. This patch thus fixes this by implementing a check and making sure that if the egress IP specified is a cluster node's, that it is also assigned to exactly that node.

Hm... in openshift-sdn it's not valid to use a node's actual IP as an egress IP. Because the node IP is going to be used for random traffic from other pods on that node, so it's not useful as an identifiable egress IP

alexanderConstantinescu · 2020-09-09T10:07:51Z

Hm... in openshift-sdn it's not valid to use a node's actual IP as an egress IP. Because the node IP is going to be used for random traffic from other pods on that node, so it's not useful as an identifiable egress IP

Dohh (insert pic of Homer Simpson)

Do we still want to insure ourselves for such a scenario (i.e, user specifies an egress IP which happens to be a cluster node IP), or does that fall under the "don't do that" category?

danwinship · 2020-09-09T11:45:50Z

We should check for it and consider it unsatisfiable/an error, like whatever we do if they request an egress IP that's not in any of the available CIDR ranges.

alexanderConstantinescu · 2020-09-15T20:18:44Z

We should check for it and consider it unsatisfiable/an error, like whatever we do if they request an egress IP that's not in any of the available CIDR ranges.

FYI @danwinship : I've update the PR to do this.

When an egress IP is specified equaling a node's IP address, we can end up in the situation where the assignment procedure assigns the egress IP to another cluster node, and thus missing the node actually hosting that IP address. This patch thus fixes this by invoking an error during the assignment procedure and triggering an event notifying him/her of this unsupported request. Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

Previously we only logged an error when the specified egress IP was invalid. Having worked on the previous commit I realized that it is more coherent to return an error to the user notifying him/her of this incorrect definition. The risk is that if a user specified multiple egress IPs and one is incorrect and we trigger an event, that this event goes unoticied but causes confusion later on. Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

alexanderConstantinescu · 2020-10-19T10:27:56Z

@danwinship : could you please take a look at this again?

danwinship

Some comments but I'm guessing they probably apply to a lot of the existing code as well, and could maybe be addressed in a general cleanup later

danwinship · 2020-10-19T11:38:00Z

go-controller/pkg/ovn/egressip.go

+func (oc *Controller) isAnyClusterNodeIP(ip net.IP) *eNode {
+	for _, eNode := range oc.eIPAllocator {
+		if (utilnet.IsIPv6(ip) && eNode.v6IP != nil && eNode.v6IP.Equal(ip)) ||
+			(!utilnet.IsIPv6(ip) && eNode.v4IP != nil && eNode.v4IP.Equal(ip)) {


fwiw just if ip.Equal(eNode.v6IP) || ip.Equal(eNode.v4IP) would work.

danwinship · 2020-10-19T11:40:49Z

go-controller/pkg/ovn/egressip.go

-				(!utilnet.IsIPv6(eIPC) && eNodes[i].v4Subnet != nil && eNodes[i].v4Subnet.Contains(eIPC)) {
-				eNodes[i].tainted, oc.eIPAllocator[eNodes[i].name].allocations[eIPC.String()] = true, true
+			if (utilnet.IsIPv6(eIPC) && assignableNodes[i].v6Subnet != nil && assignableNodes[i].v6Subnet.Contains(eIPC)) ||
+				(!utilnet.IsIPv6(eIPC) && assignableNodes[i].v4Subnet != nil && assignableNodes[i].v4Subnet.Contains(eIPC)) {


similarly, could do just

if (assignableNodes[i].v6Subnet != nil && assignableNodes[i].v6Subnet.Contains(eIPC)) || (assignableNodes[i].v4Subnet != nil && assignableNodes[i].v4Subnet.Contains(eIPC)) {

You're allowed to ask if an IPv6 subnet contains an IPv4 IP or vice versa. (The answer is just guaranteed to be "no".)

Now that local gateway mode is removed we can cleanup stuff like modeEgressIP and the likes and just use one data container as a controller for egress IP assignment/setup Signed-off-by: Alexander Constantinescu <aconstan@redhat.com> Cleanup egress IP conditions There were a couple of nits on PR: ovn-org#1668 that I didn't get the chance to fix before it merged. So I am doing that here instead Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

PR: ovn-org#1668 re-worked the way egress IP assignment is done, taking into account that users do not accidentally assign a node IP as egress IP and screws up cluster networking. However, it introduced a bug, namely: we now have a problem with reconciliation of nodes for egress, ultimately skipping any UPDATE event for nodes. Thus, if ovnkube-node does not label a node in question with k8s.ovn.org/node-primary-ifaddr before ovnkube-master received the ADD for that node, then we never proceed to update our internal data with that node when we receive the UPDATE. This commit fixes that Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

Now that local gateway mode is removed we can cleanup stuff like modeEgressIP and the likes and just use one data container as a controller for egress IP assignment/setup Signed-off-by: Alexander Constantinescu <aconstan@redhat.com> Cleanup egress IP conditions There were a couple of nits on PR: ovn-org#1668 that I didn't get the chance to fix before it merged. So I am doing that here instead Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

PR: ovn-org#1668 re-worked the way egress IP assignment is done, taking into account that users do not accidentally assign a node IP as egress IP and screws up cluster networking. However, it introduced a bug, namely: we now have a problem with reconciliation of nodes for egress, ultimately skipping any UPDATE event for nodes. Thus, if ovnkube-node does not label a node in question with k8s.ovn.org/node-primary-ifaddr before ovnkube-master received the ADD for that node, then we never proceed to update our internal data with that node when we receive the UPDATE. This commit fixes that Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

Now that local gateway mode is removed we can cleanup stuff like modeEgressIP and the likes and just use one data container as a controller for egress IP assignment/setup Signed-off-by: Alexander Constantinescu <aconstan@redhat.com> Cleanup egress IP conditions There were a couple of nits on PR: ovn-org#1668 that I didn't get the chance to fix before it merged. So I am doing that here instead Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

PR: ovn-org/ovn-kubernetes#1668 re-worked the way egress IP assignment is done, taking into account that users do not accidentally assign a node IP as egress IP and screws up cluster networking. However, it introduced a bug, namely: we now have a problem with reconciliation of nodes for egress, ultimately skipping any UPDATE event for nodes. Thus, if ovnkube-node does not label a node in question with k8s.ovn.org/node-primary-ifaddr before ovnkube-master received the ADD for that node, then we never proceed to update our internal data with that node when we receive the UPDATE. This commit fixes that Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

Now that local gateway mode is removed we can cleanup stuff like modeEgressIP and the likes and just use one data container as a controller for egress IP assignment/setup Signed-off-by: Alexander Constantinescu <aconstan@redhat.com> Cleanup egress IP conditions There were a couple of nits on PR: ovn-org/ovn-kubernetes#1668 that I didn't get the chance to fix before it merged. So I am doing that here instead Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

…y-pick-1665-to-release-4.13 [release-4.13] OCPBUGS-13170:Fix bug that resulted in routes not be restored to a new vnic

alexanderConstantinescu force-pushed the egressip-node-fix branch from 625102e to 9ebcbf1 Compare September 8, 2020 17:25

alexanderConstantinescu force-pushed the egressip-node-fix branch from 9ebcbf1 to 5ce925f Compare September 15, 2020 20:16

alexanderConstantinescu added 2 commits September 29, 2020 11:38

alexanderConstantinescu force-pushed the egressip-node-fix branch from 5ce925f to 15bb754 Compare September 29, 2020 09:39

danwinship reviewed Oct 19, 2020

View reviewed changes

danwinship merged commit 11f995f into ovn-org:master Oct 19, 2020

alexanderConstantinescu mentioned this pull request Oct 20, 2020

Miscellaneous cleanups and unification #1735

Merged

alexanderConstantinescu mentioned this pull request Nov 16, 2020

Bug 1898178: [release-4.6] Handle egress IP assignment for node IPs openshift/ovn-kubernetes#349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct egress IP assignment for egress node IPs #1668

Correct egress IP assignment for egress node IPs #1668

alexanderConstantinescu commented Sep 7, 2020 •

edited

coveralls commented Sep 7, 2020 •

edited

danwinship commented Sep 8, 2020

alexanderConstantinescu commented Sep 9, 2020

danwinship commented Sep 9, 2020

alexanderConstantinescu commented Sep 15, 2020

alexanderConstantinescu commented Oct 19, 2020

danwinship left a comment •

edited

danwinship Oct 19, 2020

danwinship Oct 19, 2020

Correct egress IP assignment for egress node IPs #1668

Correct egress IP assignment for egress node IPs #1668

Conversation

alexanderConstantinescu commented Sep 7, 2020 • edited

coveralls commented Sep 7, 2020 • edited

danwinship commented Sep 8, 2020

alexanderConstantinescu commented Sep 9, 2020

danwinship commented Sep 9, 2020

alexanderConstantinescu commented Sep 15, 2020

alexanderConstantinescu commented Oct 19, 2020

danwinship left a comment • edited

Choose a reason for hiding this comment

danwinship Oct 19, 2020

Choose a reason for hiding this comment

danwinship Oct 19, 2020

Choose a reason for hiding this comment

alexanderConstantinescu commented Sep 7, 2020 •

edited

coveralls commented Sep 7, 2020 •

edited

danwinship left a comment •

edited