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

daniloegea · 2023-04-04T17:29:55Z

Description

The Netplan's key "optional-addresses" only generates configuration for systemd-networkd. Although, the generated "OptionalAddresses" key doesn't exist so this property never worked on Netplan.

This PR re-implements the key "optional-addresses" to emit configuration for network manager, using the "may-fail" key, and for networkd, using the "RequiredFamilyForOnline" key.

Note that RequiredFamilyForOnline is actually the apposite of "optional addresses". It defaults to "any", meaning that either ipv4 or ipv6 will be enough to set the interface as online. Because of that, the only time when Netplan will generate RequiredFamilyForOnline is when the user enters "none", meaning that none of the addresses are optional, so RequiredFamilyForOnline=both will be emitted.

For Network Manager, optional-addresses never generated configuration. Now, Netplan will emit the key "may-fail" for ipv4 and/or ipv6 according to the configuration.

All the old values accepted by optional-addresses are no-ops, so if any users out there have this configuration in their YAML files, nothing will break.

These changes seem to not be a perfect fit based on our documentation, but I guess it's close 😅

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).
New/changed keys in YAML format are documented.
(Optional) Adds example YAML for new feature.
(Optional) Closes an open bug in Launchpad.

optional-addresses is generating invalid configuration for systemd-networkd. The option OptionalAddresses doesn't really exist. It doesn't emit any configuration for Network Manager. This changes reimplement optional-addresses using systemd's "RequiredFamilyForOnline" and NetworkManager's "may-fail". It keeps all the old options, for backwards compatibility, as noops, as they were before in practice, and introduce 3 new options: "ipv4", "ipv6" and "none". On systemd, RequiredFamilyForOnline defaults to "any", meaning that any interface that gets an IP will be enough to make the system online. Netplan will emit configuration for systemd only when the value of optional-addresses is "none", meaning that nothing is optional. In this case, RequiredFamilyForOnline will have the value "both". On Network Manager, it will emit "may-fail" as true or false according to the configuration. It will still allow the user to have the old values so we will not break systems out there. A warning message will emitted is this case. It resolves LP: #1880029

tests/generator/test_common.py

slyon · 2023-04-13T13:22:27Z

Okay.. this is a complex beast and some functionality seems to be lacking in the backend renderers (networkd & Networkmanager), too, like checking status of DHCP4/6, IP4LL/IP6RA or static IP4/6 addresses independently of the others. Both backends can handle the combined IP4/IP6 address families, using the RequiredFamilyForOnline=ipv4/ipv6 or ipv4/ipv6.may-fail=BOOL settings.

We should compare this re-implementation against the "network-online.target" spec (https://discourse.ubuntu.com/t/spec-definition-of-an-online-system/27838) and the internal email discussion we had in May/June 2022 ("Topic: Jammy based image failing due to systemd-networkd-wait-online changes"). I need some more time to investigate and connect all the dots...

Overall, I think this needs more work, but a few things that already came up during my initial review are:

The none value should vanish. Instead, we should rely on an empty optional-addresses list ([]) being "none" implicitly.
~~The new "ipv4"/"ipv6" values might be in conflict with the existing values (but I need to think about this harder)~~
We should not deprecate the old values, but rather try to get them implemented in the corresponding backend renderers, so we can make use of it in the future. Maybe we could print a "Not yet implemented" warning for the time being (although that's embarrassing), to warn people about this being a NO-OP currently.

Edit: I've created a spreadsheet to compare the different input & output values:
https://docs.google.com/spreadsheets/d/1bb9ire89WWQ0KHX-8fTKyg3fW06dgUf2az6B7emqXCQ

slyon

A few remarks to start with:

We should probably re-organize the internal flags/filters a little:
- "ipv4-ll" = NETPLAN_OPTIONAL_IPV4_LL
- "ipv6-ll" = NETPLAN_OPTIONAL_IPv6_LL
- "dhcp4" = NETPLAN_OPTIONAL_DHCP4
- "dhcp6" = NETPLAN_OPTIONAL_DHCP6
- "static" = NETPLAN_OPTIONAL_STATIC4 & NETPLAN_OPTIONAL_STATIC6
"ipv6-ra" should probably be re-named to "ipv6-ll". RA doesn't really make sense IMO, as we also have "dhcp6", which is kind of related.
"static" should activate two internal filters, for finer control: NETPLAN_OPTIONAL_STATIC4 and NETPLAN_OPTIONAL_STATIC6 (in favor of the _STATIC flag)

Input variables:
We have multiple input variables that we need to consider when making a decision about "optional-addresses". Any address defined in the YAML is non-optional by default.

IPv4 & IPv6 LL addresses, as defined in the link-local: setting. ([ ipv6 ] by default)
IPv4 & IPv6 static addresses, as defined in the addresses: setting (IP address family is auto-detected).
IPv4 & IPv6 dynamic addresses, as defined in the dhcp4: and dhcp6: (possibly in combination with accept-ra:) settings.

Considering the optional-addresses: schema:
Depending on the input variables (address types defined in the YAML), optional-addresses can be used to "filter out" some types, if we want to have them defined, but not block on them.

Basically, we have 4 levels of filters:

internal flags: NETPLAN_OPTIONAL_IPV4_LL, NETPLAN_OPTIONAL_IPv6_LL, NETPLAN_OPTIONAL_DHCP4, NETPLAN_OPTIONAL_DHCP6, NETPLAN_OPTIONAL_STATIC4, NETPLAN_OPTIONAL_STATIC6
YAML flags: ipv4-ll, ipv6-ra/ll, dhcp4, dhcp6, static
(YAML shortcuts: ipv4, ipv6)
- ipv4 == IPV4_LL + DHCP4 + STATIC4
- ipv6 == IPV6_RA/LL + DHCP6 + STATIC6
- I'm not yet sure if we should introduce those additional shortcuts, it might make things more complicated, but we can keep them here during implementation as a "reminder".
YAML shortcut: optional: true
- activates all the filters for this interface

So, overall I think we should keep the YAML schema for optional-addresses as-is, but split up the internal flags, to enable more fine-grained control. We should then introduce some helpers:

wants_ipv4_static(addresses, optional_addresses_flags) -> bool
wants_ipv6_static(addresses, optional_addresses_flags) -> bool
wants_ipv4_ll(link_local, optional_addresses_flags) -> bool
wants_ipv6_ll(link_local, optional_addresses_flags) -> bool
wants_ipv4_dynamic(dhcp4, optional_addresses_flags) -> bool
wants_ipv6_dynamic(dhcp6, [accept_ra,] optional_addresses_flags) -> bool

Building upon those filters we should then try to generate the corresponding output values for RequiredForOnline=routable/yes/degraded/no / RequiredFamilyForOnline=ipv6/ipv6/both/any (networkd) and ipv4.may-fail + ipv6.may-fail (NetworkManager) wherever possible, and print a warning for the cases that cannot be covered yet, due to backend limitations.

slyon · 2023-04-27T14:58:54Z

src/abi.h

@@ -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,
+    NETPLAN_OPTIONAL_NONE    = 1<<7,


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

slyon · 2023-04-27T15:02:12Z

src/abi.h

    NETPLAN_OPTIONAL_STATIC  = 1<<4,
+    NETPLAN_OPTIONAL_IPV4    = 1<<5,
+    NETPLAN_OPTIONAL_IPV6    = 1<<6,


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.

slyon · 2023-04-27T15:04:36Z

src/networkd.c

+    if (is_optional) {
+        g_string_append(link, "RequiredForOnline=no\n");
+    }


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

slyon · 2023-04-27T15:05:32Z

src/networkd.c

+        g_string_append(link, "RequiredForOnline=no\n");
+    }
+
+    if (def->optional_addresses & NETPLAN_OPTIONAL_NONE) {


Suggested change

if (def->optional_addresses & NETPLAN_OPTIONAL_NONE) {

if (def->optional_addresses == 0x0) {

slyon · 2023-04-27T15:11:44Z

src/parse.c

+                 * 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: "


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.

daniloegea added 3 commits April 3, 2023 19:01

docs: update optional-addresses entry

8da4f64

tests: fix tests after optional-addresses changes

84fc04a

daniloegea commented Apr 4, 2023

View reviewed changes

tests/generator/test_common.py Show resolved Hide resolved

daniloegea requested a review from slyon April 4, 2023 23:18

slyon requested changes Apr 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

daniloegea commented Apr 4, 2023 •

edited by slyon

Loading

slyon commented Apr 13, 2023 •

edited

Loading

slyon left a comment

slyon Apr 27, 2023

slyon Apr 27, 2023

slyon Apr 27, 2023

slyon Apr 27, 2023

slyon Apr 27, 2023

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

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

Are you sure you want to change the base?

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

Conversation

daniloegea commented Apr 4, 2023 • edited by slyon Loading

Description

Checklist

slyon commented Apr 13, 2023 • edited Loading

slyon left a comment

Choose a reason for hiding this comment

slyon Apr 27, 2023

Choose a reason for hiding this comment

slyon Apr 27, 2023

Choose a reason for hiding this comment

slyon Apr 27, 2023

Choose a reason for hiding this comment

slyon Apr 27, 2023

Choose a reason for hiding this comment

slyon Apr 27, 2023

Choose a reason for hiding this comment

daniloegea commented Apr 4, 2023 •

edited by slyon

Loading

slyon commented Apr 13, 2023 •

edited

Loading