On 26 Feb 2024, at 22:16, Mike Pattrick wrote:
> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> Signed-off-by: Mike Pattrick <m...@redhat.com>
Hi Mike,
Thanks for the patch revision. Some small comments below.
Cheers,
Eelco
> ---
> v7:
> - Make sure filter mask is added to masks of non-matching flows.
> - Added additional tests.
> ---
> Documentation/ref/ovs-tcpdump.8.rst | 8 +-
> NEWS | 3 +
> lib/flow.c | 21 +++-
> lib/flow.h | 12 +++
> ofproto/ofproto-dpif-mirror.c | 78 ++++++++++++++-
> ofproto/ofproto-dpif-mirror.h | 12 ++-
> ofproto/ofproto-dpif-xlate.c | 26 ++++-
> ofproto/ofproto-dpif.c | 9 +-
> ofproto/ofproto-dpif.h | 6 ++
> ofproto/ofproto.c | 4 +-
> ofproto/ofproto.h | 3 +
> tests/ofproto-dpif.at | 142 ++++++++++++++++++++++++++++
> utilities/ovs-tcpdump.in | 13 ++-
> vswitchd/bridge.c | 13 ++-
> vswitchd/vswitch.ovsschema | 5 +-
> vswitchd/vswitch.xml | 13 +++
> 16 files changed, 343 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/ref/ovs-tcpdump.8.rst
> b/Documentation/ref/ovs-tcpdump.8.rst
> index b9f8cdf6f..e21e61211 100644
> --- a/Documentation/ref/ovs-tcpdump.8.rst
> +++ b/Documentation/ref/ovs-tcpdump.8.rst
> @@ -61,8 +61,14 @@ Options
>
> If specified, mirror all ports (optional).
>
> +* ``--filter <flow>``
> +
> + If specified, only mirror flows that match the provided OpenFlow filter.
> + The available fields are documented in ``ovs-fields(7)``.
> +
> See Also
> ========
>
> ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
> -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
> +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
> +``wireshark(8)``.
> diff --git a/NEWS b/NEWS
> index c9e4064e6..35f7eb0c7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ Post-v3.3.0
> * Conntrack now supports 'random' flag for selecting ports in a range
> while natting and 'persistent' flag for selection of the IP address
> from a range.
> + - OVSDB:
> + * Added a new filter column in the Mirror table which can be used to
> + apply filters to mirror ports.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/lib/flow.c b/lib/flow.c
> index 8e3402388..a088bdc86 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3569,7 +3569,7 @@ miniflow_equal_in_minimask(const struct miniflow *a,
> const struct miniflow *b,
> return true;
> }
>
> -/* Returns true if 'a' and 'b' are equal at the places where there are 1-bits
> +/* Returns true if 'a' and 'b' are equal at the places where there are 0-bits
I think the logic is that only the differences on the masked data will return
false. So the original text is right.
> * in 'mask', false if they differ. */
> bool
> miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow
> *b,
> @@ -3587,6 +3587,25 @@ miniflow_equal_flow_in_minimask(const struct miniflow
> *a, const struct flow *b,
> return true;
> }
>
> +/* Returns false if 'a' and 'b' differ in places where there are 1-bits in
> + * 'wc', true otherwise. */
Maybe wright this the same as above to avoid confusion, i.e.
Returns true if 'a' and 'b' are equal at the places where there are 1-bits in
‘wc’, false if they differ [at the places where there are 1-bits in ‘wc’].
> +bool
> +miniflow_equal_flow_in_flow_wc(const struct miniflow *a, const struct flow
> *b,
> + const struct flow_wildcards *wc)
> +{
> + const struct flow *wc_masks = &wc->masks;
> + size_t idx;
> +
> + FLOWMAP_FOR_EACH_INDEX (idx, a->map) {
> + if ((miniflow_get(a, idx) ^ flow_u64_value(b, idx)) &
> + flow_u64_value(wc_masks, idx)) {
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
>
> void
> minimask_init(struct minimask *mask, const struct flow_wildcards *wc)
> diff --git a/lib/flow.h b/lib/flow.h
> index 75a9be3c1..a644be39d 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -748,6 +748,9 @@ bool miniflow_equal_in_minimask(const struct miniflow *a,
> bool miniflow_equal_flow_in_minimask(const struct miniflow *a,
> const struct flow *b,
> const struct minimask *);
> +bool miniflow_equal_flow_in_flow_wc(const struct miniflow *a,
> + const struct flow *b,
> + const struct flow_wildcards *);
> uint32_t miniflow_hash_5tuple(const struct miniflow *flow, uint32_t basis);
>
>
> @@ -939,6 +942,15 @@ flow_union_with_miniflow(struct flow *dst, const struct
> miniflow *src)
> flow_union_with_miniflow_subset(dst, src, src->map);
> }
>
> +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent
> + * fields in 'dst', storing the result in 'dst'. */
> +static inline void
> +flow_wildcards_union_with_minimask(struct flow_wildcards *dst,
> + const struct minimask *src)
> +{
> + flow_union_with_miniflow_subset(&dst->masks, &src->masks,
> src->masks.map);
> +}
> +
> static inline bool is_ct_valid(const struct flow *flow,
> const struct flow_wildcards *mask,
> struct flow_wildcards *wc)
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index a84c843b3..38e8d1fca 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -21,6 +21,7 @@
> #include "cmap.h"
> #include "hmapx.h"
> #include "ofproto.h"
> +#include "ofproto-dpif-trace.h"
> #include "vlan-bitmap.h"
> #include "openvswitch/vlog.h"
>
> @@ -50,6 +51,7 @@ struct mbundle {
>
> struct mirror {
> struct mbridge *mbridge; /* Owning ofproto. */
> + uint64_t change_seq; /* Change sequence of owning ofproto. */
> size_t idx; /* In ofproto's "mirrors" array. */
> void *aux; /* Key supplied by ofproto's client. */
>
> @@ -57,6 +59,11 @@ struct mirror {
> struct hmapx srcs; /* Contains "struct mbundle*"s. */
> struct hmapx dsts; /* Contains "struct mbundle*"s. */
>
> + /* Filter criteria. */
> + OVSRCU_TYPE(struct miniflow *) filter;
> + OVSRCU_TYPE(struct minimask *) mask;
> + char *filter_str;
> +
> /* This is accessed by handler threads assuming RCU protection (see
> * mirror_get()), but can be manipulated by mirror_set() without any
> * explicit synchronization. */
> @@ -207,8 +214,8 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
> ofbundle *ofbundle)
> }
>
> int
> -mirror_set(struct mbridge *mbridge, void *aux,
> - const struct ofproto_mirror_settings *ms,
> +mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto,
> + void *aux, const struct ofproto_mirror_settings *ms,
> const struct mirror_bundles *mb)
>
> {
> @@ -265,7 +272,11 @@ mirror_set(struct mbridge *mbridge, void *aux,
> && vlan_bitmap_equal(vlans, ms->src_vlans)
> && mirror->out == out
> && mirror->out_vlan == out_vlan
> - && mirror->snaplen == ms->snaplen)
> + && mirror->snaplen == ms->snaplen
> + && nullable_string_is_equal(mirror->filter_str, ms->filter)
> + /* If there is a filter, the datapath ports may have changed which
> + * could affect how the filter performs. Check change_seq if . */
Was some comment text deleted?
> + && (!ms->filter || ofproto->change_seq == mirror->change_seq))
> {
> hmapx_destroy(&srcs_map);
> hmapx_destroy(&dsts_map);
> @@ -289,6 +300,50 @@ mirror_set(struct mbridge *mbridge, void *aux,
> mirror->out_vlan = out_vlan;
> mirror->snaplen = ms->snaplen;
>
> + if (!nullable_string_is_equal(mirror->filter_str, ms->filter)) {
Wasn’t the goal to also re-configure in the case of a seq change?
> + if (mirror->filter_str) {
> + ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> + &mirror->filter));
> + ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> + &mirror->mask));
> + free(mirror->filter_str);
> + mirror->filter_str = NULL;
> + ovsrcu_set(&mirror->filter, NULL);
> + ovsrcu_set(&mirror->mask, NULL);
> + }
> +
> + if (ms->filter && strlen(ms->filter)) {
> + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map);
> + struct flow_wildcards wc;
> + struct flow flow;
> + char *err;
> +
> + ofproto_append_ports_to_map(&map, ofproto->ports);
> + err = parse_ofp_exact_flow(&flow, &wc,
> + ofproto_get_tun_tab(ofproto),
> + ms->filter, &map);
> + ofputil_port_map_destroy(&map);
> + if (err) {
> + VLOG_WARN("filter is invalid: %s", err);
> + free(err);
> + mirror_destroy(mbridge, mirror->aux);
> + return EINVAL;
> + }
> +
> + /* Do not allow a filter on in_port. */
> + if (wc.masks.in_port.ofp_port) {
> + VLOG_WARN("filter is invalid due to in_port field.");
> + mirror_destroy(mbridge, mirror->aux);
> + return EINVAL;
> + }
> +
> + mirror->filter_str = xstrdup(ms->filter);
> + ovsrcu_set(&mirror->mask, minimask_create(&wc));
> + ovsrcu_set(&mirror->filter, miniflow_create(&flow));
> + mirror->change_seq = ofproto->change_seq;
> + }
> + }
> +
> /* Update mbundles. */
> mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
> CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) {
> @@ -344,6 +399,17 @@ mirror_destroy(struct mbridge *mbridge, void *aux)
> ovsrcu_postpone(free, vlans);
> }
>
> + if (mirror->filter_str) {
> + ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> + &mirror->filter));
> + ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> + &mirror->mask));
> + free(mirror->filter_str);
> + mirror->filter_str = NULL;
> + ovsrcu_set(&mirror->filter, NULL);
> + ovsrcu_set(&mirror->mask, NULL);
> + }
> +
> mbridge->mirrors[mirror->idx] = NULL;
> /* mirror_get() might have just read the pointer, so we must postpone the
> * free. */
> @@ -415,7 +481,9 @@ mirror_update_stats(struct mbridge *mbridge,
> mirror_mask_t mirrors,
> * in which a 1-bit indicates that the mirror includes a particular VLAN,
> * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates
> * mirror 'index', 'mc->out' receives the output ofbundle (if any),
> - * and 'mc->out_vlan' receives the output VLAN (if any).
> + * and 'mc->out_vlan' receives the output VLAN (if any). In cases where the
> + * mirror has a filter configured 'mc->filter_flow' and 'mc->filter_mask'
> + * receives the flow and mask that this mirror should collect.
> *
> * Everything returned here is assumed to be RCU protected.
> */
> @@ -441,6 +509,8 @@ mirror_get(struct mbridge *mbridge, int index,
> mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL;
> mc->out_vlan = mirror->out_vlan;
> mc->snaplen = mirror->snaplen;
> + mc->filter_flow = ovsrcu_get(struct miniflow *, &mirror->filter);
> + mc->filter_mask = ovsrcu_get(struct minimask *, &mirror->mask);
> return true;
> }
>
> diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
> index a983b3876..7d40f7ec2 100644
> --- a/ofproto/ofproto-dpif-mirror.h
> +++ b/ofproto/ofproto-dpif-mirror.h
> @@ -23,8 +23,8 @@
> typedef uint32_t mirror_mask_t;
>
> struct ofproto_mirror_settings;
> -struct ofproto_dpif;
> struct ofbundle;
> +struct ofproto;
>
> struct mirror_bundles {
> struct ofbundle **srcs;
> @@ -43,6 +43,11 @@ struct mirror_config {
> /* VLANs of packets to select for mirroring. */
> unsigned long *vlans; /* vlan_bitmap, NULL selects all VLANs.
> */
>
> + /* The flow if a filter is used, or NULL. */
> + struct miniflow *filter_flow;
> + /* The filter's flow mask, or NULL. */
> + struct minimask *filter_mask;
> +
> /* Output (mutually exclusive). */
> struct ofbundle *out_bundle; /* A registered ofbundle handle or NULL.
> */
> uint16_t out_vlan; /* Output VLAN, not used if out_bundle is
> @@ -53,7 +58,6 @@ struct mirror_config {
> uint16_t snaplen;
> };
>
> -
I think you need to fix this in patch 1.
> /* The following functions are used by handler threads without any locking,
> * assuming RCU protection. */
>
> @@ -78,8 +82,8 @@ bool mbridge_need_revalidate(struct mbridge *);
> void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
> void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
>
> -int mirror_set(struct mbridge *mbridge, void *aux,
> - const struct ofproto_mirror_settings *ms,
> +int mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto,
> + void *aux, const struct ofproto_mirror_settings *ms,
> const struct mirror_bundles *mb);
> void mirror_destroy(struct mbridge *, void *aux);
> int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index aa2a514f9..58d89b04e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2250,7 +2250,8 @@ lookup_input_bundle(const struct xlate_ctx *ctx,
>
> /* Mirrors the packet represented by 'ctx' to appropriate mirror
> destinations,
> * given the packet is ingressing or egressing on 'xbundle', which has
> ingress
> - * or egress (as appropriate) mirrors 'mirrors'. */
> + * or egress (as appropriate) mirrors 'mirrors'. In cases where a mirror is
> + * filtered, the current flows wildcard will be modified. */
> static void
> mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> mirror_mask_t mirrors)
> @@ -2302,6 +2303,29 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> *xbundle,
> continue;
> }
>
> + /* After the VLAN check, apply a flow mask if a filter is specified.
> */
> + if (ctx->wc && mc.filter_flow) {
> + if (OVS_UNLIKELY(
> + miniflow_equal_flow_in_minimask(mc.filter_flow,
> + &ctx->xin->flow,
> + mc.filter_mask))) {
> + flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
> + } else if
> (!miniflow_equal_flow_in_minimask(&mc.filter_mask->masks,
> + &ctx->wc->masks,
> + mc.filter_mask)) {
> + /* If flow isn't in the mirror filter but flow's wildcard
> mask
> + * doesn't overlap with the filter mask then add the filter
> + * mask to the flow's wildcard mask. This prevents mirrored
> + * traffic from matching on this flow. */
> + flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
> + mirrors = zero_rightmost_1bit(mirrors);
> + continue;
> + } else {
> + mirrors = zero_rightmost_1bit(mirrors);
> + continue;
> + }
> + }
> +
> /* We sent a packet to this mirror. */
> used_mirrors |= rightmost_1bit(mirrors);
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4f31f5ebb..85fe6c10f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -222,13 +222,6 @@ static void ct_zone_config_uninit(struct dpif_backer
> *backer);
> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> static void ct_zone_limits_commit(struct dpif_backer *backer);
>
> -static inline struct ofproto_dpif *
> -ofproto_dpif_cast(const struct ofproto *ofproto)
> -{
> - ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> - return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> -}
> -
> /* Global variables. */
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> @@ -3669,7 +3662,7 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
> mb.n_dsts = s->n_dsts;
> mb.out_bundle = bundle_lookup(ofproto, s->out_bundle);
>
> - error = mirror_set(ofproto->mbridge, aux, s, &mb);
> + error = mirror_set(ofproto->mbridge, ofproto_, aux, s, &mb);
> free(mb.srcs);
> free(mb.dsts);
> return error;
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 92d33aa64..b95682595 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -363,6 +363,12 @@ struct ofproto_dpif {
>
> struct ofproto_dpif *ofproto_dpif_lookup_by_name(const char *name);
> struct ofproto_dpif *ofproto_dpif_lookup_by_uuid(const struct uuid *uuid);
> +static inline struct ofproto_dpif *
> +ofproto_dpif_cast(const struct ofproto *ofproto)
> +{
> + ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> + return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> +}
>
> ovs_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..06624006a 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6389,7 +6389,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct
> ofputil_flow_mod *fm,
> error = ofproto_flow_mod_start(ofproto, &ofm);
> if (!error) {
> ofproto_bump_tables_version(ofproto);
> - error = ofproto_flow_mod_finish(ofproto, &ofm, req);
> + error = ofproto_flow_mod_finish(ofproto, &ofm, req);
> ofmonitor_flush(ofproto->connmgr);
> }
> ovs_mutex_unlock(&ofproto_mutex);
> @@ -8460,7 +8460,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id,
> uint16_t flags)
> /* Send error referring to the original message. */
> ofconn_send_error(ofconn, be->msg, error);
> error = OFPERR_OFPBFC_MSG_FAILED;
> -
> +
> /* 2. Revert. Undo all the changes made above. */
> LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
> if (be->type == OFPTYPE_FLOW_MOD) {
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 1c07df275..655caa14e 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -501,6 +501,9 @@ struct ofproto_mirror_settings {
> uint16_t out_vlan; /* Output VLAN, only if out_bundle is NULL.
> */
> uint16_t snaplen; /* Max packet size of a mirrored packet
> in byte, set to 0 equals 65535. */
> +
> + /* Output filter. */
> + char *filter;
> };
>
> int ofproto_mirror_register(struct ofproto *, void *aux,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index daeea7775..6c5bf048a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5182,6 +5182,148 @@ OVS_VSWITCHD_STOP
> AT_CLEANUP
>
>
> +AT_SETUP([ofproto-dpif - mirroring, filter])
> +AT_KEYWORDS([mirror mirrors mirroring])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +AT_CHECK([ovs-vsctl \
> + set Bridge br0 mirrors=@m -- \
> + --id=@p3 get Port p3 -- \
> + --id=@m create Mirror name=mymirror select_all=true output_port=@p3
> filter="icmp"], [0], [ignore])
> +
My OCD probably wants some more aligned indentation, but I see both are used
throughout the various test cases.
AT_CHECK([ovs-vsctl \
set Bridge br0 mirrors=@m -- \
--id=@p3 get Port p3 -- \
--id=@m create Mirror name=mymirror select_all=true output_port=@p3
filter="icmp"],
[0], [ignore])
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=output:2
> +in_port=2 actions=output:1
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +icmp_flow="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> +tcp_flow1="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp(dst=443)"
> +tcp_flow2="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp(dst=80)"
> +
> +dnl Add mirrored flow after non-mirrored flow.
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1])
> +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p2$"], [0],
> [1
> +])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow])
> +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p3,p2$"],
> [0], [1
> +])
> +
> +dnl Check one direction, only icmp should mirror.
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +dnl Check other direction, only icmp should mirror.
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,1
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 1
> +])
> +
> +dnl Change filter to tcp, only tcp should mirror.
> +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 1
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,1
> +])
> +
> +dnl Invalid filter. Nothing should mirror, error should be logged.
> +AT_CHECK([ovs-vsctl set mirror mymirror filter="invalid"], [0])
> +dnl Setting an in_port is also invalid.
> +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"in_port=p1\""], [0])
> +
> +OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror
> configuration is invalid" ovs-vswitchd.log) -eq 4])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 1
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 1
> +])
> +
> +dnl Check more complex filter cases with partially overlapping default
> wildcards.
> +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"tcp,tcp_dst=80\""], [0])
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +dnl Change port number.
> +AT_CHECK([ovs-appctl dpif-dummy/change-port-number ovs-dummy p1 8])
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow2"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,2
> +])
> +
> +dnl Empty filter, all traffic should mirror.
> +AT_CHECK([ovs-vsctl clear mirror mymirror filter], [0])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,2
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,8
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> + [Datapath actions: 3,8
> +])
> +
> +OVS_VSWITCHD_STOP(["/filter is invalid: invalid: unknown field invalid/d
> +/filter is invalid due to in_port field/d
> +/mirror mymirror configuration is invalid/d"])
> +AT_CLEANUP
> +
> +
> AT_SETUP([ofproto-dpif - mirroring, select_all])
> AT_KEYWORDS([mirror mirrors mirroring])
> OVS_VSWITCHD_START
> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
> index 4cbd9a5d3..7483b52ac 100755
> --- a/utilities/ovs-tcpdump.in
> +++ b/utilities/ovs-tcpdump.in
> @@ -142,6 +142,7 @@ The following options are available:
> --mirror-to The name for the mirror port to use (optional)
> Default 'miINTERFACE'
> --span If specified, mirror all ports (optional)
> + --filter Set an OpenFlow formatted preselection filter
> """ % {'prog': sys.argv[0]})
> sys.exit(0)
>
> @@ -354,7 +355,7 @@ class OVSDB(object):
> return result
>
> def bridge_mirror(self, intf_name, mirror_intf_name, br_name,
> - mirror_select_all=False):
> + mirror_select_all=False, mirror_filter=None):
>
> txn = self._start_txn()
> mirror = txn.insert(self.get_table('Mirror'))
> @@ -362,6 +363,9 @@ class OVSDB(object):
>
> mirror.select_all = mirror_select_all
>
> + if mirror_filter is not None:
> + mirror.filter = mirror_filter
> +
> mirrored_port = self._find_row_by_name('Port', intf_name)
>
> mirror.verify('select_dst_port')
> @@ -445,6 +449,7 @@ def main():
> mirror_interface = None
> mirror_select_all = False
> dump_cmd = 'tcpdump'
> + mirror_filter = None
>
> for cur, nxt in argv_tuples(sys.argv[1:]):
> if skip_next:
> @@ -474,6 +479,10 @@ def main():
> elif cur in ['--span']:
> mirror_select_all = True
> continue
> + elif cur in ['--filter']:
> + mirror_filter = nxt
> + skip_next = True
> + continue
> tcpdargs.append(cur)
>
> if interface is None:
> @@ -526,7 +535,7 @@ def main():
> ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
> ovsdb.bridge_mirror(interface, mirror_interface,
> ovsdb.port_bridge(interface),
> - mirror_select_all)
> + mirror_select_all, mirror_filter=mirror_filter)
> except OVSDBException as oe:
> print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
> sys.exit(1)
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 95a65fcdc..39f8fba49 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -5144,6 +5144,7 @@ mirror_configure(struct mirror *m)
> {
> const struct ovsrec_mirror *cfg = m->cfg;
> struct ofproto_mirror_settings s;
> + int ret;
>
> /* Set name. */
> if (strcmp(cfg->name, m->name)) {
> @@ -5212,8 +5213,18 @@ mirror_configure(struct mirror *m)
> /* Get VLAN selection. */
> s.src_vlans = vlan_bitmap_from_array(cfg->select_vlan,
> cfg->n_select_vlan);
>
> + /* Set the filter, mirror_set() will strdup this pointer. */
> + s.filter = cfg->filter;
> +
> /* Configure. */
> - ofproto_mirror_register(m->bridge->ofproto, m, &s);
> + ret = ofproto_mirror_register(m->bridge->ofproto, m, &s);
> + if (ret == EOPNOTSUPP) {
> + VLOG_ERR("ofproto %s: does not support mirroring",
> + m->bridge->ofproto->name);
> + } else if (ret) {
> + VLOG_ERR("bridge %s: mirror %s configuration is invalid",
> + m->bridge->name, m->name);
> + }
>
> /* Clean up. */
> if (s.srcs != s.dsts) {
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index e2d5e2e85..bdb2c8c0a 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
> {"name": "Open_vSwitch",
> "version": "8.5.0",
> - "cksum": "4040946650 27557",
> + "cksum": "2211155747 27661",
> "tables": {
> "Open_vSwitch": {
> "columns": {
> @@ -461,6 +461,9 @@
> "type": {"key": "string", "value": "integer",
> "min": 0, "max": "unlimited"},
> "ephemeral": true},
> + "filter": {
> + "type": {"key": {"type": "string"},
> + "min": 0, "max": 1}},
> "external_ids": {
> "type": {"key": "string", "value": "string",
> "min": 0, "max": "unlimited"}}}},
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 612ba41e3..ec536a08e 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -5256,6 +5256,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> VLANs on which packets are selected for mirroring. An empty set
> selects packets on all VLANs.
> </column>
> + <column name="filter">
> + <p>
> + Filter to apply to mirror. Flows that match <ref
> column="filter"/>
> + will flow through this mirror, flows that do not match will be
> + ignored. The <ref column="filter"/> syntax is described in
> + <code>ovs-fields</code>(7).
> + </p>
> + <p>
> + This filter is applied after <ref column="select_all"/>, <ref
> + column="select_dst_port"/>, <ref column="select_src_port"/>, and
> + <ref column="select_vlan"/>.
> + </p>
> + </column>
> </group>
>
> <group title="Mirroring Destination Configuration">
> --
> 2.39.3
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
