[ovs-dev] [PATCH ovn] expr: Combine multiple ipv4 with wildcard mask.

gmingchen(陈供明) gmingchen at tencent.com
Mon Mar 22 12:10:11 UTC 2021



On 2021/3/19, 9:02 PM, "Dumitru Ceara" <dceara at redhat.com> wrote:

    On 3/10/21 2:29 PM, gmingchen(陈供明) wrote:
    > From: Gongming Chen <gmingchen at tencent.com>
    > 
    > This patch merges ipv4 addresses with wildcard masks, and replaces this
    > ipv4 addresses with the combined ip/mask. This will greatly reduce the
    > entries in the ovs security group flow table, especially when the host
    > size is large.
    > 
    > Analysis in the simplest scenario, a network 1.1.1.0/24 network,create 253
    > ports(1.1.1.2-1.1.1.254).
    > Only focus on the number of ip addresses, the original 253 addresses will
    > be combined into 13 addresses.
    > 1.1.1.2/31
    > 1.1.1.4/30
    > 1.1.1.8/29
    > 1.1.1.16/28
    > 1.1.1.32/27
    > 1.1.1.64/26
    > 1.1.1.128/26
    > 1.1.1.192/27
    > 1.1.1.224/28
    > 1.1.1.240/29
    > 1.1.1.248/30
    > 1.1.1.252/31
    > 1.1.1.254
    > 
    > Some scenes are similar to the following:
    >     1.1.1.2, 1.1.1.6
    > After the combine:
    >     1.1.1.2/255.255.255.251
    > You can use ovn-match-ip utility to match ip.
    > such as:
    >     ovs-ofctl dump-flows br-int | ovn-match-ip 1.1.1.6
    >     1.1.1.2/255.255.255.251 will show.
    > 
    > Simple description of the algorithm.
    > There are two situations
    > 1. Combine once
    > such as:
    >     1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
    >     Combined into: 1.1.1.0/31, 1.0.1.0/31
    > 2. Combine multiple times
    >     1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
    >     Combined into: 1.0.1.0/255.254.255.254
    > 
    > Considering the actual scene and simplicity, the first case is used to
    > combine once.
    > 
    > ...00...
    > ...01...
    > ...10...
    > ...11...
    > "..." means the same value omitted.
    > Obviously, the above value can be expressed as ...00.../11100111. This
    > continuous interval that can be represented by one or several wildcard
    > masks is called a segment.
    > Only if all 2<<n values from the minimum value 00...(n) to 11...(n)
    > exist, can they be combined into 00...(n)/00...( n)
    > 
    > First sort all the values by size. Iterate through each value.
    > 1. Find a new segment, where two values differ only by 1 bit, such as
    > ...0... and ...1...
    >     diff = ip_next ^ ip
    >     if (diff & (diff-1)) == 0
    >         new_segment = true
    > The first non-zero place in the high direction of ip is the end of the
    > segment(segment_end).
    > For example...100... and...101..., the segment_end is ...111...
    > 
    > 2. Count the number of consecutive and less than continuous_size in the
    > segment.
    >     diff = ip_next - ip
    >     if (diff & (diff-1)) == 0 && ip_next <= segment_end
    >         continuous_size++
    > 
    > 3. Combine different ip intervals in the segment according to
    > continuous_size.
    > In continuous_size, from the highest bit of 1 to the lowest bit of 1, in
    > the order of segment start, each bit that is 1 is a different ip interval
    > that can be combined with a wildcard mask.
    > For example, 000, 001, 010:
    >     continuous_size: 3 (binary 11), segment_start: 000
    >     mask: ~(1 << 1 - 1) = 110; ~(1 << 0 - 1) = 111;
    >     Combined to: 000/110, 010/111
    > 
    > 4. The ip that cannot be recorded in a segment will not be combined.
    > 
    > Signed-off-by: Gongming Chen <gmingchen at tencent.com>
    > ---

    Hi Gongming,

    Sorry for the delayed review.

    I have a few general remarks/concerns and some specific comments inline.
     First, the general remarks.

    I'm wondering if it would make more sense for this wildcard combination
    of IPs to be done outside OVN, in the CMS, for the following reasons:

    - it's IPv4 specific.
    - the CMS usually has more information about when it is matching on IP
    sets and can probably optimize the match it uses in the ACL.
    - the code in expr_const_sets_add() used to be a relatively direct
    translation from sets of constants to sets of port names/IPs; this
    changes with the optimization your proposing.
    - the new utility, ovn-match-ip, would be useful but I'm worried that
    users will often forget to use it.

    I'm curious about your and the maintainers' opinion on these points.

Hi Dumitru,
First, thanks for your review.

On the one hand, I agree with your point of view. The cms side is more
clear about the details of the ip, and the ip can be better optimized.
However, on the other hand, I think that ovn, as a network provider, should
not assume that upper layers such as cms have already made these optimizations.
In fact, some cms do not have such a function, such as openstack, k8s,
and they hope that this specific and complex work will be realized by
the network provider.
In terms of better serving the upper layer or upper layer use, this
optimization function of ovn simplifies the upper layer work, which
will just make users more satisfied.
This is some of my views from the perspective of cms.

    Another general remark is to increment the patch version when submitting
    a new revision.  From what I can see this specific patch is at v7.  When
    sending a v8 you could add "-v8" when generating the patch, e.g.:

    git format-patch --subject-prefix="PATCH ovn" -v8 -M origin/master

    Would generate a patch with subject:

    "Subject: [PATCH ovn v8] ...."

    This usually helps when reviewing patches that go through multiple
    iterations.

Thanks.

    More specific comments inline.

    >  debian/ovn-common.install |   1 +
    >  lib/expr.c                | 219 ++++++++++++++++++++++++++++++++++++++
    >  rhel/ovn-fedora.spec.in   |   1 +
    >  tests/ovn.at              | 136 +++++++++++------------
    >  tests/test-ovn.c          |   9 ++
    >  utilities/automake.mk     |   5 +-
    >  utilities/ovn-match-ip.in |  78 ++++++++++++++
    >  7 files changed, 382 insertions(+), 67 deletions(-)
    >  create mode 100755 utilities/ovn-match-ip.in
    > 
    > diff --git a/debian/ovn-common.install b/debian/ovn-common.install
    > index 8e5915724..0095df918 100644
    > --- a/debian/ovn-common.install
    > +++ b/debian/ovn-common.install
    > @@ -5,6 +5,7 @@ usr/bin/ovn-ic-nbctl
    >  usr/bin/ovn-ic-sbctl
    >  usr/bin/ovn-trace
    >  usr/bin/ovn-detrace
    > +usr/bin/ovn-match-ip
    >  usr/share/ovn/scripts/ovn-ctl
    >  usr/share/ovn/scripts/ovndb-servers.ocf
    >  usr/share/ovn/scripts/ovn-lib
    > diff --git a/lib/expr.c b/lib/expr.c
    > index f061a8fbe..0a7203817 100644
    > --- a/lib/expr.c
    > +++ b/lib/expr.c
    > @@ -1060,6 +1060,200 @@ expr_constant_set_destroy(struct expr_constant_set *cs)
    >      }
    >  }
    >  
    > +struct ip_in
    > +{
    > +    uint32_t *ip;
    > +    size_t size;
    > +};

    I don't think we really need this, we could just pass the IPv4 address
    array to combine_ipv4_in_mask(), along with the number of items in the
    array.

Yes, in combine_ipv4_in_mask(), size of ipv4 array is not necessary.
But I think that the structure composed of *ip and size is more helpful for
expr_const_sets_add to initialize an array memory of n_values size instead
of accurately malloc the entry size of LEX_F_IPV4.
Because expr_const_sets_add does not know the number of LEX_F_IPV4
when initializing the ipv4 array.
What do you think?

    > +
    > +struct ip_out_entry
    > +{
    > +    uint32_t ip;
    > +    uint32_t mask;
    > +    bool masked;
    > +};
    > +
    > +struct ip_out
    > +{
    > +    struct ip_out_entry *entries;
    > +    size_t used;
    > +    size_t size;
    > +};

    AFAIU, we can never have more "out" entries than "in" entries.  This
    means we could be conservative and always allocate as many output
    entries as ip_in entries and pass the preallocated struct ip_out_entry
    array to combine_ipv4_in_mask().

    This would also remove the need to resize the "out" array.

Yes,  good idea.

    > +
    > +static int
    > +compare_mask_ip(const void *a, const void *b)

    The implementation doesn't really use the fact that 'a' and 'b' are IPs.
     They're just compared as integers.  We might want to use a different
    name and move this function to a common module as it could potentially
    be used in multiple places in the future.

How about adding compare_uint32s(const void *a_, const void *b_) to lib/util.c?

    > +{
    > +    uint32_t a_ = *(uint32_t *)a;
    > +    uint32_t b_ = *(uint32_t *)b;
    > +
    > +    return a_ < b_ ? -1 : a_ > b_;
    > +}
    > +
    > +/* Function to check ip return data and xrealloc. */
    > +static void
    > +check_realloc_ip_out(struct ip_out *ip_out_data){
    > +    if (ip_out_data->used >= ip_out_data->size) {
    > +        ip_out_data->entries = x2nrealloc(ip_out_data->entries,
    > +                                    &ip_out_data->size,
    > +                                    sizeof *ip_out_data->entries);
    > +    }
    > +}

    As mentioned in the comment earlier, we can probably avoid using this
    function.

Yes.

    > +
    > +/* Simple description of the algorithm.
    > + * There are two situations
    > + * 1. Combine once
    > + * such as:
    > + *     1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
    > + *     Combined into: 1.1.1.0/31, 1.0.1.0/31
    > + * 2. Combine multiple times
    > + *     1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
    > + *     Combined into: 1.0.1.0/255.254.255.254
    > + *
    > + * Considering the actual scene and simplicity, the first case is used to
    > + * combine once.
    > + *
    > + * ...00...
    > + * ...01...
    > + * ...10...
    > + * ...11...
    > + * "..." means the same value omitted.
    > + * Obviously, the above value can be expressed as ...00.../11100111. This
    > + * continuous interval that can be represented by one or several wildcard
    > + * masks is called a segment.
    > + * Only if all 2<<n values from the minimum value 00...(n) to 11...(n)
    > + * exist, can they be combined into 00...(n)/00...( n)
    > + *
    > + * First sort all the values by size. Iterate through each value.
    > + * 1. Find a new segment, where two values differ only by 1 bit, such as
    > + * ...0... and ...1...
    > + *     diff = ip_next ^ ip
    > + *     if (diff & (diff-1)) == 0
    > + *         new_segment = true
    > + * The first non-zero place in the high direction of ip is the end of the
    > + * segment(segment_end).
    > + * For example...100... and...101..., the segment_end is ...111...
    > + *
    > + * 2. Count the number of consecutive and less than continuous_size in the
    > + * segment.
    > + *     diff = ip_next - ip
    > + *     if (diff & (diff-1)) == 0 && ip_next <= segment_end
    > + *         continuous_size++
    > + *
    > + * 3. Combine different ip intervals in the segment according to
    > + * continuous_size.
    > + * In continuous_size, from the highest bit of 1 to the lowest bit of 1, in
    > + * the order of segment start, each bit that is 1 is a different ip interval
    > + * that can be combined with a wildcard mask.
    > + * For example, 000, 001, 010:
    > + *     continuous_size: 3 (binary 11), segment_start: 000
    > + *     mask: ~(1 << 1 - 1) = 110; ~(1 << 0 - 1) = 111;
    > + *     Combined to: 000/110, 010/111
    > + *
    > + * 4. The ip that cannot be recorded in a segment will not be combined. */

    Thanks for adding the detailed description here.  However, it's a bit
    confusing because the names used in the description don't match the
    variable names used in the implementation below.  Maybe it's better to
    not use names in the description?

Ok, It would be better to replace it with a general description word.

    > +static void
    > +combine_ipv4_in_mask(struct ip_in *ip_in_data, struct ip_out *ip_out_data){

    Please move the curly brace on a new line.

Ok,  thanks.

    > +    bool recording = false;
    > +    uint32_t i, diff, connect, start, end, continuous_size, mask_base;

    Some of these can be moved in the specific blocks where they're used.

Ok.

    > +
    > +    start = 0;
    > +    continuous_size = 0;
    > +    mask_base = 0;
    > +    end = 0;
    > +
    > +    qsort(ip_in_data->ip, ip_in_data->size, sizeof(uint32_t),

    Please use "sizeof *ip_in_data->ip".

Ok, thanks.

    > +          compare_mask_ip);
    > +    memset(ip_out_data, 0, sizeof(struct ip_out));
    > +
    > +    for (i = 0; i < ip_in_data->size; i++) {
    > +        if (i + 1 >= ip_in_data->size) {
    > +            if (!recording) {
    > +                goto end_when_not_recording;
    > +            } else {
    > +                goto end_when_recording;
    > +            }

    IMO it makes the code very hard to read if we have gotos jumping inside
    if/else branches or inside loops.  This is also usually a sign that
    we're trying to avoid code duplication.  A potentially simpler and more
    readable way to do that is to add functions for the specific operations
    we don't want to duplicate.

Ok,  I will use function instead of goto.

Thanks,
Gongming

    > +        }
    > +        /* Not recording in a segment.
    > +         * Record a new segment or not combine. */
    > +        if (!recording) {
    > +            connect = ip_in_data->ip[i + 1] ^ ip_in_data->ip[i];
    > +            /* Only one bit different. */
    > +            if ((connect & (connect - 1)) == 0) {
    > +                recording = true;
    > +                start = ip_in_data->ip[i];
    > +                continuous_size = 2;
    > +                mask_base = connect;
    > +
    > +                int j = 0;
    > +                end = start;
    > +                /* The first non-zero place in the high direction is
    > +                 * the end of the segment. */
    > +                while (j < 32 && ((start & (mask_base << j)) == 0)) {
    > +                    end |= (mask_base << j);
    > +                    j++;
    > +                }
    > +
    > +                continue;
    > +            /* Different segments and different bit, dnot combine. */
    > +            } else {
    > +end_when_not_recording:

    Same here.

    > +                check_realloc_ip_out(ip_out_data);
    > +
    > +                ip_out_data->entries[ip_out_data->used].ip =
    > +                    ip_in_data->ip[i];
    > +                ip_out_data->entries[ip_out_data->used].masked = false;
    > +                ip_out_data->used++;
    > +
    > +                continue;
    > +            }
    > +        /* Recording in the current segment. */
    > +        } else {
    > +            diff = ip_in_data->ip[i + 1] - ip_in_data->ip[i];
    > +            /* Ignore equal node. */
    > +            if (diff == 0) {
    > +                continue;
    > +            }
    > +            /* Stop recording and combine, or continue recording. */
    > +            if (((diff & (diff - 1)) != 0)
    > +                || (ip_in_data->ip[i + 1] > end)) {
    > +end_when_recording:

    Here too.

    > +                recording = false;
    > +                while (continuous_size) {
    > +                    check_realloc_ip_out(ip_out_data);
    > +
    > +                    int segment_power, pow_base;
    > +                    if (continuous_size == 0) {
    > +                        segment_power = 0;
    > +                    } else {
    > +                        segment_power = 31 - clz32(continuous_size);
    > +                    }
    > +
    > +                    if (mask_base == 0) {
    > +                        pow_base = 0;
    > +                    } else {
    > +                        pow_base = 31 - clz32(mask_base);
    > +                    }
    > +
    > +                    ip_out_data->entries[ip_out_data->used].mask =
    > +                        ~(((1 << segment_power) - 1) << pow_base);
    > +                    ip_out_data->entries[ip_out_data->used].ip =
    > +                        ip_out_data->entries[ip_out_data->used].mask
    > +                        & start;
    > +                    ip_out_data->entries[ip_out_data->used].masked = true;
    > +                    ip_out_data->used++;
    > +
    > +                    continuous_size &= (~(1 << segment_power));
    > +                    start = ip_out_data->entries[ip_out_data->used - 1].ip
    > +                            + (1 << (segment_power + pow_base));
    > +                }
    > +            } else {
    > +                continuous_size++;
    > +            }
    > +
    > +            continue;
    > +        }
    > +    }
    > +}
    > +
    >  /* Adds an constant set named 'name' to 'const_sets', replacing any existing
    >   * constant set entry with the given name. */
    >  void
    > @@ -1074,6 +1268,11 @@ expr_const_sets_add(struct shash *const_sets, const char *name,
    >      cs->in_curlies = true;
    >      cs->n_values = 0;
    >      cs->values = xmalloc(n_values * sizeof *cs->values);
    > +    struct ip_out ip_out_data;
    > +    struct ip_in ip_in_data;
    > +    ip_in_data.ip = xmalloc(n_values * sizeof(uint32_t));
    > +    ip_in_data.size = 0;
    > +
    >      if (convert_to_integer) {
    >          cs->type = EXPR_C_INTEGER;
    >          for (size_t i = 0; i < n_values; i++) {
    > @@ -1086,6 +1285,9 @@ expr_const_sets_add(struct shash *const_sets, const char *name,
    >                  && lex.token.type != LEX_T_MASKED_INTEGER) {
    >                  VLOG_WARN("Invalid constant set entry: '%s', token type: %d",
    >                            values[i], lex.token.type);
    > +            } else if (lex.token.type == LEX_T_INTEGER
    > +                       && lex.token.format == LEX_F_IPV4) {
    > +                ip_in_data.ip[ip_in_data.size++] = ntohl(lex.token.value.ipv4);
    >              } else {
    >                  union expr_constant *c = &cs->values[cs->n_values++];
    >                  c->value = lex.token.value;
    > @@ -1105,6 +1307,23 @@ expr_const_sets_add(struct shash *const_sets, const char *name,
    >          }
    >      }
    >  
    > +    if (ip_in_data.size > 0) {
    > +        combine_ipv4_in_mask(&ip_in_data, &ip_out_data);
    > +        for (size_t i = 0; i < ip_out_data.used; ++i) {
    > +            union expr_constant *c = &cs->values[cs->n_values++];
    > +            memset(&c->value, 0, sizeof c->value);
    > +            memset(&c->mask, 0, sizeof c->mask);
    > +            c->value.ipv4 = htonl(ip_out_data.entries[i].ip);
    > +            c->format = LEX_F_IPV4;
    > +            c->masked = ip_out_data.entries[i].masked;
    > +            if (c->masked) {
    > +                c->mask.ipv4 = htonl(ip_out_data.entries[i].mask);
    > +            }
    > +        }
    > +        free(ip_out_data.entries);
    > +    }
    > +
    > +    free(ip_in_data.ip);
    >      shash_add(const_sets, name, cs);
    >  }
    >  
    > diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
    > index 6716dd0d2..1a0abff06 100644
    > --- a/rhel/ovn-fedora.spec.in
    > +++ b/rhel/ovn-fedora.spec.in
    > @@ -455,6 +455,7 @@ fi
    >  %{_bindir}/ovn-sbctl
    >  %{_bindir}/ovn-trace
    >  %{_bindir}/ovn-detrace
    > +%{_bindir}/ovn-match-ip
    >  %{_bindir}/ovn-appctl
    >  %{_bindir}/ovn-ic-nbctl
    >  %{_bindir}/ovn-ic-sbctl
    > diff --git a/tests/ovn.at b/tests/ovn.at
    > index b751d6db2..a243cd6f4 100644
    > --- a/tests/ovn.at
    > +++ b/tests/ovn.at
    > @@ -618,20 +618,32 @@ ip,nw_src=10.0.0.3
    >  ])
    >  AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl
    >  ip,nw_src=10.0.0.1
    > -ip,nw_src=10.0.0.2
    > -ip,nw_src=10.0.0.3
    > +ip,nw_src=10.0.0.2/31
    > +])
    > +AT_CHECK([expr_to_flow 'ip4.src == $set5'], [0], [dnl
    > +ip,nw_src=1.1.1.128/26
    > +ip,nw_src=1.1.1.16/28
    > +ip,nw_src=1.1.1.192/27
    > +ip,nw_src=1.1.1.2/31
    > +ip,nw_src=1.1.1.224/28
    > +ip,nw_src=1.1.1.240/29
    > +ip,nw_src=1.1.1.248/30
    > +ip,nw_src=1.1.1.252/31
    > +ip,nw_src=1.1.1.254
    > +ip,nw_src=1.1.1.32/27
    > +ip,nw_src=1.1.1.4/30
    > +ip,nw_src=1.1.1.64/26
    > +ip,nw_src=1.1.1.8/29
    >  ])
    >  AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0], [dnl
    >  ip,nw_src=1.2.3.4
    >  ip,nw_src=10.0.0.1
    > -ip,nw_src=10.0.0.2
    > -ip,nw_src=10.0.0.3
    > +ip,nw_src=10.0.0.2/31
    >  ])
    >  AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24, $set1}'], [0], [dnl
    >  ip,nw_src=1.2.0.0/20
    >  ip,nw_src=10.0.0.1
    > -ip,nw_src=10.0.0.2
    > -ip,nw_src=10.0.0.3
    > +ip,nw_src=10.0.0.2/31
    >  ip,nw_src=5.5.5.0/24
    >  ])
    >  AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0], [dnl
    > @@ -13768,23 +13780,20 @@ cat 2.expected > expout
    >  $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
    >  AT_CHECK([cat 2.packets], [0], [expout])
    >  
    > -# There should be total of 9 flows present with conjunction action and 2 flows
    > +# There should be total of 6 flows present with conjunction action and 2 flows
    >  # with conj match. Eg.
    > -# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45)
    > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(3,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(3,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2),conjunction(3,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2),conjunction(3,1/2)
    > -
    > -OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +#table=45, n_packets=1, n_bytes=42, idle_age=211, priority=2001,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
    > +#table=45, n_packets=0, n_bytes=0, idle_age=211, priority=2001,conj_id=3,ip,metadata=0x1 actions=drop
    > +#priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,2/2),conjunction(3,2/2)
    > +#priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(2,2/2),conjunction(3,2/2)
    > +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31 actions=conjunction(2,1/2)
    > +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,1/2)
    > +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,1/2)
    > +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31 actions=conjunction(3,1/2)
    > +
    > +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction | wc -l`])
    > -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction | wc -l`])
    >  OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conj_id | wc -l`])
    > @@ -13811,18 +13820,16 @@ AT_CHECK([cat 2.packets], [0], [])
    >  # properly.
    >  # There should be total of 6 flows present with conjunction action and 1 flow
    >  # with conj match. Eg.
    > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,1/2)
    > +# table=45, n_packets=1, n_bytes=42, idle_age=34, priority=2001,conj_id=3,ip,metadata=0x1 actions=drop
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(3,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31 actions=conjunction(3,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,1/2)
    >  
    >  ovn-nbctl acl-del ls1 to-lport 1001 \
    >  'ip4 && ip4.src == $set1 && ip4.dst == $set1'
    >  
    > -OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction | wc -l`])
    >  OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction | wc -l`])
    > @@ -13836,43 +13843,40 @@ ovn-nbctl acl-add ls1 to-lport 1001 \
    >  ovn-nbctl acl-add ls1 to-lport 1001 \
    >  'ip4 && ip4.src == $set1 && ip4.dst == {10.0.0.9, 10.0.0.10}' drop
    >  
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(5,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(5,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(5,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,1/2),conjunction(6,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(6,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
    > -
    > -OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31 actions=conjunction(3,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31 actions=conjunction(4,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(4,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(5,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(5,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(3,2/2),conjunction(4,2/2),conjunction(5,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,2/2),conjunction(4,2/2),conjunction(5,2/2)
    > +
    > +OVS_WAIT_UNTIL([test 8 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction | wc -l`])
    > -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction | wc -l`])
    > -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction.*conjunction | wc -l`])
    >  
    >  # Remove 10.0.0.7 from address set2. All flows should be updated properly.
    >  ovn-nbctl set Address_Set set2 \
    >  addresses=\"10.0.0.8\",\"10.0.0.9\"
    >  
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(9,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(7,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(8,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(9,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(7,1/2),conjunction(8,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(9,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
    > -
    > -OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31,nw_dst=10.0.0.8/31 actions=drop
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6,nw_dst=10.0.0.8/31 actions=drop
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31 actions=conjunction(4,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(4,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(5,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(5,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(5,2/2),conjunction(4,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(5,2/2),conjunction(4,2/2)
    > +
    > +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction | wc -l`])
    > -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction | wc -l`])
    > -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction.*conjunction | wc -l`])
    >  
    >  # Remove an ACL again
    > @@ -13881,16 +13885,16 @@ ovn-nbctl acl-del ls1 to-lport 1001 \
    >  
    >  wait_for_ports_up
    >  ovn-nbctl --wait=hv sync
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(10,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(11,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(10,1/2),conjunction(11,1/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(10,2/2),conjunction(11,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(10,2/2),conjunction(11,2/2)
    > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(10,2/2),conjunction(11,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6,nw_dst=10.0.0.8/31 actions=drop
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31,nw_dst=10.0.0.8/31 actions=drop
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(5,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(5,2/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(5,1/2)
    > +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(5,1/2)
    >  
    > -OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
    > -grep conjunction | wc -l`])
    >  OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
    > +grep conjunction | wc -l`])
    > +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction | wc -l`])
    >  OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
    >  grep conjunction.*conjunction.*conjunction | wc -l`])
    > @@ -15549,11 +15553,11 @@ for i in 1 2 3; do
    >  
    >      # Update address set as1
    >      ovn-nbctl --wait=hv set addr as1 addresses="10.1.2.10 10.1.2.11"
    > -    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.11"], [0], [ignore])
    > +    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.10/31"], [0], [ignore])
    >  
    >      # Update address set as2
    >      ovn-nbctl --wait=hv set addr as2 addresses="10.1.2.12 10.1.2.13"
    > -    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [0], [ignore])
    > +    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12/31"], [0], [ignore])
    >  
    >      # Add another ACL referencing as1
    >      n_flows_before=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l`
    > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
    > index 202a96c5d..4c13bacda 100644
    > --- a/tests/test-ovn.c
    > +++ b/tests/test-ovn.c
    > @@ -227,10 +227,19 @@ create_addr_sets(struct shash *addr_sets)
    >      };
    >      static const char *const addrs4[] = { NULL };
    >  
    > +    static const char *addrs5[253];
    > +    static char addrs5_[253][20];
    > +    for (int i = 0, ip_num = 2; i < 253; i++, ip_num++) {
    > +        sprintf(addrs5_[i], "1.1.1.%d", ip_num);
    > +        addrs5[i] = addrs5_[i];
    > +    }
    > +
    >      expr_const_sets_add(addr_sets, "set1", addrs1, 3, true);
    >      expr_const_sets_add(addr_sets, "set2", addrs2, 3, true);
    >      expr_const_sets_add(addr_sets, "set3", addrs3, 3, true);
    >      expr_const_sets_add(addr_sets, "set4", addrs4, 0, true);
    > +    expr_const_sets_add(addr_sets, "set5", (const char *const *)addrs5,
    > +                        253, true);
    >  }
    >  
    >  static void
    > diff --git a/utilities/automake.mk b/utilities/automake.mk
    > index c4a6d248c..af572c7a0 100644
    > --- a/utilities/automake.mk
    > +++ b/utilities/automake.mk
    > @@ -21,7 +21,8 @@ MAN_ROOTS += \
    >  bin_SCRIPTS += \
    >      utilities/ovn-docker-overlay-driver \
    >      utilities/ovn-docker-underlay-driver \
    > -    utilities/ovn-detrace
    > +    utilities/ovn-detrace \
    > +    utilities/ovn-match-ip
    >  
    >  EXTRA_DIST += \
    >      utilities/ovn-ctl \
    > @@ -35,6 +36,7 @@ EXTRA_DIST += \
    >      utilities/ovn-appctl.8.xml \
    >      utilities/ovn-trace.8.xml \
    >      utilities/ovn-detrace.in \
    > +    utilities/ovn-match-ip.in \
    >      utilities/ovndb-servers.ocf \
    >      utilities/checkpatch.py \
    >      utilities/docker/Makefile \
    > @@ -60,6 +62,7 @@ CLEANFILES += \
    >      utilities/ovn-trace.8 \
    >      utilities/ovn-detrace.1 \
    >      utilities/ovn-detrace \
    > +    utilities/ovn-match-ip \
    >      utilities/ovn-appctl.8 \
    >      utilities/ovn-appctl \
    >      utilities/ovn-sim
    > diff --git a/utilities/ovn-match-ip.in b/utilities/ovn-match-ip.in
    > new file mode 100755
    > index 000000000..af8124185
    > --- /dev/null
    > +++ b/utilities/ovn-match-ip.in
    > @@ -0,0 +1,78 @@
    > +#!/bin/bash
    > +#
    > +# Licensed under the Apache License, Version 2.0 (the "License");
    > +# you may not use this file except in compliance with the License.
    > +# You may obtain a copy of the License at:
    > +#
    > +#     http://www.apache.org/licenses/LICENSE-2.0
    > +#
    > +# Unless required by applicable law or agreed to in writing, software
    > +# distributed under the License is distributed on an "AS IS" BASIS,
    > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    > +# See the License for the specific language governing permissions and
    > +# limitations under the License.
    > +
    > +IPCALC_WILDCARD_RET=
    > +
    > +ipcalc_wildcard () {
    > +    ip=`echo $1 | awk -F '/' '{print $1}'`
    > +    ip_list=${ip//./ }
    > +    read -a ip_array <<< ${ip_list}
    > +    mask=`echo $1 | awk -F '/' '{print $2}'`
    > +    mask_list=${mask//./ }
    > +    read -a mask_array <<< ${mask_list}
    > +    ip_0=$((${ip_array[0]} & ${mask_array[0]}))
    > +    ip_1=$((${ip_array[1]} & ${mask_array[1]}))
    > +    ip_2=$((${ip_array[2]} & ${mask_array[2]}))
    > +    ip_3=$((${ip_array[3]} & ${mask_array[3]}))
    > +    IPCALC_WILDCARD_RET="$ip_0.$ip_1.$ip_2.$ip_3"
    > +}
    > +
    > +for arg
    > +do
    > +    case $arg in
    > +        -h | --help)
    > +            cat <<EOF
    > +$0: A tool that can match ip wildcard mask similar to grep
    > +usage: $0 ip
    > +Search for ip/mask that can be matched in standard input.
    > +Example: echo ip 192.168.1.0/24 | $0 192.168.1.1
    > +Result: ip 192.168.1.0/24
    > +
    > +You might use this: ovs-ofctl dump-flows br-int | $0 192.168.1.1
    > +EOF
    > +            exit 0
    > +            ;;
    > +    esac
    > +done
    > +
    > +while read line
    > +do
    > +    ip=`echo $line | grep -oE '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+'`
    > +    if test "$ip" == "$1"; then
    > +        echo $line | grep --color $1
    > +        continue
    > +    fi
    > +
    > +    ip_mask=`echo $line | grep -oE '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+'`
    > +    if test "$ip_mask" != ""; then
    > +        ipcalc_wildcard $ip_mask
    > +        ip_mask_ret=$IPCALC_WILDCARD_RET
    > +        match_ip_mask=`echo $ip_mask | sed 's/.*\//'$1'\//'`
    > +        ipcalc_wildcard $match_ip_mask
    > +        match_ip_mask_ret=$IPCALC_WILDCARD_RET
    > +        if test $ip_mask_ret == $match_ip_mask_ret; then
    > +            echo $line | grep --color $ip_mask
    > +        fi
    > +        continue
    > +    fi
    > +
    > +    ip_prefix=`echo $line | grep -oE '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+'`
    > +    if test "$ip_prefix" != ""; then
    > +        match_ip_prefix=`echo $ip_prefix | sed 's/.*\//'$1'\//'`
    > +        if test `ipcalc -n $match_ip_prefix` == `ipcalc -n $ip_prefix`; then
    > +            echo $line | grep --color $ip_prefix
    > +        fi
    > +        continue
    > +    fi
    > +done
    > 

    Regards,
    Dumitru





More information about the dev mailing list