[ovs-dev] [PATCH v7 4/4] Replace router internal MAC with gateway MAC for reply packets
Miguel Angel Ajo Pelayo
majopela at redhat.com
Tue Aug 14 14:15:18 UTC 2018
Thank you very much,
I wasn't able to perform a proper code review, but you can add the
Tested-By: Miguel Angel Ajo <majopela at redhat.com>
The issue I described previously seems to be fixed on the v7 of the series.
On Mon, Aug 6, 2018 at 9:19 PM Mark Michelson <mmichels at redhat.com> wrote:
> On 08/06/2018 01:58 PM, Anil Venkata wrote:
> > Thanks Mark. Kindly look at my comment inline.
> >
> > On Fri, Aug 3, 2018 at 2:17 AM, Mark Michelson <mmichels at redhat.com
> > <mailto:mmichels at redhat.com>> wrote:
> >
> > On 08/01/2018 08:16 AM, vkommadi at redhat.com
> > <mailto:vkommadi at redhat.com> wrote:
> >
> > From: venkata anil <vkommadi at redhat.com
> > <mailto: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
> > <
> http://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html>
> >
> > Reported-by: Miguel Angel Ajo <majopela at redhat.com
> > <mailto:majopela at redhat.com>>
> > Reported-at:
> >
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html
> > <
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html>
> > Signed-off-by: Venkata Anil <vkommadi at redhat.com
> > <mailto: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)
>
> Thank you very much for the explanation. It makes a lot more sense now.
>
> >
> >
> > /* 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
> >
> >
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list