[ovs-dev] [PATCH] ovn: Add a new action 'nd_na_router' to handle NS requests for router IPs
Mark Michelson
mmichels at redhat.com
Tue May 8 13:26:21 UTC 2018
On 05/08/2018 05:36 AM, Numan Siddique wrote:
>
>
> On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo
> <majopela at redhat.com <mailto:majopela at redhat.com>> wrote:
>
> Thank you Numan!
>
> It took me a bit to find what the "RSO" flag was, because they are
> R, S and O, may be we can change that in the commit, or reference
> the RFC/section (RFC4861 page 23).
>
>
> Ack. I will update the commit message and send v2.
>
>
> R Router flag. When set, the R-bit indicates that
> the sender is a router. The R-bit is used by
> Neighbor Unreachability Detection to detect a
> router that changes to a host.
>
> S Solicited flag. When set, the S-bit indicates that
> the advertisement was sent in response to a
> Neighbor Solicitation from the Destination address.
> The S-bit is used as a reachability confirmation
> for Neighbor Unreachability Detection. It MUST NOT
> be set in multicast advertisements or in
> unsolicited unicast advertisements.
>
> O Override flag. When set, the O-bit indicates that
> the advertisement should override an existing cache
> entry and update the cached link-layer address.
> When it is not set the advertisement will not
> update a cached link-layer address though it will
> update an existing Neighbor Cache entry for which
> no link-layer address is known. It SHOULD NOT be
> set in solicited advertisements for anycast
> addresses and in solicited proxy advertisements.
> It SHOULD be set in other solicited advertisements
> and in unsolicited advertisements.
>
>
>
>
> And same question Mark did.
>
> Thanks for handling this, good work :)
>
> On Mon, May 7, 2018 at 9:16 PM Mark Michelson <mmichels at redhat.com
> <mailto:mmichels at redhat.com>> wrote:
>
> Hi Numan, thanks for the fix.
>
> Out of curiosity, why did you add a new action instead of adding
> a new
> logical field (something like nd.rso)?
>
>
> The logical flow which uses nd_na looks like
>
> table=11(ls_in_arp_rsp ), priority=50 , match=(nd_ns &&
> ip6.dst == {2002::f816:3eff:fefe:9e2e, ff02::1:fffe:9e2e} && nd.target
> == 2002::f816:3eff:fefe:9e2e), action=(nd_na { eth.src =
> fa:16:3e:fe:9e:2e; ip6.src = 2002::f816:3eff:fefe:9e2e; nd.target =
> 2002::f816:3eff:fefe:9e2e; nd.tll = fa:16:3e:fe:9e:2e; outport = inport;
> flags.loopback = 1; output; };)
>
>
> I suppose you are suggesting to have something like - " nd_na { ..,
> nd.rso = router ..} ". All the inner logical fields are defined in expr
> symtab table [1]. But we cannot add nd.rso there as there is no
> corresponding ovs field to modify the ICMPv6 flags of a packet.
>
> From the email discussion here [2] I suppose Zak is working to add this
> feature. Once we have this support, we can make use of that in OVN.
> Right now IPv6 feature is blocked in Openstack + OVN because of this
> issue so we need this fix.
Yes, this is what I had been thinking. It sounds like if the timing had
been different, you could have waited for Zak's patch first. But given
the circumstances, I suppose this will work.
>
> [1] -
> https://github.com/openvswitch/ovs/blob/master/ovn/lib/logical-fields.c#L205
> <https://github.com/openvswitch/ovs/blob/master/ovn/lib/logical-fields.c#L205>
> [2] -
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046662.html
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046528.html
>
>
> Thanks
> Numan
>
>
>
> On 05/07/2018 02:36 PM, nusiddiq at redhat.com
> <mailto:nusiddiq at redhat.com> wrote:
> > From: Numan Siddique <nusiddiq at redhat.com
> <mailto:nusiddiq at redhat.com>>
> >
> > Presently when a VM's IPv6 stack sends a Neighbor
> Solicitation request for its
> > router IP, (mostly when the ND cache entry for the router is
> in STALE state)
> > ovn-controller responds with a Neighbor Adv packet (using the
> action nd_na).
> > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because
> of which, the
> > VM deletes the default route. The default route gets added
> again when the next
> > RA is received (but would again gets deleted if its sends NS
> request). And this
> > results in disruption of IPv6 traffic.
> >
> > This patch addresses this issue by adding a new action
> 'nd_na_router' which is
> > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO
> flags. ovn-northd
> > uses this action. A new action is added instead of modifying
> the existing 'nd_na'
> > action. This is because
> > - We cannot set the RSO flags in the "nd_na { ..actions .. }"
> > - It would be ugly to have something like nd_na {
> router_flags, ...actions .. }
> >
> > (Please note: There are 3 'Line length is >79-characters'
> warnings in ovn.at <http://ovn.at> which
> > I couldn't resolve.)
> >
> > Reported-at:
> https://bugzilla.redhat.com/show_bug.cgi?id=1567735
> <https://bugzilla.redhat.com/show_bug.cgi?id=1567735>
> > CC: Justin Pettit <jpettit at ovn.org <mailto:jpettit at ovn.org>>
> > CC: Mark Michelson <mmichels at redhat.com
> <mailto:mmichels at redhat.com>>
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com
> <mailto:nusiddiq at redhat.com>>
> > ---
> > include/ovn/actions.h | 7 +++++++
> > ovn/controller/pinctrl.c | 23 +++++++++++++++--------
> > ovn/lib/actions.c | 22 ++++++++++++++++++++++
> > ovn/northd/ovn-northd.c | 2 +-
> > ovn/utilities/ovn-trace.c | 1 +
> > tests/ovn.at <http://ovn.at> | 5 +++++
> > 6 files changed, 51 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index fb8f51509..638465193 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -68,6 +68,7 @@ struct ovn_extend_table;
> > OVNACT(ICMP6, ovnact_nest) \
> > OVNACT(TCP_RESET, ovnact_nest) \
> > OVNACT(ND_NA, ovnact_nest) \
> > + OVNACT(ND_NA_ROUTER, ovnact_nest) \
> > OVNACT(GET_ARP, ovnact_get_mac_bind) \
> > OVNACT(PUT_ARP, ovnact_put_mac_bind) \
> > OVNACT(GET_ND, ovnact_get_mac_bind) \
> > @@ -444,6 +445,12 @@ enum action_opcode {
> > * The actions, in OpenFlow 1.3 format, follow the
> action_header.
> > */
> > ACTION_OPCODE_TCP_RESET,
> > +
> > + /* "nd_na_router { ...actions... }" with rso flag
> 'ND_RSO_ROUTER' set.
> > + *
> > + * The actions, in OpenFlow 1.3 format, follow the
> action_header.
> > + */
> > + ACTION_OPCODE_ND_NA_ROUTER,
> > };
> >
> > /* Header. */
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 6e6aa1caa..305f20649 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -81,7 +81,8 @@ static void send_garp_run(struct
> controller_ctx *ctx,
> > struct sset *active_tunnels);
> > static void pinctrl_handle_nd_na(const struct flow *ip_flow,
> > const struct match *md,
> > - struct ofpbuf *userdata);
> > + struct ofpbuf *userdata,
> > + bool is_router);
> > static void reload_metadata(struct ofpbuf *ofpacts,
> > const struct match *md);
> > static void pinctrl_handle_put_nd_ra_opts(
> > @@ -1154,7 +1155,11 @@ process_packet_in(const struct
> ofp_header *msg, struct controller_ctx *ctx)
> > break;
> >
> > case ACTION_OPCODE_ND_NA:
> > - pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
> &userdata);
> > + pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
> &userdata, false);
> > + break;
> > +
> > + case ACTION_OPCODE_ND_NA_ROUTER:
> > + pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
> &userdata, true);
> > break;
> >
> > case ACTION_OPCODE_PUT_ND:
> > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts,
> const struct match *md)
> >
> > static void
> > pinctrl_handle_nd_na(const struct flow *ip_flow, const
> struct match *md,
> > - struct ofpbuf *userdata)
> > + struct ofpbuf *userdata, bool is_router)
> > {
> > /* This action only works for IPv6 ND packets, and the
> switch should only
> > * send us ND packets this way, but check here just to
> be sure. */
> > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct
> flow *ip_flow, const struct match *md,
> > struct dp_packet packet;
> > dp_packet_use_stub(&packet, packet_stub, sizeof
> packet_stub);
> >
> > - /* xxx These flags are not exactly correct. Look at
> section 7.2.4
> > - * xxx of RFC 4861. For example, we need to set
> ND_RSO_ROUTER for
> > - * xxx router's interfaces and ND_RSO_SOLICITED only if
> it was
> > - * xxx requested. */
> > + /* These flags are not exactly correct. Look at section
> 7.2.4
> > + * of RFC 4861. */
> > + uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
> > + if (is_router) {
> > + rso_flags |= ND_RSO_ROUTER;
> > + }
> > compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
> > &ip_flow->nd_target, &ip_flow->ipv6_src,
> > - htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
> > + htonl(rso_flags));
> >
> > /* Reload previous packet metadata and set actions from
> userdata. */
> > set_actions_and_enqueue_msg(&packet, md, userdata);
> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> > index a6945812d..0669cc1c9 100644
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
> > parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
> > }
> >
> > +static void
> > +parse_ND_NA_ROUTER(struct action_context *ctx)
> > +{
> > + parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
> > +}
> > +
> > static void
> > parse_ND_NS(struct action_context *ctx)
> > {
> > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest
> *nest, struct ds *s)
> > format_nested_action(nest, "nd_na", s);
> > }
> >
> > +static void
> > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct
> ds *s)
> > +{
> > + format_nested_action(nest, "nd_na_router", s);
> > +}
> > +
> > static void
> > format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
> > {
> > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest *on,
> > encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA,
> ofpacts);
> > }
> >
> > +static void
> > +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
> > + const struct ovnact_encode_params *ep,
> > + struct ofpbuf *ofpacts)
> > +{
> > + encode_nested_actions(on, ep,
> ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
> > +}
> > +
> > static void
> > encode_ND_NS(const struct ovnact_nest *on,
> > const struct ovnact_encode_params *ep,
> > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
> > parse_TCP_RESET(ctx);
> > } else if (lexer_match_id(ctx->lexer, "nd_na")) {
> > parse_ND_NA(ctx);
> > + } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
> > + parse_ND_NA_ROUTER(ctx);
> > } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
> > parse_ND_NS(ctx);
> > } else if (lexer_match_id(ctx->lexer, "get_arp")) {
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index ce472a536..b157cd1eb 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap
> *datapaths, struct hmap *ports,
> > ds_clear(&actions);
> > ds_put_format(&actions,
> > "put_nd(inport, ip6.src, nd.sll); "
> > - "nd_na { "
> > + "nd_na_router { "
> > "eth.src = %s; "
> > "ip6.src = %s; "
> > "nd.target = %s; "
> > diff --git a/ovn/utilities/ovn-trace.c
> b/ovn/utilities/ovn-trace.c
> > index 9c19b5b9a..1fd48f22e 100644
> > --- a/ovn/utilities/ovn-trace.c
> > +++ b/ovn/utilities/ovn-trace.c
> > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact
> *ovnacts, size_t ovnacts_len,
> > break;
> >
> > case OVNACT_ND_NA:
> > + case OVNACT_ND_NA_ROUTER:
> > execute_nd_na(ovnact_get_ND_NA(a), dp, uflow,
> table_id, pipeline,
> > super);
> > break;
> > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
> <http://ovn.at>
> > index 4a5316510..2c0ae9877 100644
> > --- a/tests/ovn.at <http://ovn.at>
> > +++ b/tests/ovn.at <http://ovn.at>
> > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc;
> nd.tll = 12:34:56:78:9a:bc; outport = inpor
> > formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll
> = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
> > encodes as
> controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> > has prereqs nd_ns
> > +# nd_na_router
> > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
> 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow
> sending out inport. */ output; };
> > + formats as nd_na_router { eth.src = 12:34:56:78:9a:bc;
> nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = "";
> output; };
> > + encodes as
> controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> > + has prereqs nd_ns
> >
> > # get_nd
> > get_nd(outport, ip6.dst);
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org <mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>
>
More information about the dev
mailing list