On Thu, Sep 7, 2017 at 8:12 AM Jakub Sitnicki <j...@redhat.com> wrote:
> Hi Mark,
>
> On Wed, 6 Sep 2017 16:08:37 -0500
> Mark Michelson <mmich...@redhat.com> wrote:
>
> > The ct_lb action previously assumed that any address arguments were
> > IPv4. This patch expands the parsing, formatting, and encoding of ct_lb
> > to be amenable to IPv6 addresses as well.
> >
> > Signed-off-by: Mark Michelson <mmich...@redhat.com>
> > ---
> > include/ovn/actions.h | 4 ++-
> > ovn/lib/actions.c | 99
> ++++++++++++++++++++++++++++++++++++++++-----------
> > tests/ovn.at | 8 ++++-
> > 3 files changed, 89 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 0a04af7aa..ec0063efc 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -200,7 +200,9 @@ struct ovnact_ct_nat {
> > };
> >
> > struct ovnact_ct_lb_dst {
> > - ovs_be32 ip;
> > + int family;
> > + struct in6_addr ipv6;
> > + ovs_be32 ipv4;
> > uint16_t port;
> > };
> >
>
> Do you think it would make sense to put 'ipv6' and 'ipv4' in an
> anonymous union to emphasize the fact that we're using only one or the
> other? Similar to how it was done in flow_hash_symmetric_l4():
>
>
Sure, I can do that. I think I'm going to wait for more feedback on the
patchset before posting new revisions, though.
Thanks for giving this a look!
> uint32_t
> flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis)
> {
> struct {
> union {
> ovs_be32 ipv4_addr;
> struct in6_addr ipv6_addr;
> };
> ovs_be16 eth_type;
> ovs_be16 vlan_tci;
> ovs_be16 tp_port;
> struct eth_addr eth_addr;
> uint8_t ip_proto;
> } fields;
> /* ... */
> }
>
> -Jakub
>
> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> > index d0d73b69c..8d60529cb 100644
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -883,23 +883,63 @@ parse_ct_lb_action(struct action_context *ctx)
> >
> > if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> > while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> > - if (ctx->lexer->token.type != LEX_T_INTEGER
> > - || mf_subvalue_width(&ctx->lexer->token.value) > 32) {
> > - free(dsts);
> > - lexer_syntax_error(ctx->lexer, "expecting IPv4
> address");
> > - return;
> > - }
> > + struct ovnact_ct_lb_dst dst;
> > + if (lexer_match(ctx->lexer, LEX_T_LSQUARE)) {
> > + /* IPv6 address and port */
> > + if (ctx->lexer->token.type != LEX_T_INTEGER
> > + || ctx->lexer->token.format != LEX_F_IPV6) {
> > + free(dsts);
> > + lexer_syntax_error(ctx->lexer, "expecting IPv6
> address");
> > + return;
> > + }
> > + dst.family = AF_INET6;
> > + dst.ipv6 = ctx->lexer->token.value.ipv6;
> >
> > - /* Parse IP. */
> > - ovs_be32 ip = ctx->lexer->token.value.ipv4;
> > - lexer_get(ctx->lexer);
> > + lexer_get(ctx->lexer);
> > + if (!lexer_match(ctx->lexer, LEX_T_RSQUARE)) {
> > + free(dsts);
> > + lexer_syntax_error(ctx->lexer, "no closing square "
> > + "bracket");
> > + return;
> > + }
> > + dst.port = 0;
> > + if (lexer_match(ctx->lexer, LEX_T_COLON)
> > + && !action_parse_port(ctx, &dst.port)) {
> > + free(dsts);
> > + return;
> > + }
> > + } else {
> > + if (ctx->lexer->token.type != LEX_T_INTEGER
> > + || (ctx->lexer->token.format != LEX_F_IPV4
> > + && ctx->lexer->token.format != LEX_F_IPV6)) {
> > + free(dsts);
> > + lexer_syntax_error(ctx->lexer, "expecting IP
> address");
> > + return;
> > + }
> >
> > - /* Parse optional port. */
> > - uint16_t port = 0;
> > - if (lexer_match(ctx->lexer, LEX_T_COLON)
> > - && !action_parse_port(ctx, &port)) {
> > - free(dsts);
> > - return;
> > + /* Parse IP. */
> > + struct ovnact_ct_lb_dst dst;
> > + if (ctx->lexer->token.format == LEX_F_IPV4) {
> > + dst.family = AF_INET;
> > + dst.ipv4 = ctx->lexer->token.value.ipv4;
> > + } else {
> > + dst.family = AF_INET6;
> > + dst.ipv6 = ctx->lexer->token.value.ipv6;
> > + }
> > +
> > + lexer_get(ctx->lexer);
> > + dst.port = 0;
> > + if (lexer_match(ctx->lexer, LEX_T_COLON)) {
> > + if (dst.family == AF_INET6) {
> > + free(dsts);
> > + lexer_syntax_error(ctx->lexer, "IPv6 address
> needs "
> > + "square brackets if port is included");
> > + return;
> > + } else if (!action_parse_port(ctx, &dst.port)) {
> > + free(dsts);
> > + return;
> > + }
> > + }
> > }
> > lexer_match(ctx->lexer, LEX_T_COMMA);
> >
> > @@ -907,7 +947,7 @@ parse_ct_lb_action(struct action_context *ctx)
> > if (n_dsts >= allocated_dsts) {
> > dsts = x2nrealloc(dsts, &allocated_dsts, sizeof *dsts);
> > }
> > - dsts[n_dsts++] = (struct ovnact_ct_lb_dst) { ip, port };
> > + dsts[n_dsts++] = dst;
> > }
> > }
> >
> > @@ -929,9 +969,19 @@ format_CT_LB(const struct ovnact_ct_lb *cl, struct
> ds *s)
> > }
> >
> > const struct ovnact_ct_lb_dst *dst = &cl->dsts[i];
> > - ds_put_format(s, IP_FMT, IP_ARGS(dst->ip));
> > - if (dst->port) {
> > - ds_put_format(s, ":%"PRIu16, dst->port);
> > + if (dst->family == AF_INET) {
> > + ds_put_format(s, IP_FMT, IP_ARGS(dst->ipv4));
> > + if (dst->port) {
> > + ds_put_format(s, ":%"PRIu16, dst->port);
> > + }
> > + } else {
> > + if (dst->port) {
> > + ds_put_char(s, '[');
> > + }
> > + ipv6_format_addr(&dst->ipv6, s);
> > + if (dst->port) {
> > + ds_put_format(s, "]:%"PRIu16, dst->port);
> > + }
> > }
> > }
> > ds_put_char(s, ')');
> > @@ -991,8 +1041,17 @@ encode_CT_LB(const struct ovnact_ct_lb *cl,
> > BUILD_ASSERT(MFF_LOG_DNAT_ZONE < MFF_REG0 + FLOW_N_REGS);
> > for (size_t bucket_id = 0; bucket_id < cl->n_dsts; bucket_id++) {
> > const struct ovnact_ct_lb_dst *dst = &cl->dsts[bucket_id];
> > + char ip_addr[INET6_ADDRSTRLEN];
> > + if (dst->family == AF_INET) {
> > + inet_ntop(AF_INET, &dst->ipv4, ip_addr, sizeof ip_addr);
> > + } else {
> > + inet_ntop(AF_INET6, &dst->ipv6, ip_addr, sizeof ip_addr);
> > + }
> > ds_put_format(&ds,
> ",bucket=bucket_id=%"PRIuSIZE",weight:100,actions="
> > - "ct(nat(dst="IP_FMT, bucket_id, IP_ARGS(dst->ip));
> > + "ct(nat(dst=%s%s%s", bucket_id,
> > + dst->family == AF_INET6 && dst->port ? "[" : "",
> > + ip_addr,
> > + dst->family == AF_INET6 && dst->port ? "]" : "");
> > if (dst->port) {
> > ds_put_format(&ds, ":%"PRIu16, dst->port);
> > }
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index fb9fc7352..9118f2839 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -793,13 +793,19 @@ ct_lb(192.168.1.2, 192.168.1.3, );
> > formats as ct_lb(192.168.1.2, 192.168.1.3);
> > encodes as group:2
> > has prereqs ip
> > +ct_lb(fd0f::2, fd0f::3, );
> > + formats as ct_lb(fd0f::2, fd0f::3);
> > + encodes as group:3
> > + has prereqs ip
> >
> > ct_lb(192.168.1.2:);
> > Syntax error at `)' expecting port number.
> > ct_lb(192.168.1.2:123456);
> > Syntax error at `123456' expecting port number.
> > ct_lb(foo);
> > - Syntax error at `foo' expecting IPv4 address.
> > + Syntax error at `foo' expecting IP address.
> > +ct_lb([192.168.1.2]);
> > + Syntax error at `192.168.1.2' expecting IPv6 address.
> >
> > # ct_next
> > ct_next;
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
