Add support for external subnets #129
Conversation
p4/sidecar-lite.p4
Outdated
| apply { | ||
| if (hdr.ipv4.isValid()) { nat_v4.apply(); } | ||
| if (hdr.ipv6.isValid()) { nat_v6.apply(); } | ||
| if (ingress.forward_needed == false) { |
There was a problem hiding this comment.
something is a bit wonky with indentation here
There was a problem hiding this comment.
I think it's a tab/spaces thing. It looks OK now, I think.
| default_action = NoAction; | ||
| } | ||
|
|
||
| apply { |
There was a problem hiding this comment.
indentation also wonky here
There was a problem hiding this comment.
wonkiness resolved?
There was a problem hiding this comment.
The indentation is still off in places.
p4/sidecar-lite.p4
Outdated
| ingress.forward_tgt = target; | ||
| ingress.forward_vni = vni; | ||
| ingress.forward_mac = mac; | ||
| ingress.forward_needed = true; |
There was a problem hiding this comment.
Is this doing anything at this point? I see that setting this in local::forward_to_sled is influencing how the apply block works here, but it's not clear to me what setting forward_needed here is doing.
There was a problem hiding this comment.
It's not used beyond this point in the nat_ingress control. It's there just for consistency with the forward_to_sled action in the local control.
| default_action = nonlocal; | ||
| } | ||
|
|
||
| table ext_subnet_v4 { |
There was a problem hiding this comment.
A bit odd to have these tables in the local control which is for local address lookups. Feels like there should be an attached_subnet control for this.
There was a problem hiding this comment.
In the real sidecar code, this is checked in the Filter control. In both cases, we need the packet to get through the initial sanity check of "is this packet even addressed to something within our scope". Prior to this, that meant checking to see if it is addressed to one of the links on the switch. We now have external subnet addresses we need to allow in. In the multi-rack future, we are probably going to have additional internal subnets as well.
That's a long way of saying that it might make more sense to rename the local control to filter rather than try to split this code into its own control.
| default_action = NoAction; | ||
| } | ||
|
|
||
| apply { |
There was a problem hiding this comment.
The indentation is still off in places.
No description provided.