Re: [pve-devel] [PATCH installer 1/1] fix #2164: add option to define a vlan tag within the installer

* Re: [pve-devel] [PATCH installer 1/1] fix #2164: add option to define a vlan tag within the installer
       [not found] <3978889E-73FA-4473-A70B-2C660662CC3D@credativ.de>
@ 2025-09-23 12:07 ` Christoph Heiss
  2025-09-24  8:48   ` Aaron Lauterer
  0 siblings, 1 reply; 4+ messages in thread
From: Christoph Heiss @ 2025-09-23 12:07 UTC (permalink / raw)
  To: Florian Paul Azim Hoberg; +Cc: pve-devel

The subject should be rephrased to e.g. "partially fix #2164", as the
Bugzilla entry also mentions setting up bonds.

Didn't do any testing yet, but some comments inline:

On Mon Sep 22, 2025 at 2:30 PM CEST, Florian Paul Azim Hoberg wrote:
> To ensure a Promox node can access a desired network
> resource which requires a given vlan tag, this feature
> adds the possibility to optionally define a vlan tag
> within the auto installer, tui and gui.

s/this feature adds/add/g

Also could mention that it only applies to the selected management
interface/bridge.

>
> Signed-off-by: Florian Paul Azim Hoberg @gyptazy <florian.hoberg@credativ.de>
> ---
>
>  initial commit:
>  * Add possibility to optionally add vlan tag in:
>    - auto installer
>    - tui
>    - gui
>  * Tagged interface will be generated as:
>    $interface.$tag in /etc/network/interfaces
>  * Simple validation of given vlan tag:
>    - Must be digit
>    - Must be smaller than int(4094)

That can all be part of the commit message, as that is useful
information.

[..]
> diff --git a/html/ack_template.htm b/html/ack_template.htm
> index 48d7213..4404321 100644
> --- a/html/ack_template.htm
> +++ b/html/ack_template.htm
> @@ -54,6 +54,8 @@
>
>        <tr><td>Management Interface:</td> <td>__interface__</td></tr>
>
> +      <tr><td>Management Interface vlan tag:</td> <td>__vlan__</td></tr>

VLAN should be consistently spelled uppercase everywhere user-visible.

Also; wondering if we maybe should just hide this if no VLAN tag is set,
no not clutter the summary page unnecessarily?

> +
>        <tr><td>Hostname:</td> <td>__hostname__</td></tr>
>
>        <tr><td>IP CIDR:</td> <td>__cidr__</td></tr>
> diff --git a/proxinstall b/proxinstall
> index 5ba65fa..13ece1d 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -472,6 +472,14 @@ sub create_ipconf_view {
>      $grid->attach($dns_label, 0, 4, 1, 1);
>      $grid->attach($ipconf_entry_dns, 1, 4, 1, 1);
>
> +    my $cfg_vlan = Proxmox::Install::Config::get_vlan();
> +    my $vlan = defined($cfg_vlan) ? $cfg_vlan : '';

Can be simplified to

  my $cfg_vlan = Proxmox::Install::Config::get_vlan() // '';

> +
> +    my ($vlan_label, $ipconf_entry_vlan) = create_text_input($vlan, 'VLAN Tag (Optional)');
> +
> +    $grid->attach($vlan_label, 0, 5, 1, 1);
> +    $grid->attach($ipconf_entry_vlan, 1, 5, 1, 1);
> +
>      $gtk_state->{inbox}->show_all;
>      set_next(
>          undef,
> @@ -538,7 +546,19 @@ sub create_ipconf_view {
>              }
>              Proxmox::Install::Config::set_dns($dns_ip);
>
> -            #print STDERR "TEST $ipaddress/$netmask $gateway_ip $dns_ip\n";
> +            my $vlan = $ipconf_entry_vlan->get_text();
> +            if ($vlan !~ /^\d*$/) {
> +                Proxmox::UI::message("VLAN must contain only digits.");
> +                $ipconf_entry_vlan->grab_focus();
> +                return;
> +            }
> +            if ($vlan ne '' && $vlan >= 4094) {
> +                Proxmox::UI::message("VLAN must be smaller than 4094.");
> +                $ipconf_entry_vlan->grab_focus();
> +                return;
> +            }

Should also check for 0, as that is also a reserved value.

> +            $vlan = undef if $vlan eq '';
> +            Proxmox::Install::Config::set_vlan($vlan);
>
>              $step_number++;
>              create_ack_view();
> @@ -585,6 +605,7 @@ sub create_ack_view {
>          __cidr__ => Proxmox::Install::Config::get_cidr(),
>          __gateway__ => Proxmox::Install::Config::get_gateway(),
>          __dnsserver__ => Proxmox::Install::Config::get_dns(),
> +        __vlan__ => Proxmox::Install::Config::get_vlan(),

Depending on above, whether to hide it if unset or not, in the latter
case this should at least default to 'None' as in the TUI.

>      );
>
>      while (my ($k, $v) = each %config_values) {
> diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
> index 88f4c87..cea5cb1 100644
> --- a/proxmox-auto-installer/src/answer.rs
> +++ b/proxmox-auto-installer/src/answer.rs
> @@ -186,6 +186,7 @@ struct NetworkInAnswer {
>      pub cidr: Option<CidrAddress>,
>      pub dns: Option<IpAddr>,
>      pub gateway: Option<IpAddr>,
> +    pub vlan: Option<u16>,
>      #[serde(default)]
>      pub filter: BTreeMap<String, String>,
>  }
> @@ -219,6 +220,7 @@ impl TryFrom<NetworkInAnswer> for Network {
>                      cidr: network.cidr.unwrap(),
>                      dns: network.dns.unwrap(),
>                      gateway: network.gateway.unwrap(),
> +                    vlan: network.vlan,

Before setting this, the same check as for the GUI/TUI can be
implemented above.

And it's missing the check that `vlan` is unset as with the other
properties below in the DHCP case.

>                      filter: network.filter,
>                  }),
>              })
> @@ -254,6 +256,7 @@ pub struct NetworkManual {
>      pub cidr: CidrAddress,
>      pub dns: IpAddr,
>      pub gateway: IpAddr,
> +    pub vlan: Option<u16>,
>      pub filter: BTreeMap<String, String>,
>  }
>
> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
> index 7d42f2c..2fdb6df 100644
> --- a/proxmox-auto-installer/src/utils.rs
> +++ b/proxmox-auto-installer/src/utils.rs
> @@ -66,6 +66,7 @@ fn get_network_settings(
>          network_options.address = settings.cidr.clone();
>          network_options.dns_server = settings.dns;
>          network_options.gateway = settings.gateway;
> +        network_options.vlan = settings.vlan;
>          network_options.ifname = get_single_udev_index(&settings.filter, &udev_info.nics)?;
>      }
>      info!("Network interface used is '{}'", &network_options.ifname);
> @@ -494,6 +495,7 @@ pub fn parse_answer(
>          domain: network_settings.fqdn.domain(),
>          cidr: network_settings.address,
>          gateway: network_settings.gateway,
> +        vlan: network_settings.vlan,
>          dns: network_settings.dns_server,
>
>          first_boot: InstallFirstBootSetup::default(),
[..]
> diff --git a/proxmox-auto-installer/tests/resources/parse_answer/network_vlan.toml b/proxmox-auto-installer/tests/resources/parse_answer/network_vlan.toml
> new file mode 100644
> index 0000000..fad56c6
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer/network_vlan.toml
> @@ -0,0 +1,19 @@
> +[global]
> +keyboard = "de"
> +country = "at"
> +fqdn = "pveauto.testinstall"
> +mailto = "mail@no.invalid"
> +timezone = "Europe/Vienna"
> +root-password = "12345678"
> +
> +[network]
> +source = "from-answer"
> +cidr = "10.10.10.10/24"
> +dns = "10.10.10.1"
> +vlan = 123
> +gateway = "10.10.10.1"
> +filter.ID_NET_NAME = "enp129s0f1np1"
> +
> +[disk-setup]
> +filesystem = "ext4"
> +disk-list = ["sda"]
[..]
> diff --git a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml
> index 901f894..723dfc8 100644
> --- a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml
> +++ b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml
> @@ -10,6 +10,7 @@ root-password = "12345678"
>  source = "from-answer"
>  cidr = "10.10.10.10/24"
>  dns = "10.10.10.1"
> +vlan = 123

Any reason for adding it to this test too, as the `network_vlan` test
above seems to be the exact same?

>  gateway = "10.10.10.1"
>  filter.ID_NET_NAME_MAC = "*a0369f0ab382"
>
[..]
> diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
> index 66cea72..23e43d2 100644
> --- a/proxmox-installer-common/src/setup.rs
> +++ b/proxmox-installer-common/src/setup.rs
> @@ -580,6 +580,7 @@ pub struct InstallConfig {
>      #[serde(serialize_with = "serialize_as_display")]
>      pub cidr: CidrAddress,
>      pub gateway: IpAddr,
> +    pub vlan: Option<u16>,

Should be annotated with

    #[serde(skip_serializing_if = "Option::is_none")]

to avoid serializing it as `null` if unset.

>      pub dns: IpAddr,
>
>      pub first_boot: InstallFirstBootSetup,
> diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
> index 15ee5d3..21ef16f 100644
> --- a/proxmox-tui-installer/src/main.rs
> +++ b/proxmox-tui-installer/src/main.rs
[..]
> @@ -552,6 +560,23 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView {
>                      .parse::<IpAddr>()
>                      .map_err(|err| err.to_string())?;
>
> +                let vlan_str = view
> +                    .get_value::<EditView, _>(5)
> +                    .ok_or("failed to retrieve VLAN")?;
> +
> +                let vlan = if vlan_str.trim().is_empty() {
> +                    None
> +                } else {
> +                    let parsed_vlan = vlan_str.trim().parse::<u16>()
> +                        .map_err(|e| format!("Invalid VLAN: {e}"))?;
> +
> +                    if parsed_vlan >= 4094 {
> +                        return Err(format!("VLAN must be smaller than 4094."));
> +                    }

Same tag 0 check as for the GUI.

[..]
> diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
> index c80877f..c59712e 100644
> --- a/proxmox-tui-installer/src/options.rs
> +++ b/proxmox-tui-installer/src/options.rs
> @@ -72,6 +72,7 @@ impl InstallerOptions {
>              SummaryOption::new("Keyboard layout", kb_layout),
>              SummaryOption::new("Administrator email", &self.password.email),
>              SummaryOption::new("Management interface", &self.network.ifname),
> +            SummaryOption::new("Management interface vlan tag", &self.network.vlan.map(|v| v.to_string()).unwrap_or("None".to_string())),

Uppercase spelling as in the GUI.

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread