Fix missing ICMP SNAT for L2 UDNs GR

[jcaamano]

On GRs we have an SNAT from the join port IP address to the external IP address for ICMP needs frag sent by OVN to the external network.

In L2 UDNs, even though there is no join network, the internal port has two IPs, one for the pod subnet and another for the join subnet. The ICMP source address will be the first one in lexicographical order which might be the pod subnet one.

The proper SNAT for the GR pod subnet address gets created in the gateway initialization. But:
1 when DisableSNATMultipleGWs=false, it is redundant because there is
already a SNAT for the whole pod subnet
2 when DisableSNATMultipleGWs=true we end up with no SNAT because
cleanupStalePodSNATs cleans it up becaause it never expected that
SNAT

This commit fixes (2)

[qinqon]

LGTM

[tssurya]

what is a "ICMP SNAT" ?
also I am pretty sure the issue you have linked #2 is not correct right?

[jcaamano]

what is a "ICMP SNAT" ?

Explained on the message:

On GRs we have an SNAT from the join port IP address to the external IP address for ICMP needs frag sent by OVN to the external network.

I am pretty sure the issue you have linked #2 is not correct right?

right, edited

[jcaamano]

Registered new flake
#4733

[tssurya]

So IIUC @jcaamano correct me if I am wrong

Generally in OVN we have the lb_force_snat_ip=router_ip which should auto-take care of SNATing to joinIP, but for ICMP needs frag it seems there is a bug in OVN https://issues.redhat.com/browse/FDP-159 due to which we had to manually start adding this SNAT: 49e1d9f
which looks like this right?

snat                                                         169.254.0.13                        100.65.0.3                                                                  
snat                                                         fd69::d                             fd99::3   

We did #4722 that fixed up the whole joinIP util being wrong

So qq what's the difference between:

if gwLRPIPset.Has(routerNat.LogicalIP) {
			continue
		}

and

if gw.containsJoinIP(logicalIP) {
			continue
		}

to me it looks like both are trying to catch the SNAT with joinIP as the prefix right? I think I might be getting confused with stuff..

[tssurya]

In L2 UDNs, even though there is no join network, the internal port has two IPs, one for the pod subnet and another for the join subnet. The ICMP source address will be the first one in lexicographical order which might be the pod subnet one.

yea we discussed this also needs to be fixed: https://redhat-internal.slack.com/archives/C077NGF5HE1/p1726048908687809 opened an issue to track it at the very least

[jcaamano]

So qq what's the difference between:

if gwLRPIPset.Has(routerNat.LogicalIP) {
			continue
		}

and

if gw.containsJoinIP(logicalIP) {
			continue
		}

to me it looks like both are trying to catch the SNAT with joinIP as the prefix right? I think I might be getting confused with stuff..

gwLRPIPset contains, not only the router IP in the join subnet, but also the router IP on the pod subnet for L2 primary UDNs. I could remove the check gw.containsJoinIP(logicalIP) if you would have me do that as it is currently redundant, yeah.

[tssurya]

gwLRPIPset contains, not only the router IP in the join subnet, but also the router IP on the pod subnet for L2 primary UDNs. I could remove the check gw.containsJoinIP(logicalIP) if you would have me do that as it is currently redundant, yeah.

ahhhh ack so its:

    port rtos-l2.network_ovn_layer2_switch
        mac: "0a:58:64:41:00:03"
        ipv6-lla: "fe80::858:64ff:fe41:3"
        networks: ["10.100.200.1/24", "100.65.0.3/16", "2010:100:200::1/60", "fd99::3/64"] ---> these two?

yeah go for that cleanup thanks!

[jcaamano]

@tssurya @qinqon

Done.

For many other things however, the first ip of the list is being used (gwLRPIP[0]). I hope @qinqon has us covered here.

[qinqon]

For many other things however, the first ip of the list is being used (gwLRPIP[0]). I hope @qinqon has us covered here.

That's the join subnet address, I think that's the expectation.

[jcaamano]

For many other things however, the first ip of the list is being used (gwLRPIP[0]). I hope @qinqon has us covered here.

That's the join subnet address, I think that's the expectation.

It is. But previously we used gwLRPIP[0] because we only expected one address in the slice. Now we depend on a certain order. Treading dangerous paths here.

[qinqon]

For many other things however, the first ip of the list is being used (gwLRPIP[0]). I hope @qinqon has us covered here.

That's the join subnet address, I think that's the expectation.

It is. But previously we used gwLRPIP[0] because we only expected one address in the slice. Now we depend on a certain order. Treading dangerous paths here.

Is it worth it a follow up to segregate the node gatewa address and do the gateway things at the layer2 controller.

[jcaamano]

For many other things however, the first ip of the list is being used (gwLRPIP[0]). I hope @qinqon has us covered here.

That's the join subnet address, I think that's the expectation.

It is. But previously we used gwLRPIP[0] because we only expected one address in the slice. Now we depend on a certain order. Treading dangerous paths here.

Is it worth it a follow up to segregate the node gatewa address and do the gateway things at the layer2 controller.

A comment somewhere could be nice. Or filtering them by containsJoinIP could work if we did not want to depend on the order.

[tssurya]

ah woops I forgot to press merge last night... thanks Jaime!