[ovs-dev] [PATCH ovn] Honour router_preference for solicited RA

[Numan Siddique]

On Wed, Jun 10, 2020 at 3:01 PM Gabriele Cerami <gcerami at redhat.com> wrote:

> Thanks for the review!
>
> On 10 Jun, Numan Siddique wrote:
>
> > I think test cases are fine. The action parsing test case makes sure that
> > the
> > action is encoded properly.
>
>
> I'm pretty sure they are not. But in production they end up being ok.
> For example checking the tcpdump output at the bottom of the
> description in https://bugzilla.redhat.com/show_bug.cgi?id=1804576
> you can see the RA flags are [none] (so it's using slaac) but the prefix
> info flags are [onlink, auto] and the value is c0, not 80.
>

The action parsing test case doesn't validate that the encoded value is as
per RFC.

While encoding an action, I can deliberately encode a wrong value and
in the action parsing test case in ovn.at I can add the same wrong value as
expected
and the test would pass.

So the developer working on it needs to make sure that it is encoded
properly.
One way to test is run an actual setup and make sure that when the VMs
interface
comes up, it gets configured with proper IPv6 addressed as per the
configuration - i.e slaac, dhcpv6 etc.

The tests in ovn.at use dummy datapath so we need to inject a packet and
then validate
that the response was correct.

So if the existing IPv6 RA encoding was wrong, please fix it in actions.c
and also fix the test case.



> Thing is, I'm not sure why this happens with my patch.
> Do you have time for a counter analysis ?
>
> This was my v2 yesterday, that is getting the incorrect test result
>
>
> diff --git a/lib/actions.c b/lib/actions.c
> index c50615177..7066d597e 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -2535,6 +2535,12 @@ parse_put_nd_ra_opts(struct action_context *ctx,
> const struct expr_field *dst,
>              }
>              break;
>
> +        case ND_RA_FLAG_PRF:
> +            ok = (c->string && (!strcmp(c->string, "MEDIUM") ||
> +                                !strcmp(c->string, "HIGH") ||
> +                                !strcmp(c->string, "LOW")));
> +            break;
> +
>          case ND_OPT_SOURCE_LINKADDR:
>              ok = c->format == LEX_F_ETHERNET;
>              slla_present = true;
> @@ -2580,18 +2586,29 @@ format_PUT_ND_RA_OPTS(const struct ovnact_put_opts
> *po,
>
>  static void
>  encode_put_nd_ra_option(const struct ovnact_gen_option *o,
> -                        struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
> +                        struct ofpbuf *ofpacts, struct ovs_ra_msg *ra)
>  {
>      const union expr_constant *c = o->value.values;
>
>      switch (o->option->code) {
>      case ND_RA_FLAG_ADDR_MODE:
>      {
> -        struct ovs_ra_msg *ra = ofpbuf_at(ofpacts, ra_offset, sizeof *ra);
>          if (!strcmp(c->string, "dhcpv6_stateful")) {
> -            ra->mo_flags = IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG;
> +            ra->mo_flags |= IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG;
>          } else if (!strcmp(c->string, "dhcpv6_stateless")) {
> -            ra->mo_flags = IPV6_ND_RA_FLAG_OTHER_ADDR_CONFIG;
> +            ra->mo_flags |= IPV6_ND_RA_FLAG_OTHER_ADDR_CONFIG;
> +        }
> +        break;
> +    }
> +
> +    case ND_RA_FLAG_PRF:
> +    {
> +        if (!strcmp(c->string, "LOW")) {
> +            ra->mo_flags |= IPV6_ND_RA_OPT_PRF_LOW;
> +        } else if (!strcmp(c->string, "HIGH")) {
> +            ra->mo_flags |= IPV6_ND_RA_OPT_PRF_HIGH;
> +        } else {
> +            ra->mo_flags |= IPV6_ND_RA_OPT_PRF_NORMAL;
>          }
>          break;
>      }
> @@ -2622,7 +2639,6 @@ encode_put_nd_ra_option(const struct
> ovnact_gen_option *o,
>          struct ovs_nd_prefix_opt *prefix_opt =
>              ofpbuf_put_uninit(ofpacts, sizeof *prefix_opt);
>          uint8_t prefix_len = ipv6_count_cidr_bits(&c->mask.ipv6);
> -        struct ovs_ra_msg *ra = ofpbuf_at(ofpacts, ra_offset, sizeof *ra);
>          prefix_opt->type = ND_OPT_PREFIX_INFORMATION;
>          prefix_opt->len = 4;
>          prefix_opt->prefix_len = prefix_len;
> @@ -2640,6 +2656,12 @@ encode_put_nd_ra_option(const struct
> ovnact_gen_option *o,
>          break;
>      }
>      }
> +
> +    /* RFC4191 section 2.2 */
> +    if (ntohs(ra->router_lifetime) == 0x0) {
> +        ra->mo_flags &= IPV6_ND_RA_OPT_PRF_RESET_MASK;
> +    }
> +
>  }
>
>  static void
> @@ -2660,7 +2682,6 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts
> *po,
>       * pinctrl module receives the ICMPv6 Router Solicitation packet
>       * it can copy the userdata field AS IS and resume the packet.
>       */
> -    size_t ra_offset = ofpacts->size;
>      struct ovs_ra_msg *ra = ofpbuf_put_zeros(ofpacts, sizeof *ra);
>      ra->icmph.icmp6_type = ND_ROUTER_ADVERT;
>      ra->cur_hop_limit = IPV6_ND_RA_CUR_HOP_LIMIT;
> @@ -2669,7 +2690,7 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts
> *po,
>
>      for (const struct ovnact_gen_option *o = po->options;
>           o < &po->options[po->n_options]; o++) {
> -        encode_put_nd_ra_option(o, ofpacts, ra_offset);
> +        encode_put_nd_ra_option(o, ofpacts, ra);
>      }
>
>      encode_finish_controller_op(oc_offset, ofpacts);
>
> *******
>
> > Also you need to add few tests in ovn.at for action parsing and also
> enhance
>
> ah, sure !
>
> > the test - AT_SETUP([ovn -- IPv6 ND Router Solicitation responder]) in
> > ovn.at
>
> enhance in what way? I'm covering all the cases using existing tests ...
> I'd
> wish to cover the flags reset on 0 lifetime, but the lifetime value is
> hardcoded as infinity so I'm not sure how to test that.
>

I mean to add/enhance the test case here -
https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10534

You can configure the Router preference option here -
https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10549
and make sure that the response is as expected here.  One example here -
https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10689

Thanks
Numan



>
> Thanks!
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>