On Wed, Jan 16, 2019 at 07:01:13PM +0000, Mary Manohar wrote:
> Policy-based routing (PBR) provides a mechanism to configure permit/deny and
> reroute policies on the router.
> Permit/deny policies are similar to OVN ACLs, but exist on the logical-router.
> Reroute policies are needed for service-insertion and service-chaining.
> Currently, policies are stateless.
>
> To achieve this, a new table is introduced in the ingress pipeline of the
> Logical-router.
> The new table is between the ‘IP Routing’ and the ‘ARP/ND resolution’ table.
> This way, PBR can override routing decisions and provide a different next-hop.
>
> This Patch:
> a. Changes in OVN NB Schema to introduce a new table in the Logical router.
> b. Add commands to ovn-nbctl to add/delete/list routing policies.
> c. Changes in ovn-northd to process routing-policy configurations.
>
> A new table 'Logical_Router_Policy' has been added in the northbound
> schema.
> The table has the following columns:
> * priority: Rules with numerically higher priority take precedence over
> those with lower.
> * match: Uses the same expression language as the 'match' column of
> 'Logical_Flow' table in the OVN Southbound database.
> * action: allow/drop/reroute
> * nexthop: Nexthop IP address.
>
> Each row in this table represents one routing policy for a logical
> router.
> The 'action' column for the highest priority matching row in this table
> determines a packet's treatment. If no row matches, packets are allowed
> by
> default.
>
> The new ovn-nbctl commands are as follows:
> 1. Add a new ovn-nbctl command to add a routing policy.
> lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP]
>
> Nexthop is an optional parameter.
> It needs to be provided only when 'action' is 'reroute'
> A policy is uniquely identified by priority and match.
> Multiple policies can have the same priority.
>
> 2. Add a new ovn-nbctl command to delete a routing policy.
> lr-policy-del ROUTER [PRIORITY [MATCH]]
>
> Takes priority and match as optional parameters.
> If priority and match are specified, the policy with the given
> priority and match is deleted.
> If priority is specified and match is not specified, all rules with
> that priority are deleted.
> If priority is not specified, all the rules would be deleted.
>
> 3. Add a new ovn-nbctl command to list routing-policies in the logical
> router.
> lr-policy-list ROUTER
>
> ovn-northd changes are to get routing-policies from northbound database
> and populate the same as logical flows in the southbound database.
> A new table called 'POLICY' is introduced in the Logical router's ingress
> pipeline.
> Each routing-policy configured in the northbound database translates into
> a single logical flow in the new table.
>
> The columns from the Logical_Router_Policy table are used as follows:
> The priority column is used as priority in the logical-flow.
> The match column is used as the 'match' string in the logical-flow.
> The action column is used to determine the action of the logical-flow.
>
> When the 'action' is reroute, if the nexthop ip-address is a connected
> router port or the IP address of a logical port,
> the logical-flow is constructed to route the packet to the nexthop
> ip-address.
>
> Signed-off-by: Mary Manohar <[email protected]>
Thanks for the patch! It is good work. I have some feedback for you,
although most of it is fairly superficial.
It looks like this is IPv4-only. OVN routing supports IPv4 and IPv6,
and I don't know a reason why it would be much extra work to support
IPv6 or why IPv6 should not support policy routing.
The first line of a commit message should be short, so that it fits on a
single line, but this commit message has a whole paragraph in the first
line.
A commit message should be wrapped at 75 columns or so, and generally
not indented, but this commit message has many long lines and the
indentation seems overall kind of random.
checkpatch has some feedback too. I guess you probably got the same
from the robot as well:
ERROR: Inappropriate bracing around statement
#28 FILE: ovn/northd/ovn-northd.c:4698:
if (nexthop == NULL)
WARNING: Line is 80 characters long (recommended limit is 79)
#73 FILE: ovn/northd/ovn-northd.c:4743:
VLOG_WARN_RL(&rl, "No path for routing policy priority %d; next hop
%s",
WARNING: Line is 80 characters long (recommended limit is 79)
#205 FILE: ovn/ovn-nb.ovsschema:246:
"refTable":
"Logical_Router_Policy",
WARNING: Line is 93 characters long (recommended limit is 79)
#223 FILE: ovn/ovn-nb.ovsschema:318:
"enum": ["set", ["allow",
"drop", "reroute"]]}}},
WARNING: Line is 80 characters long (recommended limit is 79)
#269 FILE: ovn/ovn-nb.xml:1869:
The packets that the routing policy should match, in the same
expression
WARNING: Line lacks whitespace around operator
#319 FILE: ovn/utilities/ovn-nbctl.c:644:
lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP]\n\
WARNING: Line lacks whitespace around operator
#321 FILE: ovn/utilities/ovn-nbctl.c:646:
lr-policy-del ROUTER [PRIORITY [MATCH]]\n\
WARNING: Line lacks whitespace around operator
#323 FILE: ovn/utilities/ovn-nbctl.c:648:
lr-policy-list ROUTER print policies for ROUTER\n\
ERROR: Inappropriate bracing around statement
#394 FILE: ovn/utilities/ovn-nbctl.c:3468:
if (next_hop != NULL)
ERROR: Inappropriate bracing around statement
#480 FILE: ovn/utilities/ovn-nbctl.c:3554:
} else
In get_outport_for_routing_policy_nexthop(), this is always going to
report error 0:
+ char *error = ip_parse_cidr(nexthop, &nexthop_be32, &plen);
+ if (!error) {
+ if (plen != 32) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+ VLOG_WARN_RL(&rl, "bad next hop ip %s for routing policy "
+ "with priority %d, error: %s ",
+ nexthop, priority, error);
+ return NULL;
+ }
but I think it could just use ip_parse() and then it wouldn't have to
worry about a mask at all.
Some of the coding style and indentation was a little unusual, and some
of the code made me think a little too hard, so I'd suggest folding in
the following in the next version because it eases all of these for me:
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 0f99420b11fd..6ffd105641b3 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -4719,32 +4719,18 @@ get_outport_for_routing_policy_nexthop(struct
ovn_datapath *od,
return NULL;
}
- /* find the router port matching the next hop. */
- int i;
- struct ovn_port *out_port = NULL;
- const char *lrp_addr_s = NULL;
- for (i = 0; i < od->nbr->n_ports; i++) {
+ /* Find the router port matching the next hop. */
+ for (int i = 0; i < od->nbr->n_ports; i++) {
struct nbrec_logical_router_port *lrp = od->nbr->ports[i];
- out_port = ovn_port_find(ports, lrp->name);
- if (!out_port) {
- /* This should not happen. */
- continue;
- }
- lrp_addr_s = find_lrp_member_ip(out_port, nexthop);
- if (lrp_addr_s) {
- break;
- } else {
- out_port = NULL;
+ struct ovn_port *out_port = ovn_port_find(ports, lrp->name);
+ if (out_port && find_lrp_member_ip(out_port, nexthop)) {
+ return out_port;
}
}
- if (!out_port) {
- /* There is no matched out port. */
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
- VLOG_WARN_RL(&rl, "No path for routing policy priority %d; next hop
%s",
- priority, nexthop);
- return NULL;
- }
- return out_port;
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+ VLOG_WARN_RL(&rl, "No path for routing policy priority %d; next hop %s",
+ priority, nexthop);
+ return NULL;
}
static void
@@ -4756,33 +4742,30 @@ build_routing_policy_flow(struct hmap *lflows, struct
ovn_datapath *od,
struct ds actions = DS_EMPTY_INITIALIZER;
if (!strcmp(rule->action, "reroute")) {
- struct ovn_port *out_port = NULL;
- const char *lrp_addr_s = NULL;
- out_port = get_outport_for_routing_policy_nexthop(
+ struct ovn_port *out_port = get_outport_for_routing_policy_nexthop(
od, ports, rule->priority, rule->nexthop);
- if (out_port == NULL) {
- return;
- } else {
- lrp_addr_s = find_lrp_member_ip(out_port, rule->nexthop);
- if (!lrp_addr_s) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
- VLOG_WARN_RL(&rl, "lrp_addr not found for routing policy "
- " priority %d nexthop %s",
- rule->priority, rule->nexthop);
- return;
- } else {
- ds_put_format(&actions, "reg0 = %s; "
- "reg1 = %s; "
- "eth.src = %s; "
- "outport = %s; "
- "flags.loopback = 1; "
- "next;",
- rule->nexthop,
- lrp_addr_s,
- out_port->lrp_networks.ea_s,
- out_port->json_key);
- }
+ if (!out_port) {
+ return;
+ }
+
+ const char *lrp_addr_s = find_lrp_member_ip(out_port, rule->nexthop);
+ if (!lrp_addr_s) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+ VLOG_WARN_RL(&rl, "lrp_addr not found for routing policy "
+ " priority %"PRId64" nexthop %s",
+ rule->priority, rule->nexthop);
+ return;
}
+ ds_put_format(&actions, "reg0 = %s; "
+ "reg1 = %s; "
+ "eth.src = %s; "
+ "outport = %s; "
+ "flags.loopback = 1; "
+ "next;",
+ rule->nexthop,
+ lrp_addr_s,
+ out_port->lrp_networks.ea_s,
+ out_port->json_key);
} else if (!strcmp(rule->action, "drop")) {
ds_put_cstr(&actions, "drop;");
} else if (!strcmp(rule->action, "allow")) {
@@ -6533,8 +6516,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
/* Convert routing policies to flows. */
for (int i = 0; i < od->nbr->n_policies; i++) {
- const struct nbrec_logical_router_policy *rule;
- rule = od->nbr->policies[i];
+ const struct nbrec_logical_router_policy *rule
+ = od->nbr->policies[i];
build_routing_policy_flow(lflows, od, ports, rule);
}
}
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 12aef7df49e9..10134c297c81 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -1242,7 +1242,7 @@
</column>
<column name="policies">
- One or more routing policies for the router.
+ Zero or more routing policies for the router.
</column>
<column name="enabled">
@@ -1852,7 +1852,7 @@
column="action"/> column for the highest-<ref column="priority"/>
matching row in this table determines a packet's treatment. If no row
matches, packets are allowed by default. (Default-deny treatment is
- possible: add a rule with <ref column="priority"/> 0, <code>0</code> as
+ possible: add a rule with <ref column="priority"/> 0, <code>1</code> as
<ref column="match"/>, and <code>drop</code> as <ref column="action"/>.)
</p>
@@ -1891,14 +1891,14 @@
</li>
<li>
- <code>reroute</code>: Reroute packet to nexthop
+ <code>reroute</code>: Reroute packet to <ref column="nexthop"/>.
</li>
</ul>
</column>
<column name="nexthop">
<p>
- Nexthop IP address for this route. Nexthop IP address should be the IP
+ Next-hop IP address for this route, which should be the IP
address of a connected router port or the IP address of a logical port.
</p>
</column>
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index babb907cef27..d833fb17e434 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -3421,6 +3421,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
}
const char *action = ctx->argv[4];
char *next_hop = NULL;
+
/* Validate action. */
if (strcmp(action, "allow") && strcmp(action, "drop")
&& strcmp(action, "reroute")) {
@@ -3429,17 +3430,18 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
}
if (!strcmp(action, "reroute")) {
if (ctx->argc < 6) {
- ctl_error(ctx, "Nexthop is not specified when action is reroute.");
+ ctl_error(ctx, "Nexthop is required when action is reroute.");
}
}
+
/* Check if same routing policy already exists.
* A policy is uniquely identified by priority and match */
for (int i = 0; i < lr->n_policies; i++) {
const struct nbrec_logical_router_policy *policy = lr->policies[i];
- if ((policy->priority == priority) &&
- (!strcmp(policy->match, ctx->argv[3]))) {
- ctl_error(ctx, "Same routing policy already existed on the "
- "logical router %s.", ctx->argv[1]);
+ if (policy->priority == priority &&
+ !strcmp(policy->match, ctx->argv[3])) {
+ ctl_error(ctx, "Same routing policy already existed on the "
+ "logical router %s.", ctx->argv[1]);
}
}
if (ctx->argc == 6) {
@@ -3448,6 +3450,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
ctl_error(ctx, "bad next hop argument: %s", ctx->argv[5]);
}
}
+
struct nbrec_logical_router_policy *policy;
policy = nbrec_logical_router_policy_insert(ctx->txn);
nbrec_logical_router_policy_set_priority(policy, priority);
@@ -3460,10 +3463,10 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
struct nbrec_logical_router_policy **new_policies
= xmalloc(sizeof *new_policies * (lr->n_policies + 1));
memcpy(new_policies, lr->policies,
- sizeof *new_policies * lr->n_policies);
+ sizeof *new_policies * lr->n_policies);
new_policies[lr->n_policies] = policy;
nbrec_logical_router_set_policies(lr, new_policies,
- lr->n_policies + 1);
+ lr->n_policies + 1);
free(new_policies);
if (next_hop != NULL)
free(next_hop);
@@ -3479,23 +3482,26 @@ nbctl_lr_policy_del(struct ctl_context *ctx)
ctx->error = error;
return;
}
- if (ctx->argc == 2) {
+
+ if (ctx->argc == 2) {
/* If a priority is not specified, delete all policies. */
nbrec_logical_router_set_policies(lr, NULL, 0);
return;
}
+
error = parse_priority(ctx->argv[2], &priority);
if (error) {
ctx->error = error;
return;
}
+
/* If match is not specified, delete all routing policies with the
* specified priority. */
if (ctx->argc == 3) {
struct nbrec_logical_router_policy **new_policies
- = xmemdup(lr->policies,
- sizeof *new_policies * lr->n_policies);
- int n_policies = 0;
+ = xmemdup(lr->policies,
+ sizeof *new_policies * lr->n_policies);
+ int n_policies = 0;
for (int i = 0; i < lr->n_policies; i++) {
if (priority != lr->policies[i]->priority) {
new_policies[n_policies++] = lr->policies[i];
@@ -3506,18 +3512,19 @@ nbctl_lr_policy_del(struct ctl_context *ctx)
free(new_policies);
return;
}
+
/* Delete policy that has the same priority and match string */
for (int i = 0; i < lr->n_policies; i++) {
struct nbrec_logical_router_policy *routing_policy = lr->policies[i];
- if (priority == routing_policy->priority &&
+ if (priority == routing_policy->priority &&
!strcmp(ctx->argv[3], routing_policy->match)) {
struct nbrec_logical_router_policy **new_policies
= xmemdup(lr->policies,
sizeof *new_policies * lr->n_policies);
- new_policies[i] = lr->policies[lr->n_policies - 1];
+ new_policies[i] = lr->policies[lr->n_policies - 1];
nbrec_logical_router_verify_policies(lr);
nbrec_logical_router_set_policies(lr, new_policies,
- lr->n_policies - 1);
+ lr->n_policies - 1);
free(new_policies);
return;
}
I'll look forward to v4.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
