[ovs-dev] [PATCH] ovn: Add a new action 'nd_na_router' to handle NS requests for router IPs
Miguel Angel Ajo Pelayo
majopela at redhat.com
Tue May 8 09:39:49 UTC 2018
Ack, makes sense to me
On Tue, May 8, 2018 at 11:36 AM, Numan Siddique <nusiddiq at redhat.com> wrote:
>
>
> On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo <
> 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>
>> 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.
>
> [1] - 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 wrote:
>>> > From: Numan Siddique <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 which
>>> > I couldn't resolve.)
>>> >
>>> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1567735
>>> > CC: Justin Pettit <jpettit at ovn.org>
>>> > CC: Mark Michelson <mmichels at redhat.com>
>>> > Signed-off-by: Numan Siddique <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 | 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 b/tests/ovn.at
>>> > index 4a5316510..2c0ae9877 100644
>>> > --- a/tests/ovn.at
>>> > +++ b/tests/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.0
>>> 3.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.0
>>> 0.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.1
>>> 8.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.0
>>> 4.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.1
>>> 0.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.0
>>> c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.0
>>> 0.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.1
>>> 8.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.0
>>> 4.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.1
>>> 0.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
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>
More information about the dev
mailing list