*** Bug 1456592 has been marked as a duplicate of this bug. ***

The plan for the first step is (later we can expand beyond that):

 - no support for rules. You have to manage rules outside NM any way you want. 
   NM ignores them.

 - routes from DHCP/DHCP6/SLAAC are still only added to the "main" table. 
   The feature only matters for manually added IPv4 and IPv6 routes.

 - manually configured IPv4 and IPv6 routes can have a new optional 
   attribute "table $TABLEID". Obviously, configuring the table of a route
   has the effect of configuring the route in that table.

That's about it. In step 1, the only new thing will be the new table property, and it only works for manual routes.




One additional thing: currently NM only manages the main table and ignores all other tables.
NM syncs the routes it wants to configure with kernel (in the main table for the interface in question). The important part here is that "sync" means to remove all routes from the main table (for that interface) which it doesn't expect. NM does not only add routes, it also deletes existing routes.

When adding support to policy-routing, that sync behavior could be harmful for existing setups. Instead, NM also a new option ipv4.route-table-sync and ipv6.route-table-sync that can have 3 values:

  - "none": means NM will never delete any routes that it finds. It only adds 
    new routes.
  - "main" (default): NM will delete routes from the main table, but in all 
    other tables it will accept existing routes without deleting them.
  - "full": NM will sync all tables (except the local table #255).

Sounds good to me.

Sounds good to me too, it covers what we do today with the exception of the table ID generation.
Currently VDSM is generating the table ID, perhaps NM can help with that as well.
(It uses the IP address 32 bits for the table ID)

Question: Have netns discussed? At some level, it may cover this need as well, but I'm not completely sure.

(In reply to Edward Haas from comment #5)
> Sounds good to me too, it covers what we do today with the exception of the
> table ID generation.
> Currently VDSM is generating the table ID, perhaps NM can help with that as
> well.
> (It uses the IP address 32 bits for the table ID)

There isn't a "generation" of the table ID. You pick a number, that's it.
It's up to the user (you) to pick that number. Using the IPv4 address sounds like a good idea.

iproute2 also supports aliases from /etc/iproute2/rt_tables. I don't think NM should accept such names, because then the connection profile is no longer self contained. E.g. export + import on another host might result in a broken configuration, because the table name is missing.


(In reply to Edward Haas from comment #6)
> Question: Have netns discussed? At some level, it may cover this need as
> well, but I'm not completely sure.

Yes, but it's entirely independent from policy routing. Links, addresses and routes all life inside one netns, there is no overlap. NM manages addresses/routes per netns. If it supports multiple netns, it manages them entirely separately. Especially there is no direct relation with policy-routing.

For afnetns that might be slightly different, because here the addresses of one interface might life in separate afnetns.

please review a first version of th/policy-routing-pt1-rh1436531

(In reply to Edward Haas from comment #0)

> In its simplest form, for each network a default route can be set, allowing
> traffic from different source addresses to be forwarded by default to a
> different next hop. This is the current usage for RHV and the trigger for
> requesting such an option from NM.

Hm, the current branch adds support for routing tables for manually added routes.

Notably, NetworkManager does not treat the default-route like normal routes (which I think is a mistake, but done historically). Hence, the branch would not yet allow you to configure the default route in another table.

Do you explicitly require to configure the default route as well?

> device: do full update_ext_ip_config() during merge-and-apply

This looks suspicious to me...

	if (commit) {
		ensure_con_ip4_config (self);
+               if (priv->queued_ip4_config_id)
+                       update_ext_ip_config (self, AF_INET, FALSE);
	}

When there is a queued ip4 configuration, ensure_con_ip4_config()
creates a new priv->con_ip4_config from the setting and then
update_ext_ip_config() will intersect it with the external
configuration, removing everything (because we have not committed the
configuration yet).


> libnm,cli: add IP setting "route-table-sync"

Please document somewhere what the default value is, for example here:

	 <varlistentry>
+          <term><varname>ipv4.route-table-sync</varname></term>
+        </varlistentry>
+        <varlistentry>


Pushed a trivial fix.

(In reply to Beniamino Galvani from comment #10)
> > device: do full update_ext_ip_config() during merge-and-apply

addressed all (?) and repushed.

How about now?

LGTM

+
+       /**
+        * NMSettingIPConfig:route-table-sync:
+        *
+        * The mode how to sync the routes. In general, when NetworkManager manages
+        * a device it will remove extraneous routes from the routing tables. The
+        * sync parameter specifies which tables are synced this way. Meaning, from
+        * which routing table NetworkManager will remove routes that it doesn't
+        * expect there. The default value can be overwritten via global configuration.
+        * If unspecified, the default value is 2 (main).

I think here would be great to have also briefly listed the available values that the property can get. Something like:

"A value of 1 (none) means that route tables will not be synced, 2 (main) means that only the main table will be synced, 3 (all) will sync all the route tables. A value of zero (or unspecified) means to use the default value, which is 2 (main) if not specified via global configuration" 


commit msg typo in:
"device: refactor update-ip-config for device"
[...]
"And also, I think it's an antipattern [two-->TO] have two mirroring functions"


Apart from this imperfections, branch looks great!

thanks. Merged branch th/policy-routing-pt1-rh1436531 to master:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e6292bcb2a1326dd2ff4f53933acb76b8724e6d9


What is still missing is support for configuring the default-route in a different table. I think that shall be done by accepting default-routes like regular routes in ipv4.routes:

  nmcli connection modify "$CON" +ipv4.routes 'default 192.168.5.1 table=55'


Currently the addition of the default route depends on ipv4.never-default, ipv4.gateway, and whatever comes via ipv4.method.
Specifying default routes in ipv4.routes would be in addition to those methods.

Please review th/policy-routing-pt2-rh1436531.

It now configures also the routing table for DHCP, default-route, everything.

This breaks API/ABI on current master branch (unstable).

(In reply to Thomas Haller from comment #15)
> Please review th/policy-routing-pt2-rh1436531.
> 
> It now configures also the routing table for DHCP, default-route, everything.

Looks good to me (pushed a trivial fixup).

> platform: mark static nla_policy variables as const

-       err = nla_parse_nested (tb, IFLA_INET6_MAX, attr, policy);
+       err = nla_parse_nested (tb, IFLA_INET6_MAX, attr, (struct nla_policy *) policy);

The cast is not needed.


> core: refactor parsing resolve.conf

gboolean
+nm_utils_resolve_conf_parse (int addr_family,
+                             const char *rc_contents,
+                             GArray *nameservers,
+                             GPtrArray *dns_options)
+{
+       guint i;
+       gboolean changed = FALSE;
+       gs_free const char **lines;

gs_free const char **lines = NULL;


+       g_return_val_if_fail (   nameservers
+                             && (   (   addr_family == AF_INET
+                                     && g_array_get_element_size (nameservers) == sizeof (in_addr_t))
+                                 || (   addr_family == AF_INET6
+                                     && g_array_get_element_size (nameservers) == sizeof (struct in6_addr))), FALSE);

Maybe, for readability split the expression in:

       g_return_val_if_fail (nameservers);
       g_return_val_if_fail (... the rest ...);

The rest LGTM.

(In reply to Beniamino Galvani from comment #17)

addresses all and repushed.

thanks!

lgtm

thanks.

merged to master: 

part1: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3a81e20d7693a3b8d317a57cfe45a48c09eef9f3
part2: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e037c66b6a14d4465cd0f2219bfe5320e72ef743

running in CI since now

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:0778

(In reply to Thomas Haller from comment #3)
> The plan for the first step is (later we can expand beyond that):
> 
>  - no support for rules. You have to manage rules outside NM any way you
> want. 
>    NM ignores them.
[...]

What is the issue number to track progress on adding support for rules?
Having to keep one configuration in n-m and one outside breaks all the confort of using nmcli to configure everything.