[pve-devel] [PATCH installer 1/1] fix #2164: add option to define a vlan tag within the installer
Aaron Lauterer
a.lauterer at proxmox.com
Wed Sep 24 10:48:54 CEST 2025
Thanks for working on this. One more comment from my side.
Since the default network configs are often used as a guideline by our
users, they should reflect best practices.
Instead of the dot notation, I would prefer the vlan-raw-device, vlan-id
variant. This allows us to give the interface a name that matches the
use, and the whole config is more explicit.
The final config could look like this:
auto management
iface management inet static
address {host ip}/{cidr}
gateway {gateway ip}
vlan-id {vlan tag}
vlan-raw-device {physical nic}
#Management interface
On 2025-09-23 14:08, Christoph Heiss wrote:
> 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 at 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 at 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
More information about the pve-devel
mailing list