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

Mark Gray mark.d.gray at redhat.com
Thu Mar 25 15:30:09 UTC 2021


On 19/03/2021 13:01, Dumitru Ceara wrote:
> On 3/10/21 2:29 PM, gmingchen(陈供明) wrote:
>> From: Gongming Chen <gmingchen at tencent.com>
>>

Thanks for the patch. Looks like a lot of thought went into the
algorithm and it has been interesting to review.

Do you know if there are any well-known algorithms to do this? It seems
like a common problem? If there was, it may be better to use it as we
could reference standard documentation.

>> 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'd like to understand more about the benefits of this change, what
problem is it solving? I agree with Dumitru that it will make debugging
quite a bit more difficult (even with the new tool) so it would be good
to understand the tradeoff?
> 
> I'm curious about your and the maintainers' opinion on these points.
> 
> 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.
> 
> 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.
> 
>> +
>> +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.
> 
>> +
>> +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.
> 
>> +{
>> +    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.
> 
>> +
>> +/* 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?

I would prefer that with some refactoring of the code below and some
in-line comments, we could make the documentation unnecessary and remove
or reduce it.
> 
>> +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.
> 
>> +    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.
> 
>> +
>> +    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".
> 
>> +          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;
>> +            }

At a first glance, I don't know what "recording" is. Maybe it needs
another name ("combining" or even "new_segment" as you have in the
documentation?) or some comments.
> 
> 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.
> 
>> +        }
>> +        /* 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];

Maybe this ^ could be a macro as it would help explain why you are
XORing these two IP addresses.

>> +            /* Only one bit different. */
>> +            if ((connect & (connect - 1)) == 0) {

Maybe you could use 'count_1bits()' as it would make it clearer

How about something like:

#define COUNT_BITS_DIFFERENT (uint32_t a, uint32_t b) count_1bits((a) ^ (b))

and
if (COUNT_BITS_DIFFERENT(...) == 1)

What do you think?

>> +                recording = true;
>> +                start = ip_in_data->ip[i];
>> +                continuous_size = 2;
>> +                mask_base = connect;
>> +
>> +                int j = 0;
>> +                end = start;

Maybe the name could be changed to 'max' or something like that as it is
not the actual end but the maximum possible ip in the segment?

>> +                /* The first non-zero place in the high direction is
>> +                 * the end of the segment. *
Maybe bitwise_scan() could be used to scan for the bit that it is set?

>> +                while (j < 32 && ((start & (mask_base << j)) == 0)) {
>> +                    end |= (mask_base << j);
>> +                    j++;
>> +                }

I think this could use a comment to explain why this is the case. I kind
of get it but it took me a while and I am still not 100% sure. As I
mentioned elsewhere, I think this is really looking for the maximum
potential IP address rather than what will actually be the the end value.

>> +
>> +                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;
>> +            }

Why would they be equal? And if they were, should we not check this in
the non-recording case above as well?

>> +            /* 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);

Maybe bitwise_one() could be used to build the mask.

As a general comment, it can take some time to reason what bit
manipulation code is actually doing. I think it would be good to use
some of the OVS/OVN bit manipulation helper functions anywhere that is
appropriate in this code. This will improve the readability.

>> +                    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));

Could you move 'ip_out_data->used++' to here and change the index above
^ to 'ip_out_data->used'?

>> +                }
>> +            } else {
>> +                continuous_size++;

Maybe the code in the 'if' statement could be simplified by calculating
the 'pow_base' and 'segment_power' here at each iteration? I'm not sure
but I suspect it might at least remove the 'while(continuous_size)' loop?

>> +            }
>> +
>> +            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 {

Seems like it would also need an IPv6 version.

Thinking about this a bit more, why does this need to be specific to
IPv4 - surely there is a similar problem across all masked integers?

>>                  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
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 




More information about the dev mailing list