Skip to content

Reimplement optional-addresses (LP: #1880029) #339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions doc/netplan-yaml.md
Original file line number Diff line number Diff line change
Expand Up @@ -531,11 +531,12 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.

- **optional-addresses** (sequence of scalars)

> Specify types of addresses that are not required for a device to be
> Specify address families that are not required for a device to be
> considered online. This changes the behavior of backends at boot time to
> avoid waiting for addresses that are marked optional, and thus consider
> the interface as "usable" sooner. This does not disable these addresses,
> which will be brought up anyway.
> which will be brought up anyway. Valid values are: ipv4, ipv6 and none.
> If "none" is used, ipv4 and ipv6 will be considered mandatory.

Example:

Expand All @@ -545,7 +546,7 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.
eth7:
dhcp4: true
dhcp6: true
optional-addresses: [ ipv4-ll, dhcp6 ]
optional-addresses: [ ipv6 ]
```

- **activation-mode** (scalar) – since **0.103**
Expand Down
3 changes: 3 additions & 0 deletions src/abi.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ typedef enum {
NETPLAN_OPTIONAL_DHCP4 = 1<<2,
NETPLAN_OPTIONAL_DHCP6 = 1<<3,
NETPLAN_OPTIONAL_STATIC = 1<<4,
NETPLAN_OPTIONAL_IPV4 = 1<<5,
NETPLAN_OPTIONAL_IPV6 = 1<<6,
Comment on lines 32 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we shouldn't introduce new IPV4 & IPv6 flags, but rather extend "_STATIC" to "_STATIC4" and "_STATIC6". But the "static4" and "static6" filters should just be internally and should not propagate to the YAML.

The "_IPv4" filter is then just a shortcut for IPV4_LL + DHCP4 + STATIC4.
The "_IPv6" filter is then just a shortcut for IPV6_RA (should this actually be called IPV6_LL?!) + DHCP6 + STATIC6.

NETPLAN_OPTIONAL_NONE = 1<<7,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the "none" case implicitly, if the bitfield == 0x0

} NetplanOptionalAddressFlag;

/* Fields below are valid for dhcp4 and dhcp6 unless otherwise noted. */
Expand Down
6 changes: 6 additions & 0 deletions src/netplan.c
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,12 @@ _serialize_yaml(
YAML_SCALAR_PLAIN(event, emitter, "dhcp6")
if (def->optional_addresses & NETPLAN_OPTIONAL_STATIC)
YAML_SCALAR_PLAIN(event, emitter, "static")
if (def->optional_addresses & NETPLAN_OPTIONAL_IPV4)
YAML_SCALAR_PLAIN(event, emitter, "ipv4")
if (def->optional_addresses & NETPLAN_OPTIONAL_IPV6)
YAML_SCALAR_PLAIN(event, emitter, "ipv6")
if (def->optional_addresses & NETPLAN_OPTIONAL_NONE)
YAML_SCALAR_PLAIN(event, emitter, "none")
YAML_SEQUENCE_CLOSE(event, emitter);
}

Expand Down
15 changes: 6 additions & 9 deletions src/networkd.c
Original file line number Diff line number Diff line change
Expand Up @@ -739,15 +739,12 @@ netplan_netdef_write_network_file(
is_optional = TRUE;
}

if (is_optional || def->optional_addresses) {
if (is_optional) {
g_string_append(link, "RequiredForOnline=no\n");
}
for (unsigned i = 0; NETPLAN_OPTIONAL_ADDRESS_TYPES[i].name != NULL; ++i) {
if (def->optional_addresses & NETPLAN_OPTIONAL_ADDRESS_TYPES[i].flag) {
g_string_append_printf(link, "OptionalAddresses=%s\n", NETPLAN_OPTIONAL_ADDRESS_TYPES[i].name);
}
}
if (is_optional) {
g_string_append(link, "RequiredForOnline=no\n");
}
Comment on lines +742 to +744
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. This is the easiest case, and basically a shortcut for IPV4_LL + DHCP4 + STATIC4 + IPV6_RA/LL + DHCP6 + STATIC6.


if (def->optional_addresses & NETPLAN_OPTIONAL_NONE) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (def->optional_addresses & NETPLAN_OPTIONAL_NONE) {
if (def->optional_addresses == 0x0) {

g_string_append(link, "RequiredFamilyForOnline=both\n");
}

if (def->mtubytes)
Expand Down
9 changes: 9 additions & 0 deletions src/nm.c
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,15 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir,
}
}

if (def->optional_addresses & NETPLAN_OPTIONAL_NONE) {
g_key_file_set_string(kf, "ipv4", "may-fail", "false");
g_key_file_set_string(kf, "ipv6", "may-fail", "false");
} else {
if (def->optional_addresses & NETPLAN_OPTIONAL_IPV4)
g_key_file_set_string(kf, "ipv4", "may-fail", "true");
if (def->optional_addresses & NETPLAN_OPTIONAL_IPV6)
g_key_file_set_string(kf, "ipv6", "may-fail", "true");
}
/* NM connection files might contain secrets, and NM insists on tight permissions */
full_path = g_strjoin(G_DIR_SEPARATOR_S, rootdir ?: "", conf_path, NULL);
orig_umask = umask(077);
Expand Down
12 changes: 12 additions & 0 deletions src/parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -1530,6 +1530,10 @@ NETPLAN_OPTIONAL_ADDRESS_TYPES[] = {
{"dhcp4", NETPLAN_OPTIONAL_DHCP4},
{"dhcp6", NETPLAN_OPTIONAL_DHCP6},
{"static", NETPLAN_OPTIONAL_STATIC},
/* All above are deprecated */
{"ipv4", NETPLAN_OPTIONAL_IPV4},
{"ipv6", NETPLAN_OPTIONAL_IPV6},
{"none", NETPLAN_OPTIONAL_NONE},
{NULL},
};

Expand All @@ -1543,6 +1547,14 @@ handle_optional_addresses(NetplanParser* npp, yaml_node_t* node, const void* _,

for (unsigned i = 0; NETPLAN_OPTIONAL_ADDRESS_TYPES[i].name != NULL; ++i) {
if (g_ascii_strcasecmp(scalar(entry), NETPLAN_OPTIONAL_ADDRESS_TYPES[i].name) == 0) {

/* Values below NETPLAN_OPTIONAL_STATIC (including it) are not valid and
* considered deprecated. See LP: #1880029
*/
if (NETPLAN_OPTIONAL_ADDRESS_TYPES[i].flag <= NETPLAN_OPTIONAL_STATIC)
g_warning("Flag \"%s\" in optional-addresses is deprecated. Valid values are: "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't deprecate the original keywords, but rather try to get them implemented in the corresponding backend renderers.

That is especially:

  • Waiting LL configuration only
    • This can already be achieved on networkd, by using RequiredForOnline=degraded (instead of routable/yes). Can be in combination with RequiredFamilyForOnline=ipv4/ipv6. – But what happens if we want to have static or dynamic IP4 but only LL IP6?
  • Waiting for static IP configuration, but not dynamic/DHCP.

"\"ipv4\", \"ipv6\" and \"none\"\n", NETPLAN_OPTIONAL_ADDRESS_TYPES[i].name);

npp->current.netdef->optional_addresses |= NETPLAN_OPTIONAL_ADDRESS_TYPES[i].flag;
found = TRUE;
break;
Expand Down
2 changes: 1 addition & 1 deletion tests/generator/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ def get_network_config_for_link(self, link_name):
def get_optional_addresses(self, eth_name):
config = self.get_network_config_for_link(eth_name)
r = set()
prefix = "OptionalAddresses="
prefix = "RequiredFamilyForOnline="
for line in config.splitlines():
if line.startswith(prefix):
r.add(line[len(prefix):])
Expand Down
71 changes: 67 additions & 4 deletions tests/generator/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,26 @@ def config_with_optional_addresses(self, eth_name, optional_addresses):

def test_optional_addresses(self):
eth_name = self.eth_name()
self.generate(self.config_with_optional_addresses(eth_name, '["dhcp4"]'))
self.assertEqual(self.get_optional_addresses(eth_name), set(["dhcp4"]))
self.generate(self.config_with_optional_addresses(eth_name, '["ipv4"]'))
self.assertEqual(self.get_optional_addresses(eth_name), set())

def test_optional_addresses_multiple(self):
eth_name = self.eth_name()
self.generate(self.config_with_optional_addresses(eth_name, '[dhcp4, ipv4-ll, ipv6-ra, dhcp6, dhcp4, static]'))
self.generate(self.config_with_optional_addresses(eth_name, '["ipv4", "ipv6", "ipv4"]'))
self.assertEqual(
self.get_optional_addresses(eth_name),
set(["ipv4-ll", "ipv6-ra", "dhcp4", "dhcp6", "static"]))
set())

def test_optional_addresses_none(self):
eth_name = self.eth_name()
self.generate(self.config_with_optional_addresses(eth_name, '["none"]'))
self.assertEqual(self.get_optional_addresses(eth_name), set(["both"]))

def test_optional_addresses_deprecated_flags(self):
flags = '["ipv4-ll", "ipv6-ra", "dhcp4", "dhcp6", "static"]'
eth_name = self.eth_name()
self.generate(self.config_with_optional_addresses(eth_name, flags))
self.assertEqual(self.get_optional_addresses(eth_name), set())

def test_optional_addresses_invalid(self):
eth_name = self.eth_name()
Expand Down Expand Up @@ -1322,6 +1333,58 @@ def test_nameserver(self):
method=ignore
'''})

def test_optional_addresses(self):
self.generate('''network:
version: 2
renderer: NetworkManager
ethernets:
eth0:
optional-addresses: [ipv4, ipv6]''')

self.assert_nm({'eth0': '''[connection]
id=netplan-eth0
type=ethernet
interface-name=eth0

[ethernet]
wake-on-lan=0

[ipv4]
method=link-local
may-fail=true

[ipv6]
method=ignore
may-fail=true
'''})
self.assert_networkd({})

def test_optional_addresses_none(self):
self.generate('''network:
version: 2
renderer: NetworkManager
ethernets:
eth0:
optional-addresses: [none]''')

self.assert_nm({'eth0': '''[connection]
id=netplan-eth0
type=ethernet
interface-name=eth0

[ethernet]
wake-on-lan=0

[ipv4]
method=link-local
may-fail=false

[ipv6]
method=ignore
may-fail=false
'''})
self.assert_networkd({})


class TestForwardDeclaration(TestBase):

Expand Down