On 19/03/2021 13:01, Dumitru Ceara wrote: > On 3/10/21 2:29 PM, gmingchen(陈供明) wrote: >> From: Gongming Chen <[email protected]> >>
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 <[email protected]> >> --- > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
