[pve-devel] [PATCH installer v2 3/6] low-level: change root password option to contain either plaintext or hash
A hashed password can be created e.g. using the `mkpasswd(1)`.
This then will allow the auto-installer to pass along a
already-hashed password from the user, instead of simple plaintext.
Signed-off-by: Christoph Heiss
---
Changes v1 -> v2:
* no changes
Proxmox/Install.pm| 25 ++---
Proxmox/Install/Config.pm | 20 +---
proxinstall | 4 ++--
3 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c0f8955..bcf8ba7 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -621,6 +621,27 @@ sub prepare_grub_efi_boot_esp {
die "failed to prepare EFI boot using Grub on '$espdev': $err" if $err;
}
+my sub setup_root_password {
+my ($targetdir) = @_;
+
+my $plain = Proxmox::Install::Config::get_root_password('plain');
+my $hashed = Proxmox::Install::Config::get_root_password('hashed');
+
+die "root password must be set!\n"
+ if !defined($plain) && !defined($hashed);
+
+die "plain and hashed root password cannot be set at the same time!\n"
+ if defined($plain) && defined($hashed);
+
+if (defined($plain)) {
+ my $octets = encode("utf-8", $plain);
+ run_command("chroot $targetdir /usr/sbin/chpasswd", undef,
"root:$octets\n");
+} elsif (defined($hashed)) {
+ my $octets = encode("utf-8", $hashed);
+ run_command("chroot $targetdir /usr/sbin/chpasswd --encrypted", undef,
"root:$octets\n");
+}
+}
+
sub extract_data {
my $iso_env = Proxmox::Install::ISOEnv::get();
my $run_env = Proxmox::Install::RunEnv::get();
@@ -1269,9 +1290,7 @@ _EOD
diversion_remove($targetdir, "/sbin/start-stop-daemon");
- # set root password
- my $octets = encode("utf-8", Proxmox::Install::Config::get_password());
- run_command("chroot $targetdir /usr/sbin/chpasswd", undef,
"root:$octets\n");
+ setup_root_password($targetdir);
# set root ssh keys
my $ssh_keys = Proxmox::Install::Config::get_root_ssh_keys();
diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm
index ecd8a74..0313fd9 100644
--- a/Proxmox/Install/Config.pm
+++ b/Proxmox/Install/Config.pm
@@ -90,7 +90,7 @@ my sub init_cfg {
keymap => 'en-us',
# root credentials & details
- password => undef,
+ root_password => undef,
mailto => 'mail@example.invalid',
root_ssh_keys => [],
@@ -196,8 +196,22 @@ sub get_timezone { return get('timezone'); }
sub set_keymap { set_key('keymap', $_[0]); }
sub get_keymap { return get('keymap'); }
-sub set_password { set_key('password', $_[0]); }
-sub get_password { return get('password'); }
+sub set_root_password {
+my ($key) = @_;
+croak "unknown root password option '$key'"
+ if $key ne 'plain' && $key ne 'hashed';
+
+set_key('root_password', { $_[0] => $_[1] });
+}
+
+sub get_root_password {
+my ($key) = @_;
+croak "unknown root password option '$key'"
+ if $key ne 'plain' && $key ne 'hashed';
+
+my $password = get('root_password');
+return defined($password->{$key}) ? $password->{$key} : undef;
+}
sub set_mailto { set_key('mailto', $_[0]); }
sub get_mailto { return get('mailto'); }
diff --git a/proxinstall b/proxinstall
index a6a4cfb..12f3eaa 100755
--- a/proxinstall
+++ b/proxinstall
@@ -674,7 +674,7 @@ sub create_password_view {
cleanup_view();
-my $password = Proxmox::Install::Config::get_password();
+my $password = Proxmox::Install::Config::get_root_password('plain');
my $grid = &$create_basic_grid();
$gtk_state->{inbox}->pack_start($grid, 0, 0, 0);
@@ -745,7 +745,7 @@ sub create_password_view {
return;
}
- Proxmox::Install::Config::set_password($t1);
+ Proxmox::Install::Config::set_root_password('plain', $t1);
Proxmox::Install::Config::set_mailto($t3);
$step_number++;
--
2.45.1
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 5/6] auto-installer: add new `global.root_password_hashed` answer option
This allows user to specify the root password in a hashed format,
generated using e.g. mkpasswd(1), instead of plaintext.
Signed-off-by: Christoph Heiss
---
Changes v1 -> v2:
* move root password setting validation into own function
* explicitly check for case for both are unset
proxmox-auto-installer/src/answer.rs | 3 ++-
proxmox-auto-installer/src/utils.rs | 16 ++--
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/proxmox-auto-installer/src/answer.rs
b/proxmox-auto-installer/src/answer.rs
index aab7198..d691da1 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -26,7 +26,8 @@ pub struct Global {
pub keyboard: KeyboardLayout,
pub mailto: String,
pub timezone: String,
-pub root_password: String,
+pub root_password: Option,
+pub root_password_hashed: Option,
#[serde(default)]
pub reboot_on_error: bool,
#[serde(default)]
diff --git a/proxmox-auto-installer/src/utils.rs
b/proxmox-auto-installer/src/utils.rs
index 229b7e2..2500f43 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -303,6 +303,17 @@ pub fn verify_locale_settings(answer: &Answer, locales:
&LocaleInfo) -> Result<(
Ok(())
}
+fn verify_root_password_settings(answer: &Answer) -> Result<()> {
+if answer.global.root_password.is_some() &&
answer.global.root_password_hashed.is_some() {
+bail!("`global.root_password` and `global.root_password_hashed` cannot
be set at the same time");
+} else if answer.global.root_password.is_none() &&
answer.global.root_password_hashed.is_none()
+{
+bail!("One of `global.root_password` or `global.root_password_hashed`
must be set");
+} else {
+Ok(())
+}
+}
+
pub fn parse_answer(
answer: &Answer,
udev_info: &UdevInfo,
@@ -318,6 +329,7 @@ pub fn parse_answer(
let network_settings = get_network_settings(answer, udev_info,
runtime_info, setup_info)?;
verify_locale_settings(answer, locales)?;
+verify_root_password_settings(answer)?;
let mut config = InstallConfig {
autoreboot: 1_usize,
@@ -337,8 +349,8 @@ pub fn parse_answer(
keymap: answer.global.keyboard.to_string(),
root_password: InstallRootPassword {
-plain: Some(answer.global.root_password.clone()),
-hashed: None,
+plain: answer.global.root_password.clone(),
+hashed: answer.global.root_password_hashed.clone(),
},
mailto: answer.global.mailto.clone(),
root_ssh_keys: answer.global.root_ssh_keys.clone(),
--
2.45.1
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 1/6] common: move `PasswordOptions` type to tui crate
It's only used internally there anyway, so make it slightly less
confusing.
Signed-off-by: Christoph Heiss
---
Changes v1 -> v2:
* no changes
proxmox-installer-common/src/options.rs | 15 ---
proxmox-tui-installer/src/main.rs | 4 ++--
proxmox-tui-installer/src/options.rs| 18 --
3 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/proxmox-installer-common/src/options.rs
b/proxmox-installer-common/src/options.rs
index e77914b..9375ded 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -327,21 +327,6 @@ impl TimezoneOptions {
}
}
-#[derive(Clone, Debug)]
-pub struct PasswordOptions {
-pub email: String,
-pub root_password: String,
-}
-
-impl Default for PasswordOptions {
-fn default() -> Self {
-Self {
-email: "mail@example.invalid".to_string(),
-root_password: String::new(),
-}
-}
-}
-
#[derive(Clone, Debug, PartialEq)]
pub struct NetworkOptions {
pub ifname: String,
diff --git a/proxmox-tui-installer/src/main.rs
b/proxmox-tui-installer/src/main.rs
index 4fb7afd..dd600c6 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -16,10 +16,10 @@ use cursive::{
use regex::Regex;
mod options;
-use options::InstallerOptions;
+use options::{InstallerOptions, PasswordOptions};
use proxmox_installer_common::{
-options::{BootdiskOptions, NetworkOptions, PasswordOptions,
TimezoneOptions},
+options::{BootdiskOptions, NetworkOptions, TimezoneOptions},
setup::{installer_setup, LocaleInfo, ProxmoxProduct, RuntimeInfo,
SetupInfo},
utils::Fqdn,
};
diff --git a/proxmox-tui-installer/src/options.rs
b/proxmox-tui-installer/src/options.rs
index 73fbf2a..6335762 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -2,8 +2,7 @@ use crate::SummaryOption;
use proxmox_installer_common::{
options::{
-BootdiskOptions, BtrfsRaidLevel, FsType, NetworkOptions,
PasswordOptions, TimezoneOptions,
-ZfsRaidLevel,
+BootdiskOptions, BtrfsRaidLevel, FsType, NetworkOptions,
TimezoneOptions, ZfsRaidLevel,
},
setup::LocaleInfo,
};
@@ -25,6 +24,21 @@ pub const FS_TYPES: &[FsType] = {
]
};
+#[derive(Clone, Debug)]
+pub struct PasswordOptions {
+pub email: String,
+pub root_password: String,
+}
+
+impl Default for PasswordOptions {
+fn default() -> Self {
+Self {
+email: "mail@example.invalid".to_string(),
+root_password: String::new(),
+}
+}
+}
+
#[derive(Clone, Debug)]
pub struct InstallerOptions {
pub bootdisk: BootdiskOptions,
--
2.45.1
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 4/6] {auto, tui}-installer: adapt to new `root_password` plain/hashed setup option
Signed-off-by: Christoph Heiss
---
Changes v1 -> v2:
* previously split up into two separate patches between auto- and tui-
installer, now merged together for consistency
proxmox-auto-installer/src/utils.rs | 9 +++--
.../tests/resources/parse_answer/disk_match.json | 2 +-
.../tests/resources/parse_answer/disk_match_all.json | 2 +-
.../tests/resources/parse_answer/disk_match_any.json | 2 +-
.../tests/resources/parse_answer/minimal.json| 2 +-
.../tests/resources/parse_answer/nic_matching.json | 2 +-
.../tests/resources/parse_answer/specific_nic.json | 2 +-
.../tests/resources/parse_answer/zfs.json| 2 +-
proxmox-installer-common/src/setup.rs| 12 ++--
proxmox-tui-installer/src/setup.rs | 10 --
10 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/proxmox-auto-installer/src/utils.rs
b/proxmox-auto-installer/src/utils.rs
index 202ad41..229b7e2 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -10,7 +10,9 @@ use crate::{
};
use proxmox_installer_common::{
options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption},
-setup::{InstallConfig, InstallZfsOption, LocaleInfo, RuntimeInfo,
SetupInfo},
+setup::{
+InstallConfig, InstallRootPassword, InstallZfsOption, LocaleInfo,
RuntimeInfo, SetupInfo,
+},
};
use serde::{Deserialize, Serialize};
@@ -334,7 +336,10 @@ pub fn parse_answer(
timezone: answer.global.timezone.clone(),
keymap: answer.global.keyboard.to_string(),
-password: answer.global.root_password.clone(),
+root_password: InstallRootPassword {
+plain: Some(answer.global.root_password.clone()),
+hashed: None,
+},
mailto: answer.global.mailto.clone(),
root_ssh_keys: answer.global.root_ssh_keys.clone(),
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
b/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
index 3a117b6..a0942cc 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
@@ -18,7 +18,7 @@
"keymap": "de",
"mailto": "mail@no.invalid",
"mngmt_nic": "eno1",
- "password": "123456",
+ "root_password": { "plain": "123456" },
"timezone": "Europe/Vienna",
"zfs_opts": {
"arc_max": 2048,
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
index 5325fc3..a7324f1 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
@@ -15,7 +15,7 @@
"keymap": "de",
"mailto": "mail@no.invalid",
"mngmt_nic": "eno1",
- "password": "123456",
+ "root_password": { "plain": "123456" },
"timezone": "Europe/Vienna",
"zfs_opts": {
"arc_max": 2048,
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
index 18e22d1..8e13496 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
@@ -22,7 +22,7 @@
"keymap": "de",
"mailto": "mail@no.invalid",
"mngmt_nic": "eno1",
- "password": "123456",
+ "root_password": { "plain": "123456" },
"timezone": "Europe/Vienna",
"zfs_opts": {
"arc_max": 2048,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
b/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
index bb72713..7a6bfd5 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
@@ -12,7 +12,7 @@
"keymap": "de",
"mailto": "mail@no.invalid",
"mngmt_nic": "eno1",
- "password": "123456",
+ "root_password": { "plain": "123456" },
"target_hd": "/dev/sda",
"timezone": "Europe/Vienna"
}
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
index de94165..e1c9d12 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
@@ -12,7 +12,7 @@
"keymap": "de",
"mailto": "mail@no.invalid",
"mngmt_nic": "enp65s0f0",
- "password": "123456",
+ "root_password": { "plain": "123456" },
"target_hd": "/dev/sda",
"timezone": "Europe/Vienna"
}
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/specific_nic.json
b/proxmox-auto-installer/tests/resources/parse_answer/specific_nic.json
index 5b4fcfc..1187eb4 100644
--- a/proxmox-auto-installer/tests/reso
[pve-devel] [PATCH installer v2 2/6] tui-installer: remove `Debug` implementation for password options
So we never accidentally show/log the password somewhere. Need to drop
it from `InstallerOptions` in turn too, but it's never used currently
anyway.
Signed-off-by: Christoph Heiss
---
Changes v1 -> v2:
* no changes
proxmox-tui-installer/src/options.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/proxmox-tui-installer/src/options.rs
b/proxmox-tui-installer/src/options.rs
index 6335762..19992ca 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -24,7 +24,7 @@ pub const FS_TYPES: &[FsType] = {
]
};
-#[derive(Clone, Debug)]
+#[derive(Clone)]
pub struct PasswordOptions {
pub email: String,
pub root_password: String,
@@ -39,7 +39,7 @@ impl Default for PasswordOptions {
}
}
-#[derive(Clone, Debug)]
+#[derive(Clone)]
pub struct InstallerOptions {
pub bootdisk: BootdiskOptions,
pub timezone: TimezoneOptions,
--
2.45.1
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 0/6] auto-installer: add option for providing hashed root password
This series adds a new answer option `global.root_password_hashed`
for the auto-installer, enabling administrators to specify the root
password of the new installation in a hashed format - as generated by
e.g. mkpasswd(1) - instead of plain-text.
Administrators/users might want to avoid passing along a plain-text
password with the different answer-fetching methods supported by the
auto-installer, for obvious reasons.
While this of course does not provide full security, sending a hashed
password might still be preferred by administrators over plain text.
Tested by installing using the GUI and TUI (to ensure no regressions
can happen) and using the auto-installer, once with `root_password` set
(again testing for potential regressions) and once with
`global.root_password_hashed` set instead, testing the new
functionality.
First two patches are small cleanups and may be applied independently.
v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063949.html
Notable changes v1 -> v2:
* rebased on latest master
* fixed rebase mistake
* merged previous patch #4/#5 for consistency across crates
* improved validation in auto-installer
Christoph Heiss (6):
common: move `PasswordOptions` type to tui crate
tui-installer: remove `Debug` implementation for password options
low-level: change root password option to contain either plaintext or
hash
{auto,tui}-installer: adapt to new `root_password` plain/hashed setup
option
auto-installer: add new `global.root_password_hashed` answer option
auto-installer: add test for hashed root password option
Proxmox/Install.pm| 25 ---
Proxmox/Install/Config.pm | 20 ---
proxinstall | 4 +--
proxmox-auto-installer/src/answer.rs | 3 ++-
proxmox-auto-installer/src/utils.rs | 21 ++--
.../resources/parse_answer/disk_match.json| 2 +-
.../parse_answer/disk_match_all.json | 2 +-
.../parse_answer/disk_match_any.json | 2 +-
.../parse_answer/hashed_root_password.json| 20 +++
.../parse_answer/hashed_root_password.toml| 14 +++
.../tests/resources/parse_answer/minimal.json | 2 +-
.../resources/parse_answer/nic_matching.json | 2 +-
.../resources/parse_answer/specific_nic.json | 2 +-
.../tests/resources/parse_answer/zfs.json | 2 +-
proxmox-installer-common/src/options.rs | 15 ---
proxmox-installer-common/src/setup.rs | 12 +++--
proxmox-tui-installer/src/main.rs | 4 +--
proxmox-tui-installer/src/options.rs | 20 ---
proxmox-tui-installer/src/setup.rs| 10 ++--
19 files changed, 140 insertions(+), 42 deletions(-)
create mode 100644
proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.json
create mode 100644
proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.toml
--
2.44.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 6/6] auto-installer: add test for hashed root password option
Signed-off-by: Christoph Heiss
---
Changes v1 -> v2:
* no changes
.../parse_answer/hashed_root_password.json| 20 +++
.../parse_answer/hashed_root_password.toml| 14 +
2 files changed, 34 insertions(+)
create mode 100644
proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.json
create mode 100644
proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.toml
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.json
b/proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.json
new file mode 100644
index 000..19c51ba
--- /dev/null
+++
b/proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.json
@@ -0,0 +1,20 @@
+{
+ "autoreboot": 1,
+ "cidr": "192.168.1.114/24",
+ "country": "at",
+ "dns": "192.168.1.254",
+ "domain": "testinstall",
+ "filesys": "ext4",
+ "gateway": "192.168.1.1",
+ "hdsize": 223.57088470458984,
+ "lvm_auto_rename": 1,
+ "hostname": "pveauto",
+ "keymap": "de",
+ "mailto": "mail@no.invalid",
+ "mngmt_nic": "eno1",
+ "root_password": {
+"hashed":
"$y$j9T$VgMv8lsz/TEvzesCZU3xD.$SK.h4QW51Jr/EmjuaTz5Bt4kYiX2Iezz6omzoqVEwj9"
+ },
+ "target_hd": "/dev/sda",
+ "timezone": "Europe/Vienna"
+}
diff --git
a/proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.toml
b/proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.toml
new file mode 100644
index 000..ed4d2e5
--- /dev/null
+++
b/proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.toml
@@ -0,0 +1,14 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password_hashed =
"$y$j9T$VgMv8lsz/TEvzesCZU3xD.$SK.h4QW51Jr/EmjuaTz5Bt4kYiX2Iezz6omzoqVEwj9"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
--
2.45.1
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer 00/14] fix #5536: implement post-(auto-)installation notification mechanism
Am 10/07/2024 um 15:27 schrieb Christoph Heiss:
> This implements a mechanism for post-installation "notifications" via a
> POST request [0] when using the auto-installer.
>
> It's implemented as a separate, small utility to facilitate separation
> of concerns and make the information gathering easier by having it
> isolated in one place.
>
> Patches #1 through #10 are simply clean-ups, refactors, etc. that were
> done along the way, which do not impact functionality in any way.
>
> Most interesting here will be patch #12, which adds the actual
> implementation of the post-hook. (Bind-)mounting the installed host
> system is done using the existing `proxmox-chroot` utility, and the HTTP
> POST functionality can fortunately be re-used 1:1 from
> `proxmox-fetch-answer`.
>
> I've also included an example of how the JSON body (pretty-printed for
> readability) of such a post-installation request would look like below,
> for reference.
>
> Tested this with both PVE and PBS ISOs, PMG did not (yet) have a
> release with an auto-installation capable ISO. The only product-specific
> code is the version detection in `proxmox-post-hook`, which - since it
> works the same for PVE and PMG - be no obstacle.
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=5536
>
> {
> "debian-version": "12.5",
> "product-version": "pve-manager/8.2.2/9355359cd7afbae4",
> "kernel-version": "proxmox-kernel-6.8.4-2-pve-signed",
no hard feelings, but from a gut feeling I'd probably move this to a
"version" object and potentially use a more reduced, easier to parse, value.
Maybe also as an object so that both, a simple X.Y(.Z) release and a full
string are given, as we changed (kernel) packages name in the past, and I
could imagine that there will be a LTS and non LTS variant if we ever rework
on where we base our kernel on, so this might change in the future too.
While the simple X.Y.Z version will more likely stay as is.
And I do not want to move the goal post here to far, but isn't some of this
information potentially interesting to have sent to a metric server?
At least with a low frequency (or just once on every boot) so that one has a
somewhat recent externally saved set of information that can be used to
identify machines more easily and be aware of some changes to correlate
regressions or strange (load) metrics with.
No need to do that now, but maybe something to keep in mind to allow easier
reuse of this.
IMO it's a big plus if we manage to keep information laid out the same way,
or at list in a similar one, wherever it's included. And that doesn't have
to mean that the whole struct has to be shared, maybe it just could be just
a collection of types that stem from common crates outside the installer
(at least in the long run, as said, no need to completely block this on
scope extension now).
> "boot-type": "bios",
We call this "mode" in the product APIs [0], might potentially make sense
to use the same schema here? Else I'd at least name this boot-mode and use
the same keys.
[0]: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/status
> "filesystem": "zfs (RAID1)",
> "fqdn": "host.domain",
> "machine-id": "f4bf9711783248b7aaffe3ccbca3e3dc",
> "bootdisk": [
could be also interesting to get all disks and flag the ones used for booting
with a boolean "bootdisk" flag. E.g. to make additional storage provisioning
later on slightly more convenient.
> {
> "size": 8589934592,
> "udev-properties": {
> "DEVNAME": "/dev/vda", [..]
> }
> },
> {
> "size": 8589934592,
> "udev-properties": {
> "DEVNAME": "/dev/vdb", [..]
> }
> }
> ],
> "management-nic": {
> "mac": "de:ad:f0:0d:12:34",
> "address": "10.0.0.10/24",
> "udev-properties": {
> "INTERFACE": "enp6s18", [..]
> }
> },
> "ssh-public-host-keys": {
> "ecdsa": "ecdsa-sha2-nistp256 [..] root@host",
> "ed25519": "ssh-ed25519 [..] root@host",
> "rsa": "ssh-rsa [..] root@host",
> }
> }
>
> Christoph Heiss (14): chroot: print full anyhow message
> tree-wide: fix some typos
> tree-wide: collect hardcoded installer runtime directory strings into
> constant
> common: simplify filesystem type serializing & Display trait impl
> common: setup: serialize `target_hd` as string explicitly
> common: split out installer setup files loading functionality
> debian: strip unused library dependencies
> fetch-answer: move http-related code to gated module in
> installer-common
> tree-wide: convert some more crates to use workspace dependencies
> auto-installer: tests: replace left/right with got/expected in output
> auto-installer: answer: add `posthook` section
> fix #5536: add post-hook utility for sending notifications after
> auto-install
> fix #5536: post-hook: add some unit tests
> unconfigured.sh: run proxmox-post-hook after successful auto-install
>
> Cargo.toml| 1
Re: [pve-devel] [PATCH installer 00/14] fix #5536: implement post-(auto-)installation notification mechanism
Thanks for the feedback!
Since this will be externally consumed API surface, changing it
afterwards will be a pain anyway - so comments on the JSON schema are
really appreciated too.
On Mon, Jul 15, 2024 at 12:42:41PM GMT, Thomas Lamprecht wrote:
> Am 10/07/2024 um 15:27 schrieb Christoph Heiss:
> > [..]
> > {
> > "debian-version": "12.5",
> > "product-version": "pve-manager/8.2.2/9355359cd7afbae4",
> > "kernel-version": "proxmox-kernel-6.8.4-2-pve-signed",
>
> no hard feelings, but from a gut feeling I'd probably move this to a
> "version" object and potentially use a more reduced, easier to parse, value.
> Maybe also as an object so that both, a simple X.Y(.Z) release and a full
> string are given, as we changed (kernel) packages name in the past, and I
> could imagine that there will be a LTS and non LTS variant if we ever rework
> on where we base our kernel on, so this might change in the future too.
> While the simple X.Y.Z version will more likely stay as is.
Yeah, that's fair. While writing this I've basically just looked around
what information could potentially be interesting for
users/administrators and then dumped them into a JSON.
Having an actual structured version object here makes sense.
>
> And I do not want to move the goal post here to far, but isn't some of this
> information potentially interesting to have sent to a metric server?
Yeah, at least Prometheus' node_exporter does send such information, so
its quite "standard" practice.
> At least with a low frequency (or just once on every boot) so that one has a
> somewhat recent externally saved set of information that can be used to
> identify machines more easily and be aware of some changes to correlate
> regressions or strange (load) metrics with.
Just to get this right: is the idea to (optionally) send some/all of
this information to a external metrics server, in addition to the
posthook target?
Or just generally to keep in mind where information like this could go
and why it should be uniform?
>
> No need to do that now, but maybe something to keep in mind to allow easier
> reuse of this.
>
> IMO it's a big plus if we manage to keep information laid out the same way,
> or at list in a similar one, wherever it's included. And that doesn't have
> to mean that the whole struct has to be shared, maybe it just could be just
> a collection of types that stem from common crates outside the installer
> (at least in the long run, as said, no need to completely block this on
> scope extension now).
>
> > "boot-type": "bios",
>
> We call this "mode" in the product APIs [0], might potentially make sense
> to use the same schema here? Else I'd at least name this boot-mode and use
> the same keys.
>
> [0]:
> https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/status
Sounds good! Looking at the API, I'd take the entire "boot-info" object
from there and have it be the same.
I will generally try to align the schema of this JSON a bit more with
that returned by the API mentioned above. E.g. also the (kernel) version
object above with the "current-kernel" structure, as far as possible.
And looking at it, we also could include the "cpuinfo" structure for
good measure.
Being consistent (where sensible, as you say) with our existing API
makes very much sense, didn't think of this!
>
> > "filesystem": "zfs (RAID1)",
> > "fqdn": "host.domain",
> > "machine-id": "f4bf9711783248b7aaffe3ccbca3e3dc",
> > "bootdisk": [
>
> could be also interesting to get all disks and flag the ones used for booting
> with a boolean "bootdisk" flag. E.g. to make additional storage provisioning
> later on slightly more convenient.
With that in mind it definitely could come in handy. Or maybe a separate
object "disks"/"other-disks"/etc. entirely? So as not have to filter out
the (non-)bootdisks again on the receiving end.
While at it, the same would IMO make sense for NICs too, since one might
want to set up additional network devices too.
>
> > {
> > "size": 8589934592,
> > "udev-properties": {
> > "DEVNAME": "/dev/vda", [..]
> > }
> > },
> > {
> > "size": 8589934592,
> > "udev-properties": {
> > "DEVNAME": "/dev/vdb", [..]
> > }
> > }
> > ],
> > "management-nic": {
> > "mac": "de:ad:f0:0d:12:34",
> > "address": "10.0.0.10/24",
> > "udev-properties": {
> > "INTERFACE": "enp6s18", [..]
> > }
> > },
> > "ssh-public-host-keys": {
> > "ecdsa": "ecdsa-sha2-nistp256 [..] root@host",
> > "ed25519": "ssh-ed25519 [..] root@host",
> > "rsa": "ssh-rsa [..] root@host",
> > }
> > }
> > [..]
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer v3 4/4] low-level: install: check for already-existing `rpool` on install
Thanks for the review!
On Fri, Jul 12, 2024 at 11:36:56AM GMT, Aaron Lauterer wrote:
> only one small nit inline
>
> On 2024-07-11 13:57, Christoph Heiss wrote:
> > [..]
> > +sub zfs_ask_existing_zpool_rename {
> > +my ($pool_name) = @_;
> > +
> > +# At this point, no pools should be imported/active
> > +my $exported_pools = Proxmox::Sys::ZFS::get_exported_pools();
> > +
> > +foreach (@$exported_pools) {
> > + next if $_->{name} ne $pool_name || $_->{state} ne 'ONLINE';
> > + my $renamed_pool = "$_->{name}-OLD-$_->{id}";
> > +
> > + my $response_ok =
> > Proxmox::Install::Config::get_existing_storage_auto_rename();
> maybe we want to name this differently to avoid confusion?
> response_ok -> do_rename
> or something in that regard?
>
> but that could be done in a follow up patch as well if we want to
Makes sense - I'll spin a new revision of this series and address this
directly.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer v3 0/4] add check/rename for already-existing ZFS rpool
Thanks again! On Fri, Jul 12, 2024 at 11:42:58AM GMT, Aaron Lauterer wrote: > tested this series more thoroughly and checked with an existing root pool > all 3 install options: > > * TUI > * GUI > * auto > > Worked well in 3 cases to rename. > > One oddity seems to be if we cancel the rename in the TUI installer. it > jumps to 100% and stays there and I then need to manually abort. Jumping to > a "setup cancelled" box that we confirm before quitting would be nice. This > is how the GUI installer handles it. Hm .. this must have existed since the inception of the TUI installer, so it least isn't a (new) regression then. It just probably never occurred that anyone tested/stumbled upon this in the TUI. Thanks for the notice tho! I'll address it in a separate patch though, as it isn't really related to the series. > > Otherwise, besides this and a tiny nit in patch 4, consider this: > > Reviewed-By: Aaron Lauterer > Tested-By: Aaron Lauterer > > On 2024-07-11 13:57, Christoph Heiss wrote: > > Pretty straight forward overall, implements a check for an existing > > `rpool` on the system and ask the user whether they would like to rename > > it, much in the same way as it works for VGs already. > > > > Without this, the installer would silently create a second (and thus > > conflicting) `rpool` and cause a boot failure after the installation, > > since it does not know which pool to import exactly. > > > > v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063874.html > > v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064619.html > > > > Notable changes v2 -> v3: > >* make low-level option lvm_auto_rename more generic > >* skip rename prompt in auto-install mode > > > > Notable changes v1 -> v2: > >* incorporated Aarons suggestions from v1 > >* rewrote tests to use a pre-defined input instead > >* moved pool renaming to own subroutine > >* documented all new subroutines > >* split out tests into own patch > > > > Christoph Heiss (4): > >test: add test cases for new zfs module > >low-level: install: check for already-existing `rpool` on install > >install: config: rename option lvm_auto_rename -> > > existing_storage_auto_rename > >low-level: install: automatically rename existing rpools on > > auto-installs > > > > Proxmox/Install.pm| 35 +++- > > Proxmox/Install/Config.pm | 6 +- > > Proxmox/Sys/ZFS.pm| 20 ++- > > proxmox-auto-installer/src/utils.rs | 2 +- > > .../resources/parse_answer/disk_match.json| 2 +- > > .../parse_answer/disk_match_all.json | 2 +- > > .../parse_answer/disk_match_any.json | 2 +- > > .../tests/resources/parse_answer/minimal.json | 2 +- > > .../resources/parse_answer/nic_matching.json | 2 +- > > .../resources/parse_answer/specific_nic.json | 2 +- > > .../tests/resources/parse_answer/zfs.json | 2 +- > > proxmox-installer-common/src/setup.rs | 2 +- > > proxmox-tui-installer/src/setup.rs| 2 +- > > test/Makefile | 7 ++- > > test/zfs-get-pool-list.pl | 57 +++ > > 15 files changed, 128 insertions(+), 17 deletions(-) > > create mode 100755 test/zfs-get-pool-list.pl > > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] S3 Support
--- Begin Message --- Hi, to everyone, i'm interested in native S3 support on proxmox, is anyone working on it ( to avoid double work )? Second question, my idea is to reuse existing pbs code but upload chunks and index to s3, sounds good? The thing puzzling me is that a lot of code will end up duplicated , when actually only the part uploading to S3 instead of pbs would be different, chunking, fleeching, restoring, backup, qemu interface would be almost identical --- End Message --- ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] S3 Support
> Tiziano Bacocco via pve-devel hat am 16.07.2024 > 00:20 CEST geschrieben: > Hi, to everyone, i'm interested in native S3 support on proxmox, is anyone > working on it ( to avoid double work )? None of us is, but there is an external developer that is trying to implement something like this. The whole topic might be a better fit for pbs-devel though ;) > Second question, my idea is to reuse existing pbs code but upload chunks > and index to s3, sounds good? The thing puzzling me is that a lot of code > will end up duplicated , when actually only the part uploading to S3 > instead of pbs would be different, chunking, fleeching, restoring, backup, > qemu interface would be almost identical https://bugzilla.proxmox.com/show_bug.cgi?id=2943 backing up directly to S3 will be hard for a multitude of reasons: - you don't want to write individual chunks as objects (too expensive) - you don't want to slow down the guest because of the speed of the object storage - .. instead, a similar approach to the tape backup feature would be needed: - define retention and bucket allocation policies - combine metadata (snapshots/..) and chunks of a local datastore in a meaningful way into buckets - have some sort of index/catalog that tells you which buckets you need to restore which snapshots into a datastore - .. hope this makes sense! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
