[ovs-dev] [PATCH v7 4/4] Replace router internal MAC with gateway MAC for reply packets
Anil Venkata
anilvenkata at redhat.com
Thu Aug 16 08:35:52 UTC 2018
Thanks Ben
On Wed, Aug 15, 2018 at 6:40 AM, Ben Pfaff <blp at ovn.org> wrote:
> I've asked Justin to take a look at this series.
>
> On Tue, Aug 14, 2018 at 08:48:25PM +0530, Anil Venkata wrote:
> > Gentle reminder requesting more reviews..
> >
> > Mark has reviewed the patch series and Miguel has already tested the
> patch
> > series.
> >
> > Thanks
> > Anil
> >
> > On Tue, Aug 14, 2018 at 7:45 PM, Miguel Angel Ajo Pelayo <
> > majopela at redhat.com> wrote:
> >
> > > 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
> > >>
> > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list