On Wed, Aug 17, 2016 at 6:39 AM, <bscha...@redhat.com> wrote:
> From: Babu Shanmugam <bscha...@redhat.com>
>
> ovn-northd sets 'ip.dscp' to the DSCP value
>
> Signed-off-by: Babu Shanmugam <bscha...@redhat.com>
> ---
> ovn/lib/logical-fields.c | 2 +-
> ovn/northd/ovn-northd.c | 13 +++++++++
> ovn/ovn-nb.xml | 6 ++++
> ovn/ovn-sb.xml | 5 ++++
> tests/ovn.at | 73 ++++++++++++++++++++++++++++++
> ++++++++++++++++++
> 5 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
> index 6dbb4ae..068c000 100644
> --- a/ovn/lib/logical-fields.c
> +++ b/ovn/lib/logical-fields.c
> @@ -134,7 +134,7 @@ ovn_init_symtab(struct shash *symtab)
> expr_symtab_add_predicate(symtab, "ip6", "eth.type == 0x86dd");
> expr_symtab_add_predicate(symtab, "ip", "ip4 || ip6");
> expr_symtab_add_field(symtab, "ip.proto", MFF_IP_PROTO, "ip", true);
> - expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP, "ip", false);
> + expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP_SHIFTED, "ip",
> false);
> expr_symtab_add_field(symtab, "ip.ecn", MFF_IP_ECN, "ip", false);
> expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
>
>
You removed the description of the flows that you are adding from
ovn-northd.8.xml. You are still adding flows. They should be described in
ovn-northd.8.xml.
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 91e1687..eed12c9 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2631,6 +2631,19 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
> ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
> ds_cstr(&match), ds_cstr(&actions));
>
> + const char *dscp = smap_get(&op->sb->options, "qos_dscp");
> + if (dscp) {
> + struct ds dscp_actions = DS_EMPTY_INITIALIZER;
> + struct ds dscp_match = DS_EMPTY_INITIALIZER;
> +
> + ds_put_format(&dscp_match, "inport == %s", op->json_key);
> + ds_put_format(&dscp_actions, "ip.dscp = %s; next;", dscp);
> + ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_IP, 100,
> + ds_cstr(&dscp_match), ds_cstr(&dscp_actions));
>
You are still adding this flow to stage S_SWITCH_IN_PORT_SEC_IP with
priority 100.
> + ds_destroy(&dscp_match);
> + ds_destroy(&dscp_actions);
> + }
> +
> if (op->nbsp->n_port_security) {
> build_port_security_ip(P_IN, op, lflows);
>
If you follow this code, it adds priority 90 and priority 80 flows to stage
S_SWITCH_IN_PORT_SEC_IP with priority 90 and priority 80. The point is to
drop traffic that does not match certain combinations of inport, source
ethernet address, and source IP address.
With your code, if qos_dscp is specified for a port, a priority 100 flow
will be inserted that will override the priority 90 and priority 80 flows,
removing IP port security functionality from that port.
IMO this should be in a new ingress pipeline stage in order to avoid
interactions with any other OVN features.
> build_port_security_nd(op, lflows);
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 76309f8..b56b304 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -291,6 +291,12 @@
> If set, indicates the maximum burst size for data sent from this
> interface, in bits.
> </column>
> +
> + <column name="options" key="qos_dscp">
> + If set, indicates the DSCP code to be marked on the packets
> ingressing
> + the switch from the VIF interface. Value should be in the range
> of
> + 0 to 63 (inclusive).
> + </column>
> </group>
> </group>
>
> The proposal in this patch is only to support setting DSCP for all traffic
on a certain inport to a single value.
While that is a common use case, it is far from the only use case. Another
common use case is to limit the maximum DSCP value on an inport, still
allowing a range of values to be used on that port. There are other use
cases.
This makes me wonder whether port-based DSCP is the right place to start. I
can think of two main alternatives:
1) Allow matching on arbitrary fields by adding a table that is similar to
ACLs but with different actions, adding to ovn-nb.ovsschema along the lines
of:
"Logical_Switch": {
"columns": {
"name": {"type": "string"},
"ports": {"type": {"key": {"type": "uuid",
"refTable":
"Logical_Switch_Port",
"refType": "strong"},
"min": 0,
"max": "unlimited"}},
"acls": {"type": {"key": {"type": "uuid",
"refTable": "ACL",
"refType": "strong"},
"min": 0,
"max": "unlimited"}},
+ "qos": {"type": {"key": {"type": "uuid",
+ "refTable": "QoS",
+ "refType": "strong"},
+ "min": 0,
+ "max": "unlimited"}},
"load_balancer": {"type": {"key": {"type": "uuid",
...
+ "QoS": {
+ "columns": {
+ "priority": {"type": {"key": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 32767}}},
+ "direction": {"type": {"key": {"type": "string",
+ "enum": ["set", ["from-lport",
"to-lport"]]}}},
+ "match": {"type": "string"},
+ "action": {"type": {"key": {"type": "string",
+ "enum": ["set", ["dscp",
"cos"]]},
+ "value": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 63}}},
+ "external_ids": {
+ "type": {"key": "string", "value": "string",
+ "min": 0, "max": "unlimited"}}},
+ "isRoot": false},
2) Use the existing ACL table directly, adding action type "dscp" and
action "value", building on the "ovn: Add second ACL stage" patch that I
proposed a few weeks ago. The main problem with this approach is the
difference in processing of "has_stateful" flows for security functionality
versus QoS. The simple approach is to add a new QoS stage that is not
stateful, replacing "has_stateful" with an array, one value for each stage.
However, there is some question in the long run whether one would ever want
to do stateful processing for QoS. This particularly makes sense if OVN
ever adds any deep packet inspection capability, identifying applications
by looking beyond the L4 header rather than trusting the TCP or UDP port
value. In this case, the stateful processing of ACLs and the stateful
processing of QoS would look rather different. This suggests that it might
make more sense to keep features such as QoS and SFC separate from ACLs for
security.
Mickey
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
