network: introduce DAD for static IPV4 address #14102

ssahani · 2019-11-21T16:25:35Z

yuwata · 2019-11-24T10:36:42Z

@ssahani LGTM. I have adjusted log level and log message, and also fixed an memory use-after-free. Change can see from here. PTAL.

yuwata · 2019-11-24T10:57:38Z

(v244-rc1 was released. Let's merge this after v244 release.)

ssahani · 2019-11-24T11:45:30Z

Thank you. LGTM

yuwata · 2019-11-26T15:59:45Z

A test case is also added.

Pesa · 2019-12-04T05:51:47Z

man/systemd.network.xml

          </listitem>
        </varlistentry>
+        <varlistentry>
+          <term><varname>IPv4DuplicateAddressDetection=</varname></term>


is yet another option really needed? we already have DuplicateAddressDetection=, currently only used for IPv6. It could be generalized to apply to both address families: use IPv4 ACD if the address is v4, use IPv6 DAD if the address is v6.

Sounds nice. Thanks. I'd like to update so. @ssahani Do you agree with that?

I noticed that DuplicateAddressDetection= is controversial. If set to yes, then NODAD flag is set. Maybe, we should deprecate the setting, and introduce IPv6DuplicateAddressDetection=.

Yes . It works for IPv6. We set this and kernel does the DAD. Mixing up will not work. Renaming indeed is nice.

oh, I didn't realize DuplicateAddressDetection= logic was inverted. Definitely surprising and user-unfriendly.

Unfortunately, there is already a IPv6DuplicateAddressDetection= option in the [Network] section. Could it be a source of confusion to have the same option name in different sections with different semantics?

yuwata · 2019-12-04T10:49:23Z

@ssahani and @Pesa PTAL the last commit.

yuwata · 2019-12-04T10:51:50Z

Hmm... I noticed that PrefixRoute= is also inverted... But let's fix that in another PR.

yuwata · 2019-12-04T14:35:54Z

@ssahani I added one more commit. PTAL.

ssahani · 2019-12-04T15:49:09Z

LGTM

Pesa · 2019-12-04T16:21:02Z

src/network/networkd-address.c

                                         address->section->filename, address->section->line);
        }

+        if (address->family != AF_INET && address->ipv4_duplicate_address_detection) {


should there be a similar check for ipv6 DAD on non-ipv6 addresses?

NODAD flag for IPv4 address should be silently ignored by kernel. So, I think it is not necessary.

yuwata · 2019-12-05T09:48:31Z

@keszybz Could you take a final look?

keszybz · 2019-12-05T11:03:46Z

man/systemd.network.xml

+          <term><varname>IPv4DuplicateAddressDetection=</varname></term>
+          <listitem>
+            <para>Takes a boolean. When true performs IPv4 Duplicate Address Detection. See
+            <ulink url="https://tools.ietf.org/html/rfc5227">RFC 5224</ulink>. Defaults to false.</para>


No, let's not add yet another option. If users wants DAD, then most likely they want both ipv4 and ipv6, and don't care about internal implementation details, i.e. whether this is the kernel that is doing this or the user space.

Let's do the following: make DuplicateAddressDetection take: yes|no, "both", "ipv4", "ipv6". If yes|no, then warn about the inverted meaning and set according to current semantics. If "ipv4", enable for IPv4, if "ipv6", enable for IPv6, if "both" enable for both.

This is easier for the users, and will give us more flexibility to add new variants of this setting in the future.

keszybz · 2019-12-05T11:05:11Z

src/network/networkd-address.c

+        switch (event) {
+
+        case SD_IPV4ACD_EVENT_STOP:
+                log_link_debug(link, "Stopping ACD client ...");


No space before the ellipsis. (Right now it looks like the ellipsis is a placeholder for a client name.)

keszybz · 2019-12-05T11:05:57Z

src/network/networkd-address.c

+                return;
+
+        case SD_IPV4ACD_EVENT_BIND:
+                log_link_debug(link, "Successfully claimed address '%s'", strna(pretty));


No quotes around the address.

src/network/networkd-address.c

yuwata · 2019-12-06T10:39:28Z

@keszybz Thank you for the comments. Updated. PTAL.

keszybz

Looks pretty good, except for the backwards compat part (which I may be misunderstanding, so please be patient).

src/network/networkd-address.c

yuwata · 2019-12-06T14:16:32Z

@keszybz Updated. Please take another look. Thank you.

keszybz

Almost there.

src/network/networkd-address.c

keszybz · 2019-12-06T14:25:38Z

src/network/networkd-address.c

+        r = parse_boolean(rvalue);
+        if (r >= 0) {
+                log_syntax(unit, LOG_WARNING, filename, line, 0,
+                           "For historical reason, %s=%s means %s=%s. "


Closes systemd#2527.

yuwata · 2019-12-06T14:31:36Z

@keszybz Thank you for the review. The two comments are addressed now. I've updated the green label.

ssahani force-pushed the acd-duplicate-ip branch 4 times, most recently from 0d5f385 to 80b9e0d Compare November 22, 2019 03:12

ssahani changed the title ~~network: introduce DAD for static address~~ network: introduce DAD for static IPV4 address Nov 22, 2019

yuwata added the network label Nov 22, 2019

yuwata force-pushed the acd-duplicate-ip branch from 80b9e0d to d1af242 Compare November 24, 2019 10:33

yuwata added this to the v245 milestone Nov 24, 2019

yuwata added the postponed label Nov 24, 2019

yuwata added new-feature and removed postponed labels Nov 24, 2019

yuwata added the good-to-merge/after-next-release label Nov 24, 2019

yuwata force-pushed the acd-duplicate-ip branch 2 times, most recently from 49ba008 to 78ab099 Compare November 26, 2019 15:56

Pesa reviewed Dec 4, 2019

View reviewed changes

yuwata removed the good-to-merge/after-next-release label Dec 4, 2019

yuwata force-pushed the acd-duplicate-ip branch from 78ab099 to 05a3192 Compare December 4, 2019 10:48

yuwata force-pushed the acd-duplicate-ip branch from 05a3192 to 909ea24 Compare December 4, 2019 11:18

Pesa reviewed Dec 4, 2019

View reviewed changes

keszybz requested changes Dec 5, 2019

View reviewed changes

yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 5, 2019

yuwata force-pushed the acd-duplicate-ip branch from f72f1d9 to ff47b67 Compare December 6, 2019 10:37

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 6, 2019

yuwata force-pushed the acd-duplicate-ip branch from ff47b67 to d87db44 Compare December 6, 2019 11:12

keszybz reviewed Dec 6, 2019

View reviewed changes

src/network/networkd-address.c Outdated Show resolved Hide resolved

keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 6, 2019

yuwata force-pushed the acd-duplicate-ip branch from d87db44 to 35d7be7 Compare December 6, 2019 14:13

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 6, 2019

keszybz reviewed Dec 6, 2019

View reviewed changes

keszybz added the good-to-merge/with-minor-suggestions label Dec 6, 2019

ssahani and others added 2 commits December 6, 2019 23:29

network: introduce DAD for static address

051e77c

Closes systemd#2527.

test-network: add test case for IPv4 DAD

dc7d3c5

yuwata force-pushed the acd-duplicate-ip branch from 35d7be7 to dc7d3c5 Compare December 6, 2019 14:29

yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Dec 6, 2019

yuwata merged commit 43a2005 into systemd:master Dec 7, 2019

ssahani deleted the acd-duplicate-ip branch March 27, 2020 02:40

slyon mentioned this pull request Oct 24, 2024

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

Open

5 tasks

Uh oh!

network: introduce DAD for static IPV4 address #14102

network: introduce DAD for static IPV4 address #14102

Uh oh!

Conversation

ssahani commented Nov 21, 2019

Uh oh!

yuwata commented Nov 24, 2019

Uh oh!

yuwata commented Nov 24, 2019

Uh oh!

ssahani commented Nov 24, 2019

Uh oh!

yuwata commented Nov 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Dec 4, 2019

Uh oh!

yuwata commented Dec 4, 2019

Uh oh!

yuwata commented Dec 4, 2019

Uh oh!

ssahani commented Dec 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Dec 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuwata commented Dec 6, 2019

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuwata commented Dec 6, 2019

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Dec 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants