From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 284DB1FF191 for ; Tue, 23 Sep 2025 14:07:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6C659D263; Tue, 23 Sep 2025 14:08:28 +0200 (CEST) Date: Tue, 23 Sep 2025 14:07:54 +0200 Message-Id: From: "Christoph Heiss" To: "Florian Paul Azim Hoberg" Mime-Version: 1.0 X-Mailer: aerc 0.21.0 References: <3978889E-73FA-4473-A70B-2C660662CC3D@credativ.de> In-Reply-To: <3978889E-73FA-4473-A70B-2C660662CC3D@credativ.de> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1758629261845 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.038 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 Cc: pve-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "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 > --- > > 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