Skip to content

Disable unsupported IPv4/IPv6 methods for wireguard#19337

Merged
martinpitt merged 2 commits intocockpit-project:mainfrom
subhoghoshX:disable-ip-methods
Oct 13, 2023
Merged

Disable unsupported IPv4/IPv6 methods for wireguard#19337
martinpitt merged 2 commits intocockpit-project:mainfrom
subhoghoshX:disable-ip-methods

Conversation

@subhoghoshX
Copy link
Contributor

@subhoghoshX subhoghoshX commented Sep 15, 2023

Follow-up from #19024

@subhoghoshX subhoghoshX mentioned this pull request Sep 15, 2023
Merged
6 tasks
jelly
jelly reviewed Sep 15, 2023
View reviewed changes
pkg/networkmanager/ip-settings.jsx Outdated
const unsupported_ipv6_method_values = ['auto', 'dhcp', 'shared'];
ipv4_method_choices = ipv4_method_choices.filter(item => !unsupported_ipv4_method_values.includes(item.choice));
ipv6_method_choices = ipv6_method_choices.filter(item => !unsupported_ipv6_method_values.includes(item.choice));
}
Copy link
Member

@jelly jelly Sep 15, 2023

Choose a reason for hiding this comment

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

Changing constants on runtime which are exported feels very brittle.

Copy link
Member

@martinpitt martinpitt Sep 16, 2023

Choose a reason for hiding this comment

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

Can these be turned into a function ipv4_method_choices(device_type)?

Copy link
Contributor Author

@subhoghoshX subhoghoshX Sep 21, 2023

Choose a reason for hiding this comment

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

Using a function indeed sounds better, thanks! I see that the IPv4 methods are a subset of the IPv6 methods:

export let ipv4_method_choices =
    [
        { choice: 'auto', title: _("Automatic (DHCP)") },
        { choice: 'link-local', title: _("Link local") },
        { choice: 'manual', title: _("Manual") },
        { choice: 'shared', title: _("Shared") },
        { choice: 'disabled', title: _("Disabled") }
    ];

export let ipv6_method_choices =
    [
        { choice: 'auto', title: _("Automatic") },
        { choice: 'dhcp', title: _("Automatic (DHCP only)") },
        { choice: 'link-local', title: _("Link local") },
        { choice: 'manual', title: _("Manual") },
        { choice: 'ignore', title: _("Ignore") },
        { choice: 'shared', title: _("Shared") },
        { choice: 'disabled', title: _("Disabled") }
    ];

Only difference is "auto" translates to "Automatic (DHCP)" is IPv4 and to just "Automatic" in IPv6. The value "auto" in both IPv4/6 seems to be exactly the same. From NM doc:

NMSettingIP4Config and NMSettingIP6Config both support "disabled", "auto", "manual", and "link-local"...

If we use the same text for "auto" in both IPv4/IPv6, we can also merge the list into one while we're at it...

Copy link
Member

@martinpitt martinpitt Sep 26, 2023

Choose a reason for hiding this comment

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

yeah, let's use "Automatic (DHCP)" for both, that'll reduce confusion and i18n overhead. Then you can even have a single function with an "ipv6?" flag argument.

Copy link
Member

@martinpitt martinpitt Sep 29, 2023

Choose a reason for hiding this comment

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

Sorry, I think that was bad advice. "Auto" for IPv6 is often not DHCP, but SLAAC, or possibly some other methods. That's why it has a separate "dhcp" mode. So we may be able to do it the other way around and name both "Automatic", but when in doubt, let's just leave them as-is?

Copy link
Contributor Author

@subhoghoshX subhoghoshX Sep 29, 2023

Choose a reason for hiding this comment

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

Thanks for the feedback @martinpitt! I did some research into this and it indeed looks like "auto" in IPv6 doesn't mean DHCP. TIL.

In case of IPv6 it says here:

If "auto" is specified then the appropriate automatic method (PPP, router advertisement, etc) is used for the device

Similar mentioned here as well, which looks like part of a documentation, but I can't find the rendered version.

I also looked into IPv4, and it says:

Enables automatic IPv4 address assignment from DHCP, PPP, or similar services.

Source: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/main/src/libnm-core-impl/nm-setting-ip4-config.c?ref_type=heads#L1285

So, IPv4 "auto" is more than just DHCP it seems, then your second suggestion of dropping (DHCP) from IPv4, might be the right way to go.

WDYT?

Copy link
Member

@martinpitt martinpitt Sep 29, 2023

Choose a reason for hiding this comment

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

Works for me -- cross check, @mvollmer ?

Copy link
Member

@mvollmer mvollmer Oct 10, 2023

Choose a reason for hiding this comment

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

I think "DHCP" is a very familiar term and will reassure people that they are getting what they think they are getting. But if it is only correct for certain types of interfaces, I don't mind dropping it.

@subhoghoshX
Copy link
Contributor Author

subhoghoshX commented Sep 27, 2023

This needs a pixel update...

@subhoghoshX subhoghoshX force-pushed the disable-ip-methods branch 5 times, most recently from ffb83c6 to b536a0c Compare October 12, 2023 11:56
martinpitt
martinpitt requested changes Oct 12, 2023
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! This looks very reasonable to me now. Strictly speaking, the pixel update should be on the first commit (which drops the "(DHCP)" from the name), not the second one. But if that's too much hassle to rebase, don't worry.

@subhoghoshX
networking: merge ipv4 and ipv6 method choices
54a9280 
This removes duplicates and reduce i18n overhead. Also drop (DHCP) from IPv4 option "Automtic (DHCP)" in the process. Adjust tests accordingly.
@subhoghoshX
networking: Disable unsupported IPv4/IPv6 methods for wireguard
b99c29c 
NM only supports a subset of IPv4/IPv6 methods for wireguard, hide them
from the UI altogether, instead of showing an error when they are
selected.
@subhoghoshX
Copy link
Contributor Author

subhoghoshX commented Oct 12, 2023

Moved the pixel update to the proper commit. Thanks for pointing it out @martinpitt.

The tests are green (s390x is still going though).

Some flakes I encountered today that might be of interest :)

TestFirewall.testFirewallPage

TestFirewall.testNetworkingPage

TestHistoryMetrics.testEvents

@martinpitt
Copy link
Member

martinpitt commented Oct 13, 2023

Thanks @subhoghoshX The metrics pixel failure has been a long-standing one; @mvollmer already brooded over it for a long time, and there's still some vestiges.

Thanks for the firewall links, I add them to my list of flakes to look into.

martinpitt
martinpitt approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinpitt martinpitt merged commit 688a044 into cockpit-project:main Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jelly jelly jelly left review comments

@mvollmer mvollmer mvollmer left review comments

@martinpitt martinpitt martinpitt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@subhoghoshX @martinpitt @jelly @mvollmer