[ovs-dev] [PATCH v7 4/4] Replace router internal MAC with gateway MAC for reply packets
Anil Venkata
anilvenkata at redhat.com
Mon Aug 6 17:58:41 UTC 2018
Thanks Mark. Kindly look at my comment inline.
On Fri, Aug 3, 2018 at 2:17 AM, Mark Michelson <mmichels at redhat.com> wrote:
> On 08/01/2018 08:16 AM, vkommadi at redhat.com wrote:
>
>> From: venkata anil <vkommadi at redhat.com>
>>
>> Previous patches in the series doesn't address issue 1 explained in [1]
>> i.e
>> 1) removal of router gateway port MAC address on external switches
>> after expiring of aging time.
>> 2) then external switches unable to learn the gateway MAC as
>> reply packets carry router internal port MAC address as source
>>
>> To fix this, router on gateway node will use router gateway MAC address
>> instead of router internal port MAC address as source for reply packets,
>> so that external switches can learn gateway MAC address.
>> This is done only for reply packets from router gateway to tenant VLAN
>> switch ports.
>> Later before delivering the packet to the port, ovn-controller will
>> replace the gateway MAC with router internal port MAC in table 33.
>>
>> [1] //mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html
>>
>> Reported-by: Miguel Angel Ajo <majopela at redhat.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349
>> 803.html
>> Signed-off-by: Venkata Anil <vkommadi at redhat.com>
>> ---
>>
>> v6->v7:
>> * Added this patch
>>
>>
>> ovn/controller/physical.c | 60 ++++++++++++++++++++++++++++++
>> ++++++++++++---
>> ovn/northd/ovn-northd.8.xml | 10 ++++++++
>> ovn/northd/ovn-northd.c | 29 ++++++++++++++++++++++
>> ovn/ovn-architecture.7.xml | 4 ++-
>> 4 files changed, 99 insertions(+), 4 deletions(-)
>>
>> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
>> index f269a1d..1f41f59 100644
>> --- a/ovn/controller/physical.c
>> +++ b/ovn/controller/physical.c
>> @@ -190,7 +190,9 @@ get_zone_ids(const struct sbrec_port_binding *binding,
>> static void
>> put_local_common_flows(uint32_t dp_key, uint32_t port_key,
>> bool nested_container, const struct zone_ids
>> *zone_ids,
>> - struct ofpbuf *ofpacts_p, struct hmap *flow_table)
>> + struct ofpbuf *ofpacts_p, struct hmap *flow_table,
>> + struct local_datapath *ld,
>> + const struct hmap *local_datapaths)
>> {
>> struct match match;
>> @@ -221,11 +223,63 @@ put_local_common_flows(uint32_t dp_key, uint32_t
>> port_key,
>> }
>> }
>> + struct ofpbuf *clone = NULL;
>> + clone = ofpbuf_clone(ofpacts_p);
>> +
>>
>
> I don't understand the use of the cloned ofpbuf here. You could just
> ofpbuf_clear(ofpacts_p) in each iteration of the for loop below and reuse
> ofpacts_p where you've used clone below.
>
>
>
I need to add additional flow with priority 150 along with default flow
which exists with prioirty 100.
1) table=33, priority=100,reg15=0x5,metadata=0x2 actions=load:0x2->NXM_NX_
REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_REG12[],resubmit(,34)
2) table=33, priority=150,reg15=0x5,metadata=0x2,dl_src=fa:16:3e:48:66:e7
actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_
REG12[],mod_dl_src:fa:16:3e:65:f2:ae,resubmit(,34)
The first flow with priority 100 is the default flow. This flow is added
for each output port residing on the current chassis, sets some registers
and resubmits to table 34.
But when the packet has router gateway MAC as source MAC address, I want to
replace the MAC before resubmitting to table 34. Second flow has same match
conditions as first flow and also same actions, additionally it checks for
the source MAC and then modifies the source MAC address.
Before I construct the second flow, I have the registers and resubmit as
part of actions i.e
actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_
REG12[],resubmit(,34)
If I simply add MAC to actions with put_load(mac64, MFF_ETH_SRC, 0, 48,
ofpacts_p); then actions in 2nd flow(with 150 priority) will look like
actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_
REG12[],resubmit(,34),mod_dl_src:fa:16:3e:65:f2:ae
i.e modifying the MAC will happen after resubmit, which is not the expected
behaviour.
So I am cloning the ofpacts_p before resubmit is added to it and using the
cloned one in constructing flow with priority 150.
With this changes, actions will look like
actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_
REG12[],mod_dl_src:fa:16:3e:65:f2:ae,resubmit(,34)
> /* Resubmit to table 34. */
>> put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p);
>> ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, 0,
>> &match, ofpacts_p);
>> + /* For a reply packet from gateway with VLAN switch port as
>> destination
>> + * (excluding localnet_port and external VLAN networks), gateway
>> router
>> + * will use gateway MAC address as source MAC instead of router
>> internal
>> + * port MAC, so that external switches can learn gateway MAC address.
>> + * Here (before packet is given to the port) we replace router
>> gateway
>> + * MAC address with router internal port MAC. */
>> + if (ld->localnet_port && (port_key != ld->localnet_port->tunnel_key))
>> {
>> + for (int i = 0; i < ld->n_peer_dps; i++) {
>> + struct local_datapath *peer_ldp = get_local_datapath(
>> + local_datapaths, ld->peer_dps[i]->peer_dp->tunnel_key);
>> + const struct sbrec_port_binding *crp;
>> + crp = peer_ldp->chassisredirect_port;
>> + if (!crp) {
>> + continue;
>> + }
>> +
>> + if (strcmp(smap_get(&crp->options, "distributed-port"),
>> + ld->peer_dps[i]->peer->logical_port) &&
>> + (port_key != ld->peer_dps[i]->patch->tunnel_key)) {
>> + for (int j = 0; j < crp->n_mac; j++) {
>> + struct lport_addresses laddrs;
>> + if (!extract_lsp_addresses(crp->mac[j], &laddrs)) {
>> + continue;
>> + }
>> + match_set_dl_src(&match, laddrs.ea);
>> + destroy_lport_addresses(&laddrs);
>> + break;
>> + }
>> + for (int j = 0; j < ld->peer_dps[i]->peer->n_mac; j++) {
>> + struct lport_addresses laddrs;
>> + uint64_t mac64;
>> + if (!extract_lsp_addresses(
>> + ld->peer_dps[i]->peer->mac[j], &laddrs)) {
>> + continue;
>> + }
>> + mac64 = eth_addr_to_uint64(laddrs.ea);
>> + put_load(mac64,
>> + MFF_ETH_SRC, 0, 48, clone);
>> + destroy_lport_addresses(&laddrs);
>> + break;
>> + }
>> + put_resubmit(OFTABLE_CHECK_LOOPBACK, clone);
>> + ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 150, 0,
>> + &match, clone);
>> + }
>> + }
>> + }
>> + ofpbuf_delete(clone);
>> +
>> /* Table 34, Priority 100.
>> * =======================
>> *
>> @@ -330,7 +384,7 @@ consider_port_binding(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>> struct zone_ids binding_zones = get_zone_ids(binding,
>> ct_zones);
>> put_local_common_flows(dp_key, port_key, false, &binding_zones,
>> - ofpacts_p, flow_table);
>> + ofpacts_p, flow_table, ld,
>> local_datapaths);
>> match_init_catchall(&match);
>> ofpbuf_clear(ofpacts_p);
>> @@ -531,7 +585,7 @@ consider_port_binding(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>> struct zone_ids zone_ids = get_zone_ids(binding, ct_zones);
>> put_local_common_flows(dp_key, port_key, nested_container,
>> &zone_ids,
>> - ofpacts_p, flow_table);
>> + ofpacts_p, flow_table, ld,
>> local_datapaths);
>> /* Table 0, Priority 200, 150 and 100.
>> * ==============================
>> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
>> index 8fa5272..876c121 100644
>> --- a/ovn/northd/ovn-northd.8.xml
>> +++ b/ovn/northd/ovn-northd.8.xml
>> @@ -2013,6 +2013,16 @@ next;
>> </li>
>> <li>
>> + A priority-100 logical flow with match
>> + <code>inport == <var>GW</var> &&
>> + flags.rcv_from_vlan == 1</code> has actions
>> + <code>eth.dst = <var>E</var>; next;</code>, where
>> + <var>GW</var> is the logical router distributed gateway
>> + port and <var>E</var> is the MAC address of router
>> + distributed gateway port.
>> + </li>
>> +
>> + <li>
>> For each NAT rule in the OVN Northbound database that can
>> be handled in a distributed manner, a priority-100 logical
>> flow with match <code>ip4.src == <var>B</var> &&
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index bcf0b66..d012bb8 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -4419,6 +4419,15 @@ add_route(struct hmap *lflows, const struct
>> ovn_port *op,
>> } else {
>> ds_put_format(&actions, "ip%s.dst", is_ipv4 ? "4" : "6");
>> }
>> +
>> + if (op->peer && op->peer->od->localnet_port &&
>> + op->od->l3dgw_port && op->od->l3redirect_port &&
>> + (op != op->od->l3redirect_port) &&
>> + (op != op->od->l3dgw_port)) {
>> + ds_put_format(&match, " && is_chassis_resident(%s)",
>> + op->od->l3redirect_port->json_key);
>> + ds_put_format(&actions, "; flags.rcv_from_vlan = 1");
>> + }
>> ds_put_format(&actions, "; "
>> "%sreg1 = %s; "
>> "eth.src = %s; "
>> @@ -6131,6 +6140,26 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> hmap *ports,
>> op->lrp_networks.ipv6_addrs[i].network_s,
>> op->lrp_networks.ipv6_addrs[i].plen, NULL, NULL);
>> }
>> +
>> + /* For a reply packet from gateway with VLAN switch port as
>> + * destination, replace router internal port MAC with router
>> gateway
>> + * MAC address, so that external switches can learn gateway MAC
>> + * address. Later before delivering the packet to the port,
>> + * controller will replace the gateway MAC with router internal
>> port
>> + * MAC in table 33. */
>> + if (op->od->l3dgw_port && (op == op->od->l3dgw_port) &&
>> + op->od->l3redirect_port) {
>> + ds_clear(&actions);
>> + ds_clear(&match);
>> + ds_put_format(&match, "inport == %s", op->json_key);
>> + ds_put_format(&match, " && flags.rcv_from_vlan == 1");
>> + ds_put_format(&match, " && is_chassis_resident(%s)",
>> + op->od->l3redirect_port->json_key);
>> + ds_put_format(&actions,
>> + "eth.src = %s; next;", op->lrp_networks.ea_s);
>> + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_GW_REDIRECT, 100,
>> + ds_cstr(&match), ds_cstr(&actions));
>> + }
>> }
>> /* Convert the static routes to flows. */
>> diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
>> index ad2101c..0de41d2 100644
>> --- a/ovn/ovn-architecture.7.xml
>> +++ b/ovn/ovn-architecture.7.xml
>> @@ -1067,7 +1067,9 @@
>> <p>
>> Flows in table 33 resemble those in table 32 but for logical
>> ports that
>> - reside locally rather than remotely. For unicast logical output
>> ports
>> + reside locally rather than remotely. If these are VLAN ports and
>> + packet has router gateway port MAC address as source, replace it
>> with
>> + router internal port MAC address. For unicast logical output
>> ports
>> on the local hypervisor, the actions just resubmit to table
>> 34. For
>> multicast output ports that include one or more logical ports
>> on the
>> local hypervisor, for each such logical port <var>P</var>, the
>> actions
>>
>>
>
More information about the dev
mailing list