Allow to install an host with IPv6 management interface #2

benjamreis · 2022-09-15T08:18:12Z

Adds a screen to choose between IPv4, IPv6
and dual stack (with IPv4 as primary address type for management).

Write network conf file for IPv6 so that netinstall work in IPv6's modes

Configure the host with an IPv6 management interface

Signed-off-by: BenjiReis benjamin.reis@vates.fr

benjamreis · 2022-10-12T15:26:40Z

Hi, any comments on this? :)

lindig

I think this looks good in general but I'm not familiar with this area; since it involves a UI change, could you include screenshots?

backend.py

lindig · 2022-10-14T08:57:02Z

netutil.py

-        if int(el) > 255:
-            return False
-    return True
+    ipv4_re = '^(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}'


Both regexp are constructed from the same patterns repeatedly. I'd like to see the patterns bound to names and the final regexp (which is just a string) composed from the names.

https://stackoverflow.com/questions/6930982/how-to-use-a-variable-inside-a-regular-expression

Why are we not using the ipaddress library here, is it just because we don't include it in the install image?

It is indeed not included. Should it be and used?
Since this code is kind of a small ip lib i assumed it was by design not included.

The code doesn't use ipaddress because it long predates its introduction in Python 3.3.

We could use one of the unofficial Python 2 backports but I'd probably prefer that we move to Python 3 first and then do further improvements.

In the meantime, I think using a regex here would be OK if the repeated patterns were condensed.

I removed the use of regex to instead use socket.inet_pton. Is that okay with you?

upgrade.py

MarkSymsCtx · 2022-10-14T15:35:00Z

netinterface.py

@@ -32,8 +32,8 @@ class NetInterface:
    Autoconf = 3

    def __init__(self, mode, hwaddr, ipaddr=None, netmask=None, gateway=None,


all of the conditionals in this make this scream out for some heavy refactoring, splitting into different classes with static/dhcp helpers

I agree, but I'd wager that this PR adds a new feature, to add refactoring on top of that would make it even more complex.
IMHO, a new PR (or several) should be created to refactor the code and perhaps adds some GH actions to enforce coding style and such.

Typically the process is to refactor before adding the new feature and refactor after all changes to remove duplication and complexity.

Just realising: a netintrface can be configured for both IPv4 and IPv6 so splitting in 2 classes is not possible.

I've created a NetinterfaceV6 class, inheriting from Netinterface, to split the __init__ codes between the 2 classes.

MarkSymsCtx · 2022-10-14T15:37:47Z

netutil.py

-        if int(el) > 255:
-            return False
-    return True
+    ipv4_re = '^(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}'


Why are we not using the ipaddress library here, is it just because we don't include it in the install image?

MarkSymsCtx · 2022-10-14T15:38:58Z

tui/installer/screens.py

@@ -941,7 +942,7 @@ def nsvalue(answers, id):
                answers['manual-nameservers'][1].append(ns2_entry.value())
                if ns3_entry.value() != '':
                    answers['manual-nameservers'][1].append(ns3_entry.value())
-            if 'net-admin-configuration' in answers and answers['net-admin-configuration'].isStatic():
+            if 'net-admin-configuration' in answers and answers['net-admin-configuration'].valid() and not answers['net-admin-configuration'].isDHCP():


This check is now complex enough to warrant pulling it out as a well named method.

tui/network.py

MarkSymsCtx · 2022-10-14T15:41:19Z

tui/network.py

+                                dns_field.value(), vlan=vlan_value)
+        return RIGHT_FORWARDS, answers
+
+    def get_ipv6_configuration(nic, txt, defaults, include_dns):


far too long and far too complicated

Not sure how to do better than a subfunction per screen.
TUI in code is very verbose.

I'v mutualized the codes for both IPv4 and IPv6 interface configuration form

backend.py

tui/network.py

rosslagerwall · 2022-12-13T15:34:02Z

upgrade.py

@@ -487,6 +487,11 @@ def completeUpgrade(self, mounts, prev_install, target_disk, backup_partnum, log
            nfd.write("NETWORKING_IPV6=no\n")
            nfd.close()
            netutil.disable_ipv6_module(mounts["root"])
+        else:


Don't you need to write NETWORKING_IPV6=yes here to sysconfig/network?

Although since sysconfig/network is preserved across upgrades, I think it would be better to change the above if statement so that it only runs if sysconfig/network doesn't exist or if it exists but doesn't contain NETWORKING_IPV6 (i.e. to handle upgrades from old installations that didn't write it out).

NETWORKING_IPV6 is written here.

rosslagerwall · 2022-12-13T15:34:22Z

upgrade.py

@@ -487,6 +487,11 @@ def completeUpgrade(self, mounts, prev_install, target_disk, backup_partnum, log
            nfd.write("NETWORKING_IPV6=no\n")
            nfd.close()
            netutil.disable_ipv6_module(mounts["root"])
+        else:
+            # Enable IPV6
+            with open("%s/etc/sysctl.d/91-net-ipv6.conf" % mounts["root"], "w") as ipv6_conf:


You could add this to the restore list instead.

I'm not sure to understand this comment. Could you point me towards this restore list?
I assume it allows to keep a config file upon an host upgrade.
Since this would be a new file is there something else to do?

rosslagerwall · 2022-12-13T15:34:33Z

upgrade.py

+            # Enable IPV6
+            with open("%s/etc/sysctl.d/91-net-ipv6.conf" % mounts["root"], "w") as ipv6_conf:
+                for i in ['all', 'default']:
+                    ipv6_conf.write('net.ipv6.conf.%s.disable_ipv6=0\n' % i)



Do you need to enable ip6tables here?

I don't think so, the IPv6 is working without doing it.

rosslagerwall · 2022-12-13T15:35:53Z

tui/installer/screens.py

@@ -811,7 +811,8 @@ def ns_callback((enabled, )):
        for entry in [ns1_entry, ns2_entry, ns3_entry]:
            entry.setFlags(FLAG_DISABLED, enabled)

-    hide_rb = answers['net-admin-configuration'].isStatic()
+    admin_config = answers['net-admin-configuration']
+    hide_rb = admin_config.valid() and not admin_config.isDHCP()


I had the same comment earlier but setting nameservers shouldn't be conditional on DHCP should it?

Here it's to hide the radio button to select the auto DHCP nameservers so I think this is correct.
The code before my PR was already making a condition on the config being static.

tui/network.py

rosslagerwall · 2022-12-13T17:01:48Z

netutil.py

-        if int(el) > 255:
-            return False
-    return True
+    ipv4_re = '^(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}'


The code doesn't use ipaddress because it long predates its introduction in Python 3.3.

We could use one of the unofficial Python 2 backports but I'd probably prefer that we move to Python 3 first and then do further improvements.

In the meantime, I think using a regex here would be OK if the repeated patterns were condensed.

rosslagerwall · 2022-12-13T17:12:30Z

Aside from the small comments above, I wonder if it would be possible to have a config option (or just a variable in constants.py) that controls whether the new configuration screens are shown? At least at this point, I don't think we'd want to expose this option to our users so it would be nice to have an easy way of hiding it.

benjamreis · 2023-03-03T14:15:42Z

@rosslagerwall a quick question about the config option for enabling/disabling IPv6 support in the installer.
Is it still something wanted since it's now in the pipes to support IPv6 for XenServer as well AFAIK?

benjamreis · 2023-04-11T08:52:25Z

I fixed a few things, refactored the code to mutualize as much as possible.
Remains the questions of the restore list for 91-net-ipv6.conf for which I need directions and if the param to enable the IPv6 config is still needed.

For the rest this is good to review, it has been tested.

benjamreis · 2023-06-13T13:11:15Z

I've splitted the PR in several independant to commits to ease the review.
The Debstyle interfaces file no longer supports IPv6 since it seems to me it's dead code and I proposer a PR to remove it. I can add it back if needed.
The 91-net-ipv6.conf had been added to the restore list instead of manually rewriting it upon upgrade.

I can add a settings to enable/disable the IPv6 configuration in TUI as discussed above if this is still wanted, i'm not sure it is though so i'm waiting for your feedback.

lindig · 2023-06-13T13:20:02Z

netutil.py

+        return True
+
+    inet6s = filter(lambda x: x.startswith("    inet6 "), out.split("\n"))
+    return len(inet6s) > 1  # Not just the fe80:: address


This condition could be moved into the filter; this code is duplicated - so would it be worth to put it into a function?

You mean having 1 filter for both IPv4 and IPv6?
The thing is, having one IPv4 is enough but having only one IPv6 is not fecause of the link local addresses.

I was thinking to filter the output such that only inet6 that are not localhost remain. That would make it more explicit which ones you are looking for rather than assuming fe80:: is in the list or results.

So something like:

inet6s = filter(lambda x: x.startswith(" inet6 ") and not x.startswith(" inet6 fe80::"), out.split("\n"))

?

Yes. Is there a risk that the formatting of the input changes such that it would be better to use x.strip().find("inet6") or similar?

I don't think so. More recent versions of ipaddr still use this format.

benjamreis · 2023-06-15T07:51:09Z

netinterface.py

+        if self.isStatic6() and self.ipv6_gateway and util.runCmd2(
+            ['/usr/sbin/ndisc6', '-1', '-w', '120', self.ipv6_gateway, iface_name]
+        ):
+            return False


This code is equivalent to the arping code above but for IPv6 as arping only supports IPv4.
Please note this requires to have ndisc6 installed in host-installer image.

Is this reflected in the corresponding RPM spec file?

As far as I know your installer spec file repository is not public so I can't provide a PR to add ndisc6 which is why I commented my code for someone with access to reflect the change. :)

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Add family specific validator as well Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Only static configuration should allow a user to fill the fields Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

In case of error return directly Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

IPv4 for IPv4 only IPv6 for IPv6 only Dual for both (IPv4 will be primary in XAPI) Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Need to add `ndisc6` to the install.img Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Add `isDynamic` helper to `NetInterface` Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Add `etc/sysctl.d/91-net-ipv6.conf` in the restore list Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

The file should always be written if the fileds have been filled. Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis · 2023-06-21T07:37:13Z

Screens of the modified TUI:

lindig · 2023-06-21T07:41:23Z

I don't know if this is pre-existing: the blue background for the text inside the pop-up make it really busy; I don't think these input elements need to be highlighted at all or a more subtle method should be used: text colouring or bold text but not changing background colour.

lindig · 2023-06-21T07:43:53Z

@MarkSymsCtx commented that he would have liked to see a refactoring to avoid the heavy branching introduced by IPv6. While I think this is valid, I do like the counted work on this feature.

benjamreis · 2023-06-21T07:48:12Z

@MarkSymsCtx commented that he would have liked to see a refactoring to avoid the heavy branching introduced by IPv6. While I think this is valid, I do like the counted work on this feature.

I have splitted the PR in several commits to discriminate the refactorings and the changes of behavior to support IPv6 so reviewing commit per commit should allow to see what's what. 👍

benjamreis · 2023-06-21T07:48:46Z

I don't know if this is pre-existing: the blue background for the text inside the pop-up make it really busy; I don't think these input elements need to be highlighted at all or a more subtle method should be used: text colouring or bold text but not changing background colour.

AFAIK this was already like this

bernhardkaindl

I reviewed the "backend" changes. I did not review the frontend changes, but I guess that IPv6 for xsconsole should be merged/integrated first and then both can be tested together.

I'll be away until about 16 August.

Not reviewed: tui/network.py

benjamreis mentioned this pull request Sep 15, 2022

Allow to install an XCP-ng host in IPv6 (static only) or in dual stac… xcp-ng/host-installer_old#24

Closed

lindig reviewed Oct 14, 2022

View reviewed changes

MarkSymsCtx reviewed Oct 14, 2022

View reviewed changes

MarkSymsCtx requested a review from rosslagerwall October 14, 2022 15:42

rosslagerwall reviewed Dec 13, 2022

View reviewed changes

benjamreis force-pushed the allow-ipv6-install branch from 8ea4448 to 1791b5c Compare December 20, 2022 08:56

benjamreis force-pushed the allow-ipv6-install branch from b206e90 to 5e8f0a2 Compare February 24, 2023 08:19

benjamreis force-pushed the allow-ipv6-install branch from 6e8c2dd to dd57535 Compare March 3, 2023 14:28

benjamreis requested a review from rosslagerwall April 11, 2023 08:52

benjamreis force-pushed the allow-ipv6-install branch from a0c4f0d to 9b0f0bd Compare May 11, 2023 11:01

benjamreis mentioned this pull request May 11, 2023

Merge ipv6 support xcp-ng/host-installer#13

Merged

benjamreis mentioned this pull request Jun 8, 2023

Merge IPv6 patches xcp-ng/host-installer#18

Closed

benjamreis force-pushed the allow-ipv6-install branch 3 times, most recently from 9b0f0bd to a469a6b Compare June 13, 2023 13:06

lindig previously approved these changes Jun 13, 2023

View reviewed changes

benjamreis dismissed lindig’s stale review via 116df4e June 15, 2023 07:19

benjamreis force-pushed the allow-ipv6-install branch 5 times, most recently from 5c78f9e to 8bb3b5d Compare June 15, 2023 07:49

benjamreis commented Jun 15, 2023

View reviewed changes

benjamreis force-pushed the allow-ipv6-install branch from 8bb3b5d to 58e3ca0 Compare June 15, 2023 08:07

benjamreis added 2 commits June 15, 2023 10:36

isStatic helpers by IP family

2ba2428

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Use socket.inet_pton to validate IP

44d0e58

Add family specific validator as well Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis force-pushed the allow-ipv6-install branch from 58e3ca0 to d0bc68a Compare June 15, 2023 08:36

benjamreis added 7 commits June 16, 2023 14:55

Take IPv6 addresses into account to determine an interface is up

6f67e14

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Enable IPv6 in the host when it is configured

1210931

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Write IPv6 conf files for the installer

e70aa28

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Disable IP config textfield for autoconf

7e542f1

Only static configuration should allow a user to fill the fields Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Add a display_error helper for wainting network

e46ced8

In case of error return directly Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Wait a bit for autoconf when checking for network

dea0e44

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Put IP config info collection in a helper

681768f

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis force-pushed the allow-ipv6-install branch 4 times, most recently from 1bf7d15 to 0f18e5d Compare June 19, 2023 13:17

benjamreis added 6 commits June 20, 2023 11:06

Adapt get_iface_configuration helper for IPv6

81c0926

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Add IP family selection in TUI

578c7c0

IPv4 for IPv4 only IPv6 for IPv6 only Dual for both (IPv4 will be primary in XAPI) Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

waitUntilUp also wait for IPv6 if statically configured

6b5f1eb

Need to add `ndisc6` to the install.img Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Hide dynamic buttons in tui when in static mode

2d680c0

Add `isDynamic` helper to `NetInterface` Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Keep IPv6 enablement/disablement upon upgrades

14b75e3

Add `etc/sysctl.d/91-net-ipv6.conf` in the restore list Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Write resolver file if dns or searchdomain have specified

7add5d1

The file should always be written if the fileds have been filled. Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis force-pushed the allow-ipv6-install branch from 6ffa420 to 7c3eb0d Compare June 20, 2023 09:06

Allow to set manual DNS in TUI for autoconf

67389d6

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis force-pushed the allow-ipv6-install branch from 7c3eb0d to 67389d6 Compare June 20, 2023 09:07

bernhardkaindl approved these changes Jul 19, 2023

View reviewed changes

		@@ -32,8 +32,8 @@ class NetInterface:
		Autoconf = 3

		def __init__(self, mode, hwaddr, ipaddr=None, netmask=None, gateway=None,

Allow to install an host with IPv6 management interface #2

Are you sure you want to change the base?

Allow to install an host with IPv6 management interface #2

Conversation

benjamreis commented Sep 15, 2022

benjamreis commented Oct 12, 2022

lindig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosslagerwall commented Dec 13, 2022

benjamreis commented Mar 3, 2023

benjamreis commented Apr 11, 2023

benjamreis commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamreis Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamreis commented Jun 21, 2023

lindig commented Jun 21, 2023

lindig commented Jun 21, 2023

benjamreis commented Jun 21, 2023 • edited Loading

benjamreis commented Jun 21, 2023

bernhardkaindl left a comment • edited Loading

Choose a reason for hiding this comment

benjamreis Jun 16, 2023 •

edited

Loading

benjamreis commented Jun 21, 2023 •

edited

Loading

bernhardkaindl left a comment •

edited

Loading