diff mbox series

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

Message ID 20200617184153.1272773-1-numans@ovn.org
State Superseded
Headers show
Series [ovs-dev,ovn] Support packet metadata marking for logical router policies. | expand

Commit Message

Numan Siddique June 17, 2020, 6:41 p.m. UTC 
From: Numan Siddique <numans@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@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 NEWS                 |   2 +
 lib/logical-fields.c |   2 +
 northd/ovn-northd.c  |   8 +++
 ovn-nb.ovsschema     |   7 +-
 ovn-nb.xml           |   9 +++
 tests/ovn.at         | 167 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 193 insertions(+), 2 deletions(-)

Comments

Gabriele Cerami June 17, 2020, 9:51 p.m. UTC | #1 
Hi,

I added some note for my better understanding, but I have some questions
at the end on the testing part.
Sorry if they seem irrelevant or too picky, still learning.

On 18 Jun, numans@ovn.org wrote:
> +    expr_symtab_add_field(symtab, "pkt.mark", MFF_PKT_MARK, NULL, false);
> +

Note to self: this part maps the mnemonic string with the meta flow
field in OVS as described in include/openvswitch/meta-flow.h .

> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c

Note to self: the pkt.mark is enabled for both allow and reroute
policies

> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema

Note to self: even if a value is interpreted as integer in the schema, all values
are saved as strings (HEX compatibility ?)

> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9acb48ae5..f31ab9bbf 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -965,6 +965,22 @@ ip.ttl--;
>  ip.ttl
>      Syntax error at end of input expecting `--'.

Missing a section index here ? # 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)
> @@ -19898,3 +19914,154 @@ 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"
> +ovn-nbctl lsp-set-port-security sw0-port1 "50:54:00:00:00:03 10.0.0.3"

really nitpicking here .... 50:54:00 is a range reserved to cisco ....
local test should use a locally administered range :P

More seriously, is there a convention for the usage of differnt OUIs ?,
I see a mix of cisco, dataindustrier, not sure if different ranges have
different meanings in the tests.

> +
> +ovn-nbctl lsp-add sw0 sw0-port2
> +ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:04 10.0.0.4"
> +
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> +ovn-nbctl lsp-add sw0 sw0-lr0
> +ovn-nbctl lsp-set-type sw0-lr0 router
> +ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01

You are creating a router port with a mac, then creating a switch port
ion sw0 to connect to it, with the same mac.
Is there a reason for using the same mac ?
Maybe it doesn't really matter in OVS implementation, and the router
will probably never send a packet to the switch itself, but I would
imagine problems when a host sends a packet for the router and the
switch assumes it's for itself and doesn't forward it to other ports.

> +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
> +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-nbctl --wait=hv sync
> +
> +# Add logical router polcy and set pkt_mark on it.

nit: policy

> +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
> +
> +pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2000)
> +pol2=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1000)

I don't see pol2 used anywhere in the following steps. 10.0.0.4 sources
(to which pol2 would be attached to ) are used the verify the absence of
marks. And a different mark replaces the first in the same policy pol1
in later tests.

> +ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> +    grep "load:0x64->NXM_NX_PKT_MARK" -c)
> +])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, priority=0 actions=NORMAL
> +table=0, priority=100, pkt_mark=0x64 actions=drop
> +table=0, priority=100, pkt_mark=0x2 actions=drop
> +])
> +
> +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_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 | grep "priority=100,pkt_mark=0x64" | \

The regex on the first grep is contained in the regex for the second.
Do you need both for some reason ?

> +    grep "n_packets=1" -c)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    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)
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep priority=0 | \
> +    grep "n_packets=1" -c)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep priority=100 | grep "priority=100,pkt_mark=0x64" | \

Same for the grep here.

> +    grep "n_packets=1" -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)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    test 0 -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-phys table=0 | \
> +    grep priority=100 | grep "priority=100,pkt_mark=0x2" | \

Same for the grep here.

> +    grep "n_packets=1" -c)
> +])
> +
> +OVS_WAIT_UNTIL([
> +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> +    grep priority=0 | \
> +    grep "n_packets=1" -c)
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP

Is there any need to add a test for the reroute policy case ?
Or a test for seeing the pkt.mark option not present after removing the
policy/options:pkt_mark ?

> -- 
> 2.26.2
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Numan Siddique June 18, 2020, 9 a.m. UTC | #2 
Hi,

Thanks for the review comments.

Please see below for some comments.

Thanks
Numan


On Thu, Jun 18, 2020 at 3:22 AM Gabriele Cerami <gcerami@redhat.com> wrote:

> Hi,
>
> I added some note for my better understanding, but I have some questions
> at the end on the testing part.
> Sorry if they seem irrelevant or too picky, still learning.
>
> On 18 Jun, numans@ovn.org wrote:
> > +    expr_symtab_add_field(symtab, "pkt.mark", MFF_PKT_MARK, NULL,
> false);
> > +
>
> Note to self: this part maps the mnemonic string with the meta flow
> field in OVS as described in include/openvswitch/meta-flow.h .
>
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>
> Note to self: the pkt.mark is enabled for both allow and reroute
> policies
>
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
>
> Note to self: even if a value is interpreted as integer in the schema, all
> values
> are saved as strings (HEX compatibility ?)
>
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 9acb48ae5..f31ab9bbf 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -965,6 +965,22 @@ ip.ttl--;
> >  ip.ttl
> >      Syntax error at end of input expecting `--'.
>
> Missing a section index here ? # Packet mark
>

Ack.

>
> > +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)
> > @@ -19898,3 +19914,154 @@ 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"
> > +ovn-nbctl lsp-set-port-security sw0-port1 "50:54:00:00:00:03 10.0.0.3"
>
> really nitpicking here .... 50:54:00 is a range reserved to cisco ....
> local test should use a locally administered range :P
>

I don't know how it was chosen. I normally copy from the existing tests.


> More seriously, is there a convention for the usage of differnt OUIs ?,
>

Not sure.

> I see a mix of cisco, dataindustrier, not sure if different ranges have
> different meanings in the tests.
>


All the tests are run locally. Are there any serious ramifications of using
these different
OUIs ?

I just left the MACs AS IS now.



> > +
> > +ovn-nbctl lsp-add sw0 sw0-port2
> > +ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:04 10.0.0.4"
> > +
> > +ovn-nbctl lr-add lr0
> > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> > +ovn-nbctl lsp-add sw0 sw0-lr0
> > +ovn-nbctl lsp-set-type sw0-lr0 router
> > +ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
>
> You are creating a router port with a mac, then creating a switch port
> ion sw0 to connect to it, with the same mac.
> Is there a reason for using the same mac ?
> Maybe it doesn't really matter in OVS implementation, and the router
> will probably never send a packet to the switch itself, but I would
> imagine problems when a host sends a packet for the router and the
> switch assumes it's for itself and doesn't forward it to other ports.
>
>
In order to connect a logical switch to a logical router, we need to
create a logical switch port and logical router port and kind of attach
them.

Either the MAC has to be the same or we can do

ovn-nbctl lsp-set-addresses sw0-lr0 router.

Both works. But I think setting to router is preferable.



> +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
> > +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-nbctl --wait=hv sync
> > +
> > +# Add logical router polcy and set pkt_mark on it.
>
> nit: policy
>
> Ack. Done.



> > +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
> > +
> > +pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
> priority=2000)
> > +pol2=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
> priority=1000)
>
> I don't see pol2 used anywhere in the following steps. 10.0.0.4 sources
> (to which pol2 would be attached to ) are used the verify the absence of
> marks. And a different mark replaces the first in the same policy pol1
> in later tests.
>

That was intentional to not mark anything for 2nd policy. I'll just delete
this storing the uuid in pol2.


> > +ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100
> > +
> > +OVS_WAIT_UNTIL([
> > +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
> > +    grep "load:0x64->NXM_NX_PKT_MARK" -c)
> > +])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0, priority=0 actions=NORMAL
> > +table=0, priority=100, pkt_mark=0x64 actions=drop
> > +table=0, priority=100, pkt_mark=0x2 actions=drop
> > +])
> > +
> > +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_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 | grep "priority=100,pkt_mark=0x64" | \
>
> The regex on the first grep is contained in the regex for the second.
> Do you need both for some reason ?
>

I'll remove the redundant one in v2.



> > +    grep "n_packets=1" -c)
> > +])
> > +
> > +OVS_WAIT_UNTIL([
> > +    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)
> > +
> > +OVS_WAIT_UNTIL([
> > +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> > +    grep priority=0 | \
> > +    grep "n_packets=1" -c)
> > +])
> > +
> > +OVS_WAIT_UNTIL([
> > +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> > +    grep priority=100 | grep "priority=100,pkt_mark=0x64" | \
>
> Same for the grep here.
>
> > +    grep "n_packets=1" -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)
> > +])
> > +
> > +OVS_WAIT_UNTIL([
> > +    test 0 -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-phys table=0 | \
> > +    grep priority=100 | grep "priority=100,pkt_mark=0x2" | \
>
> Same for the grep here.
>
> > +    grep "n_packets=1" -c)
> > +])
> > +
> > +OVS_WAIT_UNTIL([
> > +    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> > +    grep priority=0 | \
> > +    grep "n_packets=1" -c)
> > +])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
>
> Is there any need to add a test for the reroute policy case ?
> Or a test for seeing the pkt.mark option not present after removing the
> policy/options:pkt_mark ?
>

I'll add a test for this in v2.

Thanks
Numan


> > --
> > 2.26.2
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
diff mbox series

Patch

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 c788d924b..97a287b84 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 8368d5108..21fe5e773 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2541,6 +2541,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 of 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/tests/ovn.at b/tests/ovn.at
index 9acb48ae5..f31ab9bbf 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -965,6 +965,22 @@  ip.ttl--;
 ip.ttl
     Syntax error at end of input expecting `--'.
 
+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)
@@ -19898,3 +19914,154 @@  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"
+ovn-nbctl lsp-set-port-security sw0-port1 "50:54:00:00:00:03 10.0.0.3"
+
+ovn-nbctl lsp-add sw0 sw0-port2
+ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:04 10.0.0.4"
+
+ovn-nbctl lr-add lr0
+ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
+ovn-nbctl lsp-add sw0 sw0-lr0
+ovn-nbctl lsp-set-type sw0-lr0 router
+ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
+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
+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-nbctl --wait=hv sync
+
+# Add logical router polcy 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
+
+pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2000)
+pol2=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1000)
+
+ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100
+
+OVS_WAIT_UNTIL([
+    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \
+    grep "load:0x64->NXM_NX_PKT_MARK" -c)
+])
+
+AT_DATA([flows.txt], [dnl
+table=0, priority=0 actions=NORMAL
+table=0, priority=100, pkt_mark=0x64 actions=drop
+table=0, priority=100, pkt_mark=0x2 actions=drop
+])
+
+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_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 | grep "priority=100,pkt_mark=0x64" | \
+    grep "n_packets=1" -c)
+])
+
+OVS_WAIT_UNTIL([
+    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)
+
+OVS_WAIT_UNTIL([
+    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
+    grep priority=0 | \
+    grep "n_packets=1" -c)
+])
+
+OVS_WAIT_UNTIL([
+    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
+    grep priority=100 | grep "priority=100,pkt_mark=0x64" | \
+    grep "n_packets=1" -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)
+])
+
+OVS_WAIT_UNTIL([
+    test 0 -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-phys table=0 | \
+    grep priority=100 | grep "priority=100,pkt_mark=0x2" | \
+    grep "n_packets=1" -c)
+])
+
+OVS_WAIT_UNTIL([
+    test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
+    grep priority=0 | \
+    grep "n_packets=1" -c)
+])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP