netplan-developers team mailing list archive

[Martin Pitt <martin.pitt@xxxxxxxxxx>]

Review: Needs Fixing

Very close now, thanks! A bunch of nitpicks, two errors, and some missing tests. Feel free to land this yourself after this round, as I'll be on EOY holidays from tomorrow on.

Diff comments:

> diff --git a/src/networkd.c b/src/networkd.c
> index 2d0e586..1efccdc 100644
> --- a/src/networkd.c
> +++ b/src/networkd.c
> @@ -168,6 +168,16 @@ write_network_file(net_definition* def, const char* rootdir, const char* path)
>              if (nd->vlan_link == def)
>                  g_string_append_printf(s, "VLAN=%s\n", nd->id);
>      }
> +    if (def->routes != NULL) {
> +        for (unsigned i = 0; i < def->routes->len; ++i) {

nitpick: you can just add that \n to the append_printf() two lines down.

> +            g_string_append(s, "\n");
> +            ip_route* cur_route = g_array_index (def->routes, ip_route*, i);
> +            g_string_append_printf(s, "[Route]\nDestination=%s\nGateway=%s\n",
> +                                   cur_route->to, cur_route->via);
> +            if (cur_route->metric != G_MAXUINT)

Fine for me, but to avoid confusion I'd actually do something like "#define METRIC_UNSPEC G_MAXUINT".

> +                g_string_append_printf(s, "Metric=%d\n", cur_route->metric);
> +        }
> +    }
>  
>      /* NetworkManager compatible route metrics */
>      if (def->dhcp4 || def->dhcp6)
> diff --git a/src/nm.c b/src/nm.c
> index f1d39af..2a4253b 100644
> --- a/src/nm.c
> +++ b/src/nm.c
> @@ -235,6 +255,7 @@ write_nm_conf_access_point(net_definition* def, const char* rootdir, const wifi_
>          g_string_append(s, "\n");
>      }
>      write_search_domains(def, s);
> +    write_routes(def, s, AF_INET);
>  
>      if (def->dhcp6 || def->ip6_addresses || def->gateway6 || def->ip6_nameservers) {

I think you need to add || def->routes here, and likewise to the IPv4 block above. Otherwise nothing will be written if the *only* thing in the netdef definition is a routes:. (This should perhaps get a test case then). Admittedly it does not make much sense to add a "routes:" without "addresses:" or "dhcp4:", but one never knows..

>          g_string_append(s, "\n[ipv6]\n");
> diff --git a/src/parse.c b/src/parse.c
> index 2db2b11..b56c646 100644
> --- a/src/parse.c
> +++ b/src/parse.c
> @@ -625,6 +628,125 @@ handle_nameservers_addresses(yaml_document_t* doc, yaml_node_t* node, const void
>      return TRUE;
>  }
>  
> +
> +static int 
> +get_ip_family(const char* address)
> +{
> +    struct in_addr a4;
> +    struct in6_addr a6;
> +    g_autofree char *ip_str;
> +    char *prefix_len;
> +    int family;
> +    int ret = -1;
> +
> +    ip_str = g_strdup(address);
> +    prefix_len = strrchr(ip_str, '/');
> +    if (prefix_len)
> +        *prefix_len = '\0';
> +
> +    family = AF_INET;

No need for this variable IMHO, just return the AF_INET constant. Same thing for 6 below.

> +    ret = inet_pton(AF_INET, ip_str, &a4);
> +    g_assert(ret >= 0);
> +    if (ret > 0)
> +        return family;
> +
> +    family = AF_INET6;
> +    ret = inet_pton(AF_INET6, ip_str, &a6);
> +    g_assert(ret >= 0);
> +    if (ret > 0)
> +        return family;
> +
> +    return -1;
> +}
> +
> +static gboolean
> +check_and_set_family(int family)
> +{
> +    if (cur_route->family != -1 && cur_route->family != family)
> +        return FALSE;
> +
> +    cur_route->family = family;
> +
> +    return TRUE;
> +}
> +
> +static gboolean
> +handle_routes_ip(yaml_document_t* doc, yaml_node_t* node, const void* data, GError** error)
> +{
> +    guint offset = GPOINTER_TO_UINT(data);
> +    int family = get_ip_family(scalar(node));
> +    char** dest = (char**) ((void*) cur_route + offset);
> +    g_free(*dest);
> +
> +    if (family < 0)
> +        return yaml_error(node, error, "invalid IP family %d", family);
> +
> +    if (!check_and_set_family(family))
> +        return yaml_error(node, error, "IP family mismatch in route to %s", scalar(node));
> +
> +    *dest = g_strdup(scalar(node));
> +
> +    return TRUE;
> +}
> +
> +static gboolean
> +handle_routes_metric(yaml_document_t* doc, yaml_node_t* node, const void* _, GError** error)
> +{
> +    guint64 v;
> +    gchar* endptr;
> +
> +    v = g_ascii_strtoull(scalar(node), &endptr, 10);
> +    if (*endptr != '\0' || v > G_MAXUINT)
> +        return yaml_error(node, error, "invalid unsigned int value %s", scalar(node));
> +
> +    cur_route->metric = (guint) v;
> +    return TRUE;
> +}
> +
> +/****************************************************
> + * Grammar and handlers for network config "routes" entry
> + ****************************************************/
> +
> +const mapping_entry_handler routes_handlers[] = {
> +    {"to", YAML_SCALAR_NODE, handle_routes_ip, NULL, route_offset(to)},
> +    {"via", YAML_SCALAR_NODE, handle_routes_ip, NULL, route_offset(via)},
> +    {"metric", YAML_SCALAR_NODE, handle_routes_metric},
> +    {NULL}
> +};
> +
> +static gboolean
> +handle_routes(yaml_document_t* doc, yaml_node_t* node, const void* _, GError** error)
> +{
> +    for (yaml_node_item_t *i = node->data.sequence.items.start; i < node->data.sequence.items.top; i++) {
> +        yaml_node_t *entry = yaml_document_get_node(doc, *i);
> +
> +        cur_route = g_new0(ip_route, 1);
> +        cur_route->family = G_MAXUINT; /* 0 is a valid family ID */
> +        cur_route->metric = G_MAXUINT; /* 0 is a valid metric */
> +
> +        if (process_mapping(doc, entry, routes_handlers, error)) {
> +            if (!cur_netdef->routes) {
> +                cur_netdef->routes = g_array_new(FALSE, FALSE, sizeof(ip_route*));
> +            }

We never expect two "routes:" entries as that would be an invalid dict. Perhaps this should be a g_assert for NULL instead of an "if"?

> +
> +            g_array_append_val(cur_netdef->routes, cur_route);
> +        }
> +
> +        if (!cur_route->to || !cur_route->via)
> +            return yaml_error(node, error, "route must include both a 'to' and 'via' IP");
> +
> +        cur_route = NULL;
> +
> +        if (error && *error)
> +            return FALSE;
> +    }
> +    return TRUE;
> +}
> +
> +/****************************************************
> + * Grammar and handlers for network devices
> + ****************************************************/
> +
>  const mapping_entry_handler nameservers_handlers[] = {
>      {"search", YAML_SEQUENCE_NODE, handle_nameservers_search},
>      {"addresses", YAML_SEQUENCE_NODE, handle_nameservers_addresses},
> @@ -657,6 +780,7 @@ const mapping_entry_handler wifi_def_handlers[] = {
>      {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
>      {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
>      {"access-points", YAML_MAPPING_NODE, handle_wifi_access_points},
> +    {"routes", YAML_SEQUENCE_NODE, NULL, routes_handlers},

This and the entries below need to be handle_routes, like for ethernet. I suggest to add a test case for wifi and/or a bridge route too.

>      {NULL}
>  };
>  
> diff --git a/src/parse.h b/src/parse.h
> index 7c5bf2b..bd0ea26 100644
> --- a/src/parse.h
> +++ b/src/parse.h
> @@ -97,6 +98,14 @@ typedef struct {
>      char* password;
>  } wifi_access_point;
>  
> +typedef struct {
> +    guint family;
> +
> +    char* to;
> +    char* via;
> +
> +    guint metric;

Maybe add a comment about ROUTE_UNSPEC / G_MAXUINT here?

> +} ip_route;
>  
>  /* Written/updated by parse_yaml(): char* id →  net_definition */
>  extern GHashTable* netdefs;
> diff --git a/tests/generate.py b/tests/generate.py
> index b3a127b..3eee8be 100755
> --- a/tests/generate.py
> +++ b/tests/generate.py
> @@ -1180,6 +1265,96 @@ method=manual
>  address1=2001:FFfe::1/64
>  '''})
>  
> +    def test_route_v4_single(self):
> +        self.generate('''network:
> +  version: 2
> +  renderer: NetworkManager
> +  ethernets:
> +    engreen:
> +      addresses: ["192.168.14.2/24"]
> +      routes:
> +        - to: 10.10.10.0/24
> +          via: 192.168.14.20
> +          metric: 100
> +          ''')
> +
> +        self.assert_nm({'engreen': '''[connection]
> +id=netplan-engreen
> +type=ethernet
> +interface-name=engreen
> +
> +[ethernet]
> +wake-on-lan=0
> +
> +[ipv4]
> +method=manual
> +address1=192.168.14.2/24
> +route1=10.10.10.0/24,192.168.14.20,100
> +'''})
> +
> +    def test_route_v4_multiple(self):
> +        self.generate('''network:
> +  version: 2
> +  renderer: NetworkManager
> +  ethernets:
> +    engreen:
> +      addresses: ["192.168.14.2/24"]
> +      routes:
> +        - to: 8.8.0.0/16
> +          via: 192.168.1.1
> +          metric: 10

All of your tests set a metric, you should have one without a metric setting too (once per family).

> +        - to: 10.10.10.8
> +          via: 192.168.1.2
> +          metric: 5000
> +        - to: 11.11.11.0/24
> +          via: 192.168.1.3
> +          metric: 9999
> +          ''')
> +
> +        self.assert_nm({'engreen': '''[connection]
> +id=netplan-engreen
> +type=ethernet
> +interface-name=engreen
> +
> +[ethernet]
> +wake-on-lan=0
> +
> +[ipv4]
> +method=manual
> +address1=192.168.14.2/24
> +route1=8.8.0.0/16,192.168.1.1,10
> +route2=10.10.10.8,192.168.1.2,5000
> +route3=11.11.11.0/24,192.168.1.3,9999
> +'''})
> +
> +    def test_route_v6_single(self):
> +        self.generate('''network:
> +  version: 2
> +  renderer: NetworkManager
> +  ethernets:
> +    enblue:
> +      addresses: ["2001:f00f:f00f::2/64"]
> +      routes:
> +        - to: 2001:dead:beef::2/64
> +          via: 2001:beef:beef::1''')
> +
> +        self.assert_nm({'enblue': '''[connection]
> +id=netplan-enblue
> +type=ethernet
> +interface-name=enblue
> +
> +[ethernet]
> +wake-on-lan=0
> +
> +[ipv4]
> +method=link-local
> +
> +[ipv6]
> +method=manual
> +address1=2001:f00f:f00f::2/64
> +route1=2001:dead:beef::2/64,2001:beef:beef::1
> +'''})
> +
>      def test_wifi_default(self):
>          self.generate('''network:
>    version: 2
> diff --git a/tests/integration.py b/tests/integration.py
> index 6897395..6474a64 100755
> --- a/tests/integration.py
> +++ b/tests/integration.py
> @@ -413,6 +414,72 @@ class _CommonTests:
>          with open('/sys/class/net/mybond/bonding/slaves') as f:
>              self.assertEqual(f.read().strip(), self.dev_e_client)
>  
> +    @unittest.skip("fails due to networkd bug setting routes with dhcp")
> +    def test_routes_v4_with_dhcp(self):
> +        self.setup_eth(None)
> +        with open(self.config, 'w') as f:
> +            f.write('''network:
> +  renderer: %(r)s
> +  ethernets:
> +    %(ec)s:
> +      dhcp4: yes
> +      routes:
> +          - to: 10.10.10.0/24
> +            via: 192.168.5.254
> +            metric: 99''' % {'r': self.backend, 'ec': self.dev_e_client})
> +        self.generate_and_settle()
> +        self.assert_iface_up(self.dev_e_client,
> +                             ['inet 192.168.5.[0-9]+/24'])  # from DHCP
> +        self.assertIn(b'default via 192.168.5.1',  # from DHCP
> +                      subprocess.check_output(['ip', 'route', 'show', 'dev', self.dev_e_client]))
> +        self.assertIn(b'10.10.10.0/24 via 192.168.5.254',  # from DHCP
> +                      subprocess.check_output(['ip', 'route', 'show', 'dev', self.dev_e_client]))
> +
> +    def test_routes_v4(self):

Fine for me to add new tests, if their additional runtime doesn't bother you. Otherwise you could add the routes to test_manual_addresses().

> +        self.setup_eth(None)
> +        with open(self.config, 'w') as f:
> +            f.write('''network:
> +  renderer: %(r)s
> +  ethernets:
> +    %(ec)s:
> +      addresses:
> +          - 192.168.5.99/24
> +      gateway4: 192.168.5.1
> +      routes:
> +          - to: 10.10.10.0/24
> +            via: 192.168.5.254
> +            metric: 99''' % {'r': self.backend, 'ec': self.dev_e_client})
> +        self.generate_and_settle()
> +        self.assert_iface_up(self.dev_e_client,
> +                             ['inet 192.168.5.[0-9]+/24'])  # from DHCP
> +        self.assertIn(b'default via 192.168.5.1',  # from DHCP
> +                      subprocess.check_output(['ip', 'route', 'show', 'dev', self.dev_e_client]))
> +        self.assertIn(b'10.10.10.0/24 via 192.168.5.254',  # from DHCP
> +                      subprocess.check_output(['ip', 'route', 'show', 'dev', self.dev_e_client]))

Please check that the metric is as expected too.

> +
> +    def test_routes_v6(self):
> +        self.setup_eth(None)
> +        with open(self.config, 'w') as f:
> +            f.write('''network:
> +  renderer: %(r)s
> +  ethernets:
> +    %(ec)s:
> +      addresses: ["9876:BBBB::11/70"]
> +      gateway6: "9876:BBBB::1"
> +      routes:
> +          - to: 2001:f00f:f00f::1/64
> +            via: 9876:BBBB::5
> +            metric: 99''' % {'r': self.backend, 'ec': self.dev_e_client})
> +        self.generate_and_settle()
> +        self.assert_iface_up(self.dev_e_client,
> +                             ['inet6 9876:bbbb::11/70'])
> +        self.assertNotIn(b'default',
> +                         subprocess.check_output(['ip', 'route', 'show', 'dev', self.dev_e_client]))
> +        self.assertIn(b'default via 9876:bbbb::1',
> +                      subprocess.check_output(['ip', '-6', 'route', 'show', 'dev', self.dev_e_client]))
> +        self.assertIn(b'2001:f00f:f00f::/64 via 9876:bbbb::5',
> +                      subprocess.check_output(['ip', '-6', 'route', 'show', 'dev', self.dev_e_client]))
> +
>      def test_manual_addresses(self):
>          self.setup_eth(None)
>          with open(self.config, 'w') as f:


-- 
https://code.launchpad.net/~netplan-developers/netplan/+git/netplan/+merge/312683
Your team Developers of netplan is subscribed to branch netplan:master.