[ovs-dev] [PATCH ovn v5] Support packet metadata marking for logical router policies.

Dumitru Ceara dceara at redhat.com
Mon Jun 22 15:46:20 UTC 2020


On 6/22/20 1:11 PM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> This patch adds a new column 'options' of type smap in the
> Logical_Router_Policy table in the NB DB and supports the key 'pkt_mark'.
> CMS can set a desired value for this key in the 'options' column. When this
> router policy is applied, the packet metadata is marked with the specified
> value (to the NXM_NX_PKT_MARK OVS field).
> 
> In the case of Linux, this corresponds to struct sk_buff's "skb_mark"
> member and this mark can be seen by the linux networking subsystem.
> CMS can inspect this value (as an iptables rule or adding an OF flow
> in another ovs bridge) and take appropriate action when the marked packet
> leaves the integration bridge via the patch port.
> 
> Requested-at: https://bugzilla.redhat.com/show_bug.cgi?id=1828933
> Requested-by: Alexander Constantinescu <aconstan at redhat.com>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
> 
> v4 -> v5
> ---
>   * Forgot to check-in the tests/ovn.at file in v4.
> 
> v3 -> v4
> ---
>  * Addressed review comments from Dumitru.
> 
> v2 -> v3
> ---
>  * Added test cases for IPv6 routing policies as suggested by Alexander
>    Constantinescu.
> 
> v1 -> v2
> ---
>  * Addressed review comments from Gabriele Cerami.
>  * Documented the  field pkt.mark in ovn-sb.xml.
> 
>  NEWS                 |   2 +
>  lib/logical-fields.c |   2 +
>  northd/ovn-northd.c  |   8 ++
>  ovn-nb.ovsschema     |   7 +-
>  ovn-nb.xml           |   9 ++
>  ovn-sb.xml           |   1 +
>  tests/ovn.at         | 290 +++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 317 insertions(+), 2 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index c6bb9b2fb..893063b01 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,7 @@
>  Post-v20.06.0
>  --------------------------
> +   - Added packet marking support for traffic routed with
> +     a routing policy.
>  
>  OVN v20.06.0
>  --------------------------
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index a007085b3..8ad56aa53 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -254,6 +254,8 @@ ovn_init_symtab(struct shash *symtab)
>      expr_symtab_add_field(symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false);
>      expr_symtab_add_field(symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false);
>  
> +    expr_symtab_add_field(symtab, "pkt.mark", MFF_PKT_MARK, NULL, false);
> +
>      expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu", OVN_ICMP4_FRAG_MTU);
>  }
>  
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 40aeb0a58..d02447c73 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -7101,6 +7101,10 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>                           rule->priority, rule->nexthop);
>              return;
>          }
> +        uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0);
> +        if (pkt_mark) {
> +            ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
> +        }
>          bool is_ipv4 = strchr(rule->nexthop, '.') ? true : false;
>          ds_put_format(&actions, "%sreg0 = %s; "
>                        "%sreg1 = %s; "
> @@ -7118,6 +7122,10 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>      } else if (!strcmp(rule->action, "drop")) {
>          ds_put_cstr(&actions, "drop;");
>      } else if (!strcmp(rule->action, "allow")) {
> +        uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0);
> +        if (pkt_mark) {
> +            ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
> +        }
>          ds_put_cstr(&actions, "next;");
>      }
>      ds_put_format(&match, "%s", rule->match);
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index a06972aa0..da9af7157 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.23.0",
> -    "cksum": "111023208 25806",
> +    "version": "5.24.0",
> +    "cksum": "1092394564 25961",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -379,6 +379,9 @@
>                      "key": {"type": "string",
>                              "enum": ["set", ["allow", "drop", "reroute"]]}}},
>                  "nexthop": {"type": {"key": "string", "min": 0, "max": 1}},
> +                "options": {
> +                    "type": {"key": "string", "value": "string",
> +                             "min": 0, "max": "unlimited"}},
>                  "external_ids": {
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 025948791..2f610ee44 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2542,6 +2542,15 @@
>        </p>
>      </column>
>  
> +    <column name="options" key="pkt_mark">
> +      <p>
> +        Marks the packet with the value specified when the router policy
> +        is applied. CMS can inspect this packet marker and take some decisions
> +        if desired. This value is not preserved when the packet goes out on the
> +        wire.
> +      </p>
> +    </column>
> +
>      <group title="Common Columns">
>        <column name="external_ids">
>          See <em>External IDs</em> at the beginning of this document.
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 208fb936f..00e7e7fc5 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -975,6 +975,7 @@
>          <li><code>xxreg0</code> <code>xxreg1</code></li>
>          <li><code>inport</code> <code>outport</code></li>
>          <li><code>flags.loopback</code></li>
> +        <li><code>pkt.mark</code></li>
>          <li><code>eth.src</code> <code>eth.dst</code> <code>eth.type</code></li>
>          <li><code>vlan.tci</code> <code>vlan.vid</code> <code>vlan.pcp</code> <code>vlan.present</code></li>
>          <li><code>ip.proto</code> <code>ip.dscp</code> <code>ip.ecn</code> <code>ip.ttl</code> <code>ip.frag</code></li>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 8ee348397..b8d7a09a6 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -965,6 +965,23 @@ ip.ttl--;
>  ip.ttl
>      Syntax error at end of input expecting `--'.
>  
> +# Packet mark.
> +pkt.mark=1;
> +    formats as pkt.mark = 1;
> +    encodes as set_field:0x1->pkt_mark
> +
> +pkt.mark = 1000;
> +    encodes as set_field:0x3e8->pkt_mark
> +
> +pkt.mark;
> +    Syntax error at `pkt.mark' expecting action.
> +
> +pkt.mark = foo;
> +    Syntax error at `foo' expecting field name.
> +
> +pkt.mark = "foo";
> +    Integer field pkt.mark is not compatible with string constant.
> +
>  # load balancing.
>  ct_lb;
>      encodes as ct(table=19,zone=NXM_NX_REG13[0..15],nat)
> @@ -19905,3 +19922,276 @@ OVS_WAIT_UNTIL([test 0 = $(ovn-sbctl show | grep Port_Binding -c)], [0])
>  
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- Logical router policy packet marking])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> +    set interface hv1-vif1 external-ids:iface-id=sw0-port1 \
> +    options:tx_pcap=hv1/vif1-tx.pcap \
> +    options:rxq_pcap=hv1/vif1-rx.pcap \
> +    ofport-request=1
> +ovs-vsctl -- add-port br-int hv1-vif2 -- \
> +    set interface hv1-vif2 external-ids:iface-id=sw0-port2 \
> +    options:tx_pcap=hv1/vif2-tx.pcap \
> +    options:rxq_pcap=hv1/vif2-rx.pcap \
> +    ofport-request=2
> +
> +as hv1 ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=public:br-phys
> +
> +ovn-nbctl ls-add sw0
> +ovn-nbctl lsp-add sw0 sw0-port1
> +ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:03 10.0.0.3 10.0.0.5"
> +ovn-nbctl lsp-set-port-security sw0-port1 "50:54:00:00:00:03 10.0.0.3 10.0.0.5"
> +
> +ovn-nbctl lsp-add sw0 sw0-port2
> +ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:04 10.0.0.4 aef0::4"
> +
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 aef0::1/64
> +ovn-nbctl lsp-add sw0 sw0-lr0
> +ovn-nbctl lsp-set-type sw0-lr0 router
> +ovn-nbctl lsp-set-addresses sw0-lr0 router
> +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
> +
> +ovn-nbctl ls-add public
> +ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 bef0::1/64
> +ovn-nbctl lsp-add public public-lr0
> +ovn-nbctl lsp-set-type public-lr0 router
> +ovn-nbctl lsp-set-addresses public-lr0 router
> +ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> +
> +# localnet port
> +ovn-nbctl lsp-add public ln-public
> +ovn-nbctl lsp-set-type ln-public localnet
> +ovn-nbctl lsp-set-addresses ln-public unknown
> +ovn-nbctl lsp-set-options ln-public network_name=public
> +
> +ovn-nbctl lrp-set-gateway-chassis lr0-public hv1 20
> +ovn-nbctl lr-nat-add lr0 snat 172.168.0.100 10.0.0.0/24
> +lr0_dp_uuid=$(ovn-sbctl --bare --columns _uuid list datapath_binding lr0)
> +ovn-sbctl create mac_binding datapath=$lr0_dp_uuid ip=172.168.0.120 \
> +logical_port=lr0-public mac="10\:54\:00\:00\:00\:03"
> +ovn-sbctl create mac_binding datapath=$lr0_dp_uuid ip=172.168.0.200 \
> +logical_port=lr0-public mac="10\:54\:00\:00\:00\:04"
> +ovn-sbctl create mac_binding datapath=$lr0_dp_uuid ip="bef0\:\:4" \
> +logical_port=lr0-public mac="10\:54\:00\:00\:00\:05"
> +ovn-sbctl create mac_binding datapath=$lr0_dp_uuid ip="bef0\:\:5" \
> +logical_port=lr0-public mac="10\:54\:00\:00\:00\:06"
> +ovn-sbctl create mac_binding datapath=$lr0_dp_uuid ip="bef0\:\:6" \
> +logical_port=lr0-public mac="10\:54\:00\:00\:00\:07"
> +
> +ovn-nbctl -- --id=@lrt create Logical_Router_Static_Route \
> +ip_prefix="\:\:/64" nexthop="bef0\:\:4" -- add Logical_Router lr0 \
> +static_routes @lrt
> +
> +ovn-nbctl --wait=hv sync
> +
> +# Add logical router policy and set pkt_mark on it.
> +ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow
> +ovn-nbctl lr-policy-add lr0 1000 "ip4.src == 10.0.0.4" allow
> +ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute 172.168.0.200
> +ovn-nbctl lr-policy-add lr0 2001 "ip6.dst == bef0::5" reroute bef0::6
> +ovn-nbctl lr-policy-add lr0 1001 "ip6" allow
> +
> +
> +pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2000)
> +pol3=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=900)
> +pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2001)
> +pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1001)
> +
> +ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100
> +ovn-nbctl set logical_router_policy $pol3 options:pkt_mark=3
> +ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4
> +ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5
> +ovn-nbctl --wait=hv sync
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x64->NXM_NX_PKT_MARK" -c)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x3->NXM_NX_PKT_MARK" -c)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x4->NXM_NX_PKT_MARK" -c)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x5->NXM_NX_PKT_MARK" -c)
> +])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, priority=0 actions=NORMAL
> +table=0, priority=200 arp,actions=drop
> +table=0, priority=100, pkt_mark=0x64 actions=drop
> +table=0, priority=100, pkt_mark=0x2 actions=drop
> +table=0, priority=100, pkt_mark=0x3 actions=drop
> +table=0, priority=100, pkt_mark=0x4 actions=drop
> +table=0, priority=100, pkt_mark=0x5 actions=drop
> +])
> +

Hi Numan,

The test still fails occasionaly because here we might have already
received a GARP from ovn-controller. "add-flows" below will not reset
the packet counters.

What we can do is either:

a. del the flows here:
AT_CHECK([as hv1 ovs-ofctl del-flows br-phys])

b. add the br-phys flows in the beginning of the test, before
ovn-controller has the chance to send GARPs.

With this addressed:

Acked-by: Dumitru Ceara <dceara at redhat.com>

Regards,
Dumitru

> +AT_CHECK([as hv1 ovs-ofctl --protocols=OpenFlow13 add-flows br-phys flows.txt])
> +
> +ip_to_hex() {
> +     printf "%02x%02x%02x%02x" "$@"
> +}
> +
> +send_ipv4_pkt() {
> +    local hv=$1 inport=$2 eth_src=$3 eth_dst=$4
> +    local ip_src=$5 ip_dst=$6
> +    packet=${eth_dst}${eth_src}08004500001c0000000040110000${ip_src}${ip_dst}0035111100080000
> +    as $hv ovs-appctl netdev-dummy/receive ${inport} ${packet}
> +}
> +
> +send_icmp6_packet() {
> +    local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 ipv6_src=$5 ipv6_dst=$6
> +
> +    local ip6_hdr=6000000000083aff${ipv6_src}${ipv6_dst}
> +    local packet=${eth_dst}${eth_src}86dd${ip6_hdr}8000dcb662f00001
> +
> +    as $hv ovs-appctl netdev-dummy/receive ${inport} ${packet}
> +}
> +
> +send_ipv4_pkt hv1 hv1-vif1 505400000003 00000000ff01 \
> +    $(ip_to_hex 10 0 0 3) $(ip_to_hex 172 168 0 120)
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x64" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep priority=0 | \
> +    grep "n_packets=0" -c)
> +])
> +
> +# Send the pkt from sw0-port2. Packet should not be marked.
> +send_ipv4_pkt hv1 hv1-vif2 505400000004 00000000ff01 \
> +    $(ip_to_hex 10 0 0 4) $(ip_to_hex 172 168 0 120)
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep priority=0 | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x64" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x3" | \
> +    grep "n_packets=0" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x4" | \
> +    grep "n_packets=0" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x5" | \
> +    grep "n_packets=0" -c)
> +])
> +
> +ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=2
> +send_ipv4_pkt hv1 hv1-vif1 505400000003 00000000ff01 \
> +    $(ip_to_hex 10 0 0 3) $(ip_to_hex 172 168 0 120)
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x2->NXM_NX_PKT_MARK" -c)
> +])
> +
> +AT_CHECK([
> +    test 0 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x64->NXM_NX_PKT_MARK" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x2" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep priority=0 | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x3" | \
> +    grep "n_packets=0" -c)
> +])
> +
> +# Send with src ip 10.0.0.5. The reroute policy should be hit
> +# and the packet should be marked with 5.
> +send_ipv4_pkt hv1 hv1-vif1 505400000003 00000000ff01 \
> +    $(ip_to_hex 10 0 0 5) $(ip_to_hex 172 168 0 120)
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x3" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +# Send IPv6 traffic.
> +src_ip6=aef00000000000000000000000000004
> +dst_ip6=bef00000000000000000000000000004
> +
> +send_icmp6_packet hv1 hv1-vif2 505400000004 00000000ff01 ${src_ip6} ${dst_ip6}
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x5" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x4" | \
> +    grep "n_packets=0" -c)
> +])
> +
> +# Send IPv6 packet which hits the reroute policy. Packet should be marked
> +# with 4.
> +
> +src_ip6=aef00000000000000000000000000004
> +dst_ip6=bef00000000000000000000000000005
> +
> +send_icmp6_packet hv1 hv1-vif2 505400000004 00000000ff01 ${src_ip6} ${dst_ip6}
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x4" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +AT_CHECK([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep "priority=100,pkt_mark=0x5" | \
> +    grep "n_packets=1" -c)
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> 



More information about the dev mailing list