Skip to content
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

Add DuplicateAddressDetection settings for systemd-networkd (LP: #1959190) #525

Open
wants to merge 1 commit 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
8 changes: 8 additions & 0 deletions doc/netplan-yaml.md
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,12 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.
> In addition to the addresses themselves one can specify configuration
> parameters as mappings. Current supported options are:

- **`duplicate-address-detection`** (scalar)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the duplicate-address-detection naming. There's also the notion of "IPv4 Address Conflict Detection" (ACD) [RFC 5227], but I have the impression that "Duplicate Address Detection" (DAD) is used commonly across IPV4 & IPv6 these days.

See

Copy link
Collaborator

@slyon slyon Oct 24, 2024

Choose a reason for hiding this comment

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

nitpick (non-blocking):

Suggested change
- **`duplicate-address-detection`** (scalar)
- **`duplicate-address-detection`** (scalar) – since 1.2


> Configure the duplicate address detection (DAD). Valid options
> are `ipv4`, `ipv6`, `both` or `none`. Currently supported on the
> networkd back end only.
Comment on lines +427 to +429
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is currently a direct copy of the networkd settings. I'd rather like to represent this as a sequence/list of flags, though. Maybe on the global level instead of per-address.

e.g.

ethernets:
  eth0:
    addresses: [169.254.0.13/16]
    dhcp6: true
    duplicate-address-detection: [ipv4-ll, ipv6]

The current default could be represented as [ipv4-ll, ipv6], we could have "both" [ipv4, ipv6] or "none" []. ipv4 should probably include ipv4-ll implicitly.

On a per-IP level it does not alway make sense, or is there any use case for this? E.g:

ethernets:
  eth0:
    addresses:
    - 169.254.0.13/16:
      duplicate-address-detection: "ipv6"
    dhcp6: true

Copy link
Author

Choose a reason for hiding this comment

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

On a per-IP level it does not alway make sense, or is there any use case for this? E.g: [...]

I don't think this it makes sense too to have this configuration

But thinking of weird corner cases, I don't know why someone would do that, but it still is a valid configuration for networkd, that cannot be reproduced with nm afaik.

ethernets:
  eth0:
    addresses:
    - 172.16.0.1/24:
      duplicate-address-detection: "ipv4"
    - 169.254.0.13/16:
      duplicate-address-detection: "none"
    dhcp6: true


- **`lifetime`** (scalar) – since 0.100

> Default: `forever`. This can be `forever` or `0` and corresponds
Expand All @@ -445,6 +451,8 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.
- "10.0.0.15/24":
lifetime: 0
label: "maas"
- "169.254.10.1/24":
duplicate-address-detection: "none"
- "2001:1::1/64"
```

Expand Down
1 change: 1 addition & 0 deletions python-cffi/netplan/_build_cffi.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
char* address;
char* lifetime;
char* label;
char* duplicate_address_detection;
} NetplanAddressOptions;
struct address_iter { ...; };
struct nameserver_iter { ...; };
Expand Down
7 changes: 5 additions & 2 deletions python-cffi/netplan/netdef.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,11 @@ def __next__(self):


class NetplanAddress:
def __init__(self, address: str, lifetime: str, label: str):
def __init__(self, address: str, lifetime: str, label: str, duplicate_address_detection: str):
self.address = address
self.lifetime = lifetime
self.label = label
self.duplicate_address_detection = duplicate_address_detection

def __str__(self) -> str:
return self.address
Expand All @@ -241,7 +242,9 @@ def __next__(self):
address = ffi.string(content.address).decode('utf-8') if content.address else None
lifetime = ffi.string(content.lifetime).decode('utf-8') if content.lifetime else None
label = ffi.string(content.label).decode('utf-8') if content.label else None
return NetplanAddress(address, lifetime, label)
duplicate_address_detection = ffi.string(content.duplicate_address_detection).decode('utf-8') \
if content.duplicate_address_detection else None
return NetplanAddress(address, lifetime, label, duplicate_address_detection)


class _NetdefNameserverIterator:
Expand Down
1 change: 1 addition & 0 deletions src/netplan.c
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,7 @@ write_addresses(yaml_event_t* event, yaml_emitter_t* emitter, const NetplanNetDe
YAML_MAPPING_OPEN(event, emitter);
YAML_NONNULL_STRING(event, emitter, "label", opts->label);
YAML_NONNULL_STRING(event, emitter, "lifetime", opts->lifetime);
YAML_NONNULL_STRING(event, emitter, "duplicate-address-detection", opts->duplicate_address_detection);
YAML_MAPPING_CLOSE(event, emitter);
YAML_MAPPING_CLOSE(event, emitter);
}
Expand Down
2 changes: 2 additions & 0 deletions src/networkd.c
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,8 @@ write_addr_option(NetplanAddressOptions* o, GString* s)
g_assert(o->address != NULL);
g_string_append_printf(s, "Address=%s\n", o->address);

if (o->duplicate_address_detection)
g_string_append_printf(s, "DuplicateAddressDetection=%s\n", o->duplicate_address_detection);
if (o->lifetime)
g_string_append_printf(s, "PreferredLifetime=%s\n", o->lifetime);
if (o->label)
Expand Down
13 changes: 13 additions & 0 deletions src/parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -1393,9 +1393,22 @@ handle_address_option_label(NetplanParser* npp, yaml_node_t* node, const void* d
return handle_generic_str(npp, node, npp->current.addr_options, data, error);
}

STATIC gboolean
handle_address_option_duplicate_address_detection(NetplanParser* npp, yaml_node_t* node, const void* data, GError** error)
{
if (g_ascii_strcasecmp(scalar(node), "ipv4") != 0 &&
g_ascii_strcasecmp(scalar(node), "ipv6") != 0 &&
g_ascii_strcasecmp(scalar(node), "both") != 0 &&
g_ascii_strcasecmp(scalar(node), "none") != 0) {
return yaml_error(npp, node, error, "invalid duplicate-address-detection value '%s'", scalar(node));
}
return handle_generic_str(npp, node, npp->current.addr_options, data, error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rather store this in a guint and represent it as an enum... Maybe something along those lines?

typedef enum {
    NETPLAN_DAD_UNSET = 0,
    NETPLAN_DAD_BOTH = 1<<0,
    NETPLAN_DAD_NONE = 1<<1,
    NETPLAN_DAD_IPV4LL = 1<<2,
    NETPLAN_DAD_IPV4 = 1<<3,
    NETPLAN_DAD_IPV6 = 1<<4,
    NETPLAN_DAD_MAX_,
} NetplanDadFlag;

The current (networkd) default could then be reflected as NETPLAN_DAD_IPV4LL | NETPLAN_DAD_IPV6

More flags could be added as needed in the future and the backend renderers (networkd.c / nm.c) would be able to handle only the flags that they actually understand individually, writing out the corresponding per-address or per-connection settings.

}

const mapping_entry_handler address_option_handlers[] = {
{"lifetime", YAML_SCALAR_NODE, {.generic=handle_address_option_lifetime}, addr_option_offset(lifetime)},
{"label", YAML_SCALAR_NODE, {.generic=handle_address_option_label}, addr_option_offset(label)},
{"duplicate-address-detection", YAML_SCALAR_NODE, {.generic=handle_address_option_duplicate_address_detection}, addr_option_offset(duplicate_address_detection)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this setting should move to COMMON_LINK_HANDLERS.

For network-manager, we can only control it on a per ipv4/ipv6 level:

Also, when defining it directly on a IPv4 address only none or ipv4 makes sense (boolean effectively, maybe "ipv4-ll" would be a special case), while when defining it directly on a IPv6 address, only none (false) or ipv6(true) makes sense. So I think it should be a per NetDef/interface setting, that way it could also describe dynamic addresses, like DHCP or IPv6RA.

{NULL}
};

Expand Down
1 change: 1 addition & 0 deletions src/types-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ typedef struct {
char* address;
char* lifetime;
char* label;
char* duplicate_address_detection;
} NetplanAddressOptions;

struct address_iter {
Expand Down
1 change: 1 addition & 0 deletions src/types.c
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ free_address_options(void* ptr)
g_free(opts->address);
g_free(opts->label);
g_free(opts->lifetime);
g_free(opts->duplicate_address_detection);
g_free(opts);
}

Expand Down
1 change: 1 addition & 0 deletions src/util.c
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,7 @@ _netplan_address_iter_next(struct address_iter* it)
options->address = g_strdup(netdef_options->address);
options->lifetime = g_strdup(netdef_options->lifetime);
options->label = g_strdup(netdef_options->label);
options->duplicate_address_detection = g_strdup(netdef_options->duplicate_address_detection);
it->last_address = options;
return options;
}
Expand Down
5 changes: 5 additions & 0 deletions tests/config_fuzzer/schemas/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,11 @@ export const common_properties = {
label: {
type: "string",
maxLength: 15,
},
"duplicate-address-detection": {
{
type: "string",
enum: ["ipv4", "ipv6", "both", "none"],
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions tests/generator/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,34 @@ def test_eth_address_option_label(self):
[Address]
Address=192.168.14.2/24
Label=test-label
'''})

def test_eth_address_option_duplicate_address_detection(self):
self.generate('''network:
version: 2
ethernets:
engreen:
addresses:
- 192.168.14.2/24:
duplicate-address-detection: ipv4
- 2001:FFfe::1/64:
duplicate-address-detection: none
- 10.0.0.1/24''')

self.assert_networkd({'engreen.network': '''[Match]
Name=engreen

[Network]
LinkLocalAddressing=ipv6
Address=10.0.0.1/24

[Address]
Address=192.168.14.2/24
DuplicateAddressDetection=ipv4

[Address]
Address=2001:FFfe::1/64
DuplicateAddressDetection=none
'''})

def test_eth_address_option_multi_pass(self):
Expand Down
10 changes: 10 additions & 0 deletions tests/generator/test_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,16 @@ def test_invalid_address_option_lifetime(self):
lifetime: 1''', expect_fail=True)
self.assertIn("invalid lifetime value '1'", err)

def test_invalid_address_option_duplicate_address_detection(self):
err = self.generate('''network:
version: 2
ethernets:
engreen:
addresses:
- 192.168.1.15/24:
duplicate-address-detection: a''', expect_fail=True)
self.assertIn("invalid duplicate-address-detection value 'a'", err)

def test_invalid_nm_options(self):
err = self.generate('''network:
version: 2
Expand Down
8 changes: 7 additions & 1 deletion tests/test_libnetplan.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,17 +200,23 @@ def test_iter_ethernets_with_options(self):
- 172.16.0.1/24:
lifetime: 0
label: label1
duplicate-address-detection: none
- 1234:4321:abcd::cdef/96:
lifetime: forever
label: label2''')
label: label2
duplicate-address-detection: both''')

expected_ips = set(["1234:4321:abcd::cdef/96", "192.168.0.1/24", "172.16.0.1/24"])
expected_lifetime_options = set([None, "0", "forever"])
expected_label_options = set([None, "label1", "label2"])
expected_duplicate_address_detection_options = set([None, "none", "both"])
netdef = next(netplan.netdef.NetDefinitionIterator(state, "ethernets"))
self.assertSetEqual(expected_ips, set(ip.address for ip in netdef.addresses))
self.assertSetEqual(expected_lifetime_options, set(ip.lifetime for ip in netdef.addresses))
self.assertSetEqual(expected_label_options, set(ip.label for ip in netdef.addresses))
self.assertSetEqual(
expected_duplicate_address_detection_options, set(ip.duplicate_address_detection for ip in netdef.addresses)
)

def test_drop_iterator_before_finishing(self):
state = state_from_yaml(self.confdir, '''network:
Expand Down
Loading