I am sending out v10 shortly with the agreed changes. Thank you for the
review, Mickey.
On Saturday 27 August 2016 01:00 AM, Mickey Spiegel wrote:
On Wed, Aug 17, 2016 at 6:39 AM, <bscha...@redhat.com
<mailto:bscha...@redhat.com>> wrote:
From: Babu Shanmugam <bscha...@redhat.com
<mailto:bscha...@redhat.com>>
ovn-northd sets 'ip.dscp' to the DSCP value
Signed-off-by: Babu Shanmugam <bscha...@redhat.com
<mailto: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 <http://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.
I agree.
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.
I agree.
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.
This is a nice idea. Do you think the proposed patch is good for now?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
