From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 635C11FF183 for ; Wed, 24 Sep 2025 10:49:00 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 241721F688; Wed, 24 Sep 2025 10:49:29 +0200 (CEST) Message-ID: <655087f3-a592-43e0-b33b-251d0f1d50d5@proxmox.com> Date: Wed, 24 Sep 2025 10:48:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Christoph Heiss , Florian Paul Azim Hoberg References: <3978889E-73FA-4473-A70B-2C660662CC3D@credativ.de> From: Aaron Lauterer In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1758703720925 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.012 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH installer 1/1] fix #2164: add option to define a vlan tag within the installer X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 >> --- >> >> 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 @@ >> >> Management Interface: __interface__ >> >> + Management Interface vlan tag: __vlan__ > > 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? > >> + >> Hostname: __hostname__ >> >> IP CIDR: __cidr__ >> 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, >> pub dns: Option, >> pub gateway: Option, >> + pub vlan: Option, >> #[serde(default)] >> pub filter: BTreeMap, >> } >> @@ -219,6 +220,7 @@ impl TryFrom 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, >> pub filter: BTreeMap, >> } >> >> 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, > > 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::() >> .map_err(|err| err.to_string())?; >> >> + let vlan_str = view >> + .get_value::(5) >> + .ok_or("failed to retrieve VLAN")?; >> + >> + let vlan = if vlan_str.trim().is_empty() { >> + None >> + } else { >> + let parsed_vlan = vlan_str.trim().parse::() >> + .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 > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel