[pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Max Carrara]

This marks version 02 of the patch series "Fix #4759: Configure
Permissions for ceph-crash.service" [0]. 

Each patch also lists which changes have been made between versions.

Other changes not otherwise mentioned in patches:
  v1 --> v2:
    * drop patch regarding POSIX compatibility in debian/postinst hook 
      as suggested [1]


Regarding `ceph.client.crash.keyring`
-------------------------------------

One idea [2] that was mentioned was to have a keyring for each host
instead of sharing a keyring on pmxcfs. While that is indeed possible,
it would require host-specific `client.crash` keys, each requiring
a separate section ('client.crash.$HOSTNAME'). Alternatively, the
default search path location may also be changed, however, the Ceph docs
do not recommend this [3]:

> It is possible to override this search-path location by adding a
> `keyring` option in the `[global]` section of your Ceph configuration
> file, but this is not recommended.

In our case, `ceph-crash` appears to try
'/etc/pve/priv/ceph.client.crash.$HOSTNAME.keyring' first (and
subsequently logs an authentication error) before attempting to
authenticate via other clients/keyrings, which aligns with what Fabian
had discovered.

It is therefore easier to:
  * have one keyring file at '/etc/pve/ceph/ceph.client.crash.keyring'
  * have the configuration point to that file
  * patch Ceph in order to make `ceph-crash` try to authenticate with
    'client.crash' first

This gets rid of the unnecessary warnings and errors that otherwise show
up in the systemd journal.

Furthermore, the Ceph Crash docs use 'client.crash' [4], so I personally
doubt that upstream expects that people use the host-specific key;
rather, it seems that any of the three keys is fine.

Further points I have regarding sharing the key via pmxcfs:
  * in case the key & keyring file ever need to be rotated, only a
    single file location and cephx auth entry needs to be updated
    - instead of having to update every host's key *and* each
      corresponding cephx auth entry

  * the key is generated only once, exactly when the first monitor is
    created
    - otherwise we would need to ensure that the host-specific keys
      exist and are reliably added to cephx - e.g. per each use of
      `pveceph init` while also having to set up the first host-specific
      key when the first monitor is created via `pveceph mon create`

  * the keyring file's name doesn't need to be changed if the hostname
    changes (although that's admittedly a very minor benefit)

  * overall less administrative friction for the user (again, single
    location, single cephx auth entry)

Therefore, I decided to keep most things as they were with other
suggestions incorporated. The order of the identities that `ceph-crash`
checks is adapted, in order to "suppress" the warnings in the systemd
journal.

Though, I will of course consider any feedback in this regard,
especially if my reasoning here proves to be erroneous.


[0]: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061546.html
[1]: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061561.html
[2]: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061566.html
[3]: https://docs.ceph.com/en/reef/rados/configuration/auth-config-ref/#enabling-cephx
[4]: https://docs.ceph.com/en/quincy/mgr/crash/#enabling



ceph (master):

Max Carrara (2):
  debian: add patch to fix ceph crash dir permissions in postinst hook
  patches: add patch that reorders clients used by ceph-crash

 ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
 patches/series                                |  2 +
 3 files changed, 82 insertions(+)
 create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
 create mode 100644 patches/0016-ceph-crash-change-order-of-client-names.patch


ceph (quincy-stable-8):

Max Carrara (2):
  debian: add patch to fix ceph crash dir permissions in postinst hook
  patches: add patch that reorders clients used by ceph-crash

 ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
 patches/series                                |  2 +
 3 files changed, 82 insertions(+)
 create mode 100644 patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
 create mode 100644 patches/0025-ceph-crash-change-order-of-client-names.patch


pve-storage:

Max Carrara (3):
  cephconfig: align our parser more with Ceph's parser
  cephconfig: allow writing arbitrary sections
  amend! cephconfig: allow writing arbitrary sections

 src/PVE/CephConfig.pm | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)


pve-manager:

Max Carrara (4):
  ceph: fix edge case of wrong files being deleted on purge
  fix #4759: ceph: configure keyring for ceph-crash.service
  ceph: create '/etc/pve/ceph' during `pveceph init`
  fix #4759: debian/postinst: configure ceph-crash.service and its key

 PVE/API2/Ceph.pm     |   5 ++
 PVE/API2/Ceph/MON.pm |  17 ++++++-
 PVE/Ceph/Tools.pm    |  57 +++++++++++++++++++---
 debian/postinst      | 113 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 185 insertions(+), 7 deletions(-)


-- 
2.39.2

[pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook

[Max Carrara]

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.

This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * use `find` instead of for-loop

 ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 51 insertions(+)
 create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch

diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
new file mode 100644
index 000000000..7445f3945
--- /dev/null
+++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
@@ -0,0 +1,50 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 1 Feb 2024 18:43:36 +0100
+Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by also updating the permissions of
+/var/lib/ceph/*/* - the subdirectories and files of the directories in
+/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 16 +++++++++-------
+ 1 file changed, 9 insertions(+), 7 deletions(-)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..70d07977f82 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -33,13 +33,15 @@ case "$1" in
+ 	rm -f /etc/init/ceph.conf
+ 	[ -x /sbin/start ] && start ceph-all || :
+ 
+-        # adjust file and directory permissions
+-	for DIR in /var/lib/ceph/* ; do
+-	    if ! dpkg-statoverride --list $DIR >/dev/null
+-	    then
+-		chown $SERVER_USER:$SERVER_GROUP $DIR
+-	    fi
+-	done
++	PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
++
++	# adjust directory permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++	# adjust file permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
+     ;;
+     abort-upgrade|abort-remove|abort-deconfigure)
+ 	:
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 865caf23d..cf8f1ea31 100644
--- a/patches/series
+++ b/patches/series
@@ -12,3 +12,4 @@
 0012-backport-mgr-dashboard-simplify-authentication-proto.patch
 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
+0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
-- 
2.39.2

[pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash

[Max Carrara]

This patch makes it so that `ceph-crash` attempts to use the
non-host-specific keyring before anything else, which avoids
unnecessary error messages landing in the systemd-journal in our case.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * new

 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 31 insertions(+)
 create mode 100644 patches/0016-ceph-crash-change-order-of-client-names.patch

diff --git a/patches/0016-ceph-crash-change-order-of-client-names.patch b/patches/0016-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..cc0bdccdb
--- /dev/null
+++ b/patches/0016-ceph-crash-change-order-of-client-names.patch
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Mon, 5 Feb 2024 11:44:14 +0100
+Subject: [PATCH] ceph-crash: change order of client names
+
+This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ src/ceph-crash.in | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/ceph-crash.in b/src/ceph-crash.in
+index 010d6ea2e82..d1d9f4123e6 100755
+--- a/src/ceph-crash.in
++++ b/src/ceph-crash.in
+@@ -16,8 +16,8 @@ import time
+ logging.basicConfig(level=logging.INFO)
+ log = logging.getLogger('ceph-crash')
+ 
+-auth_names = ['client.crash.%s' % socket.gethostname(),
+-              'client.crash',
++auth_names = ['client.crash',
++              'client.crash.%s' % socket.gethostname(),
+               'client.admin']
+ 
+ 
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index cf8f1ea31..1c78e62ba 100644
--- a/patches/series
+++ b/patches/series
@@ -13,3 +13,4 @@
 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
 0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
+0016-ceph-crash-change-order-of-client-names.patch
-- 
2.39.2

[pve-devel] [PATCH v2 quincy-stable-8 ceph 03/11] debian: add patch to fix ceph crash dir permissions in postinst hook

[Max Carrara]

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.

This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * use `find` instead of for-loop

 ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 51 insertions(+)
 create mode 100644 patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch

diff --git a/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
new file mode 100644
index 000000000..7445f3945
--- /dev/null
+++ b/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
@@ -0,0 +1,50 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 1 Feb 2024 18:43:36 +0100
+Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by also updating the permissions of
+/var/lib/ceph/*/* - the subdirectories and files of the directories in
+/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 16 +++++++++-------
+ 1 file changed, 9 insertions(+), 7 deletions(-)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..70d07977f82 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -33,13 +33,15 @@ case "$1" in
+ 	rm -f /etc/init/ceph.conf
+ 	[ -x /sbin/start ] && start ceph-all || :
+ 
+-        # adjust file and directory permissions
+-	for DIR in /var/lib/ceph/* ; do
+-	    if ! dpkg-statoverride --list $DIR >/dev/null
+-	    then
+-		chown $SERVER_USER:$SERVER_GROUP $DIR
+-	    fi
+-	done
++	PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
++
++	# adjust directory permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++	# adjust file permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
+     ;;
+     abort-upgrade|abort-remove|abort-deconfigure)
+ 	:
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index ee897a78a..34185c7da 100644
--- a/patches/series
+++ b/patches/series
@@ -16,3 +16,4 @@
 0021-backport-mgr-dashboard-simplify-authentication-proto.patch
 0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0023-rocksb-inherit-parent-cmake-cxx-flags.patch
+0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
-- 
2.39.2

[pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash

[Max Carrara]

This patch makes it so that `ceph-crash` attempts to use the
non-host-specific keyring before anything else, which avoids
unnecessary error messages landing in the systemd-journal in our case.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * new

 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 31 insertions(+)
 create mode 100644 patches/0025-ceph-crash-change-order-of-client-names.patch

diff --git a/patches/0025-ceph-crash-change-order-of-client-names.patch b/patches/0025-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..cc0bdccdb
--- /dev/null
+++ b/patches/0025-ceph-crash-change-order-of-client-names.patch
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Mon, 5 Feb 2024 11:44:14 +0100
+Subject: [PATCH] ceph-crash: change order of client names
+
+This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ src/ceph-crash.in | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/ceph-crash.in b/src/ceph-crash.in
+index 010d6ea2e82..d1d9f4123e6 100755
+--- a/src/ceph-crash.in
++++ b/src/ceph-crash.in
+@@ -16,8 +16,8 @@ import time
+ logging.basicConfig(level=logging.INFO)
+ log = logging.getLogger('ceph-crash')
+ 
+-auth_names = ['client.crash.%s' % socket.gethostname(),
+-              'client.crash',
++auth_names = ['client.crash',
++              'client.crash.%s' % socket.gethostname(),
+               'client.admin']
+ 
+ 
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 34185c7da..8160de645 100644
--- a/patches/series
+++ b/patches/series
@@ -17,3 +17,4 @@
 0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0023-rocksb-inherit-parent-cmake-cxx-flags.patch
 0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
+0025-ceph-crash-change-order-of-client-names.patch
-- 
2.39.2

[pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser

[Max Carrara]

 1. Comments, irrespective of whether they start with '#' or ';' are
    now treated the same. Otherwise, sections and key-value pairs with
    a trailing comment starting with ';' are still parsed. Consider
    this example:

      [some.section] # inline comment after section
      foo = bar ; inline comment after value

     The '[some.section]' section in the example above would otherwise
     not be parsed at all, while in the key-value definition 'foo'
     parses as the key, which is correct, but 'bar ; inline comment
     after value' parses as value, which is incorrect according to
     Ceph's grammar [0][1].

 2. Sections may now contain any character, including whitespace, but
    not '\n' or a comment literal '#' or ';'. The case for comment
    literals is handled in 1. above.

 3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
    are treated the same, like in Ceph's parser [2].

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
[1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
[2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * new

 src/PVE/CephConfig.pm | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b10d46..77b745f 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
 		  \&parse_ceph_config,
 		  \&write_ceph_config);
 
+# For more details on how Ceph's config parser works, see:
+# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
 sub parse_ceph_config {
     my ($filename, $raw) = @_;
 
@@ -20,14 +22,13 @@ sub parse_ceph_config {
 
     my $section;
 
-    foreach my $line (@lines) {
-	$line =~ s/#.*$//;
+    for my $line (@lines) {
+	$line =~ s/(#|;).*$//;
 	$line =~ s/^\s+//;
-	$line =~ s/^;.*$//;
 	$line =~ s/\s+$//;
 	next if !$line;
 
-	$section = $1 if $line =~ m/^\[(\S+)\]$/;
+	$section = $1 if $line =~ m/^\[(.+)\]$/;
 	if (!$section) {
 	    warn "no section - skip: $line\n";
 	    next;
@@ -35,11 +36,10 @@ sub parse_ceph_config {
 
 	if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
 	    my ($key, $val) = ($1, $2);
-	    # ceph treats ' ', '_' and '-' in keys the same, so lets do too
-	    $key =~ s/[-\ ]/_/g;
+	    # ceph treats ' ' and '_' in keys the same, so lets do too
+	    $key =~ s/ /_/g;
 	    $cfg->{$section}->{$key} = $val;
 	}
-
     }
 
     return $cfg;
-- 
2.39.2

[pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections

[Max Carrara]

This adds support for writing arbitrary sections to 'ceph.conf' while
ensuring that already written sections are not duplicated.

Sections that are associated with the client, for example
'[client.foo]', are written directly after the '[client]' section.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * Instead of just adding 'client.crash' as a separate section,
    also allow writing arbitrary sections

 src/PVE/CephConfig.pm | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 77b745f..86d3079 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
 sub write_ceph_config {
     my ($filename, $cfg) = @_;
 
+    my $written_sections = {};
     my $out = '';
 
     my $cond_write_sec = sub {
 	my $re = shift;
 
-	foreach my $section (sort keys %$cfg) {
-	    next if $section !~ m/^$re$/;
+	for my $section (sort keys %$cfg) {
+	    if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
+		next;
+	    }
+
 	    $out .= "[$section]\n";
-	    foreach my $key (sort keys %{$cfg->{$section}}) {
-		$out .= "\t $key = $cfg->{$section}->{$key}\n";
+	    for my $key (sort keys %{$cfg->{$section}}) {
+		$out .= "\t$key = $cfg->{$section}->{$key}\n";
 	    }
 	    $out .= "\n";
+
+	    $written_sections->{$section} = 1;
 	}
     };
 
     &$cond_write_sec('global');
     &$cond_write_sec('client');
+    &$cond_write_sec('client\..*');
 
     &$cond_write_sec('mds');
     &$cond_write_sec('mon');
@@ -88,6 +95,8 @@ sub write_ceph_config {
     &$cond_write_sec('osd\..*');
     &$cond_write_sec('mgr\..*');
 
+    &$cond_write_sec('.*');
+
     return $out;
 }
 
-- 
2.39.2

[pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections

[Max Carrara]

cephconfig: allow writing arbitrary sections

This adds support for writing arbitrary sections to 'ceph.conf' while
ensuring that already written sections are not duplicated.

Sections that are associated with the client, for example
'[client.foo]', are written directly after the '[client]' section.

Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
written directly after their associated section.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
NOTE: This amend really just changes the order of the sections below and
      may be dropped if not desired.

Changes v1 --> v2:
  * new

 src/PVE/CephConfig.pm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 86d3079..00b1f35 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -82,17 +82,20 @@ sub write_ceph_config {
     };
 
     &$cond_write_sec('global');
+
     &$cond_write_sec('client');
     &$cond_write_sec('client\..*');
 
     &$cond_write_sec('mds');
-    &$cond_write_sec('mon');
-    &$cond_write_sec('osd');
-    &$cond_write_sec('mgr');
-
     &$cond_write_sec('mds\..*');
+
+    &$cond_write_sec('mon');
     &$cond_write_sec('mon\..*');
+
+    &$cond_write_sec('osd');
     &$cond_write_sec('osd\..*');
+
+    &$cond_write_sec('mgr');
     &$cond_write_sec('mgr\..*');
 
     &$cond_write_sec('.*');
-- 
2.39.2

[pve-devel] [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge

[Max Carrara]

Having a file named e.g. "60" in your current directory will cause it
to be deleted when executing `pveceph purge`. This commit fixes that
by making the config hash differ between which values represent file
paths and which don't.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:as
  * use two hashes instead of one large nested hash to differ between
    config values that correspond to files and adapt surrounding logic
    correspondingly

 PVE/Ceph/Tools.pm | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index a1458b40..273a3eb6 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -35,15 +35,18 @@ my $ceph_service = {
     ceph_volume => '/usr/sbin/ceph-volume',
 };
 
-my $config_hash = {
+my $config_values = {
     ccname => $ccname,
+    ceph_mds_data_dir => $ceph_mds_data_dir,
+    long_rados_timeout => 60,
+};
+
+my $config_files = {
     pve_ceph_cfgpath => $pve_ceph_cfgpath,
     pve_mon_key_path => $pve_mon_key_path,
     pve_ckeyring_path => $pve_ckeyring_path,
     ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
     ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
-    ceph_mds_data_dir => $ceph_mds_data_dir,
-    long_rados_timeout => 60,
     ceph_cfgpath => $ceph_cfgpath,
 };
 
@@ -84,9 +87,12 @@ sub get_cluster_versions {
 sub get_config {
     my $key = shift;
 
-    my $value = $config_hash->{$key};
+    my $value = $config_values->{$key};
+    if (! defined($value)) {
+	$value = $config_files->{$key};
+    }
 
-    die "no such ceph config '$key'" if !$value;
+    die "no such ceph config '$key'" if ! defined($value);
 
     return $value;
 }
@@ -123,7 +129,7 @@ sub purge_all_ceph_files {
 	warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
     } else {
 	print "Removing config & keyring files\n";
-	foreach my $file (%$config_hash) {
+	for my $file (%$config_files) {
 	    unlink $file if (-e $file);
 	}
     }
-- 
2.39.2

[pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service

[Max Carrara]

when creating the cluster's first monitor.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * do not enable/restart `ceph-crash` anymore when creating first mon
  * drop changes to function `ceph_service_cmd` as they are no longer
    needed
  * create keyring for `ceph-crash` before modifying 'ceph.conf'
  * always set keyring for 'client.crash' section instead of only
    if section doesn't exist already
  * only modify the keyring file in `get_or_create_crash_keyring()`
    if the content differs from the output of `ceph auth get-or-create`

 PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
 PVE/Ceph/Tools.pm    | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..ae12a2d3 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
 	    });
 	    die $@ if $@;
 	    # automatically create manager after the first monitor is created
+	    # and set up keyring and config for ceph-crash.service
 	    if ($is_first_monitor) {
 		PVE::API2::Ceph::MGR->createmgr({
 		    node => $param->{node},
 		    id => $param->{node}
-		})
+		});
+
+		eval {
+		    PVE::Ceph::Tools::get_or_create_crash_keyring();
+		};
+		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
+
+		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
+		    my $cfg = cfs_read_file('ceph.conf');
+
+		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
+
+		    cfs_write_file('ceph.conf', $cfg);
+		});
+		die $@ if $@;
 	    }
 	};
 
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 273a3eb6..02a932e3 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
 my $ceph_cfgdir = "/etc/ceph";
 my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
 my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
+my $pve_ceph_cfgdir = "/etc/pve/ceph";
 
+my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
 my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
 my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
 my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
@@ -37,12 +39,14 @@ my $ceph_service = {
 
 my $config_values = {
     ccname => $ccname,
+    pve_ceph_cfgdir => $pve_ceph_cfgdir,
     ceph_mds_data_dir => $ceph_mds_data_dir,
     long_rados_timeout => 60,
 };
 
 my $config_files = {
     pve_ceph_cfgpath => $pve_ceph_cfgpath,
+    pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
     pve_mon_key_path => $pve_mon_key_path,
     pve_ckeyring_path => $pve_ckeyring_path,
     ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
@@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
     return $pve_ckeyring_path;
 }
 
+# requires connection to existing monitor
+sub get_or_create_crash_keyring {
+    my ($rados) = @_;
+
+    if (!defined($rados)) {
+	$rados = PVE::RADOS->new();
+    }
+
+    my $output = $rados->mon_command({
+	prefix => 'auth get-or-create',
+	entity => 'client.crash',
+	caps => [
+	    mon => 'profile crash',
+	    mgr => 'profile crash',
+	],
+	format => 'plain',
+    });
+
+    if (! -d $pve_ceph_cfgdir) {
+	File::Path::make_path($pve_ceph_cfgdir);
+    }
+
+    if (-f $pve_ceph_crash_key_path) {
+	my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
+
+	if ($contents ne $output) {
+	    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+	}
+    } else {
+	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+    }
+
+    return $pve_ceph_crash_key_path;
+}
+
 # get ceph-volume managed osds
 sub ceph_volume_list {
     my $result = {};
-- 
2.39.2

[pve-devel] [PATCH v2 pve-manager 10/11] ceph: create '/etc/pve/ceph' during `pveceph init`

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * none

 PVE/API2/Ceph.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index 81c17d6e..7fedb87a 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -192,6 +192,11 @@ __PACKAGE__->register_method ({
 	    PVE::Ceph::Tools::check_ceph_installed('ceph_bin');
 	}
 
+	my $pve_ceph_cfgdir = PVE::Ceph::Tools::get_config('pve_ceph_cfgdir');
+	if (! -d $pve_ceph_cfgdir) {
+	    File::Path::make_path($pve_ceph_cfgdir);
+	}
+
 	my $auth = $param->{disable_cephx} ? 'none' : 'cephx';
 
 	# simply load old config if it already exists
-- 
2.39.2

[pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key

[Max Carrara]

This commit adds the `set_ceph_crash_conf` function, which dynamically
adapts the host's Ceph configuration in order to allow the Ceph crash
module's daemon to run without elevated privileges.

This adaptation is only performed if:
 * Ceph is installed
 * Ceph is configured ('/etc/pve/ceph.conf' exists)
 * Connection to RADOS is successful

If the above conditions are met, the function will ensure that:
 * Ceph possesses a key named 'client.crash'
 * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
 * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
 * The 'client.crash' section has a key named 'keyring' which
   references '/etc/pve/ceph/ceph.client.crash.keyring'

Furthermore, if a key named 'client.crash' already exists within the
cluster, it shall be reused and not regenerated. Also, the
configuration is not altered if the conditions above are already met.

This way the keyring file is available as read-only in
'/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
Because the `ceph` user has been made part of said `www-data` group
[0], it may access the file without requiring any additional
privileges.

Thus, the configuration for the Ceph crash daemon is safely adapted as
expected by PVE tooling and also shared via pmxcfs across one's
cluster.

[0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * fix 'keyring' key being appended to 'client.crash' section even
    if it already exists and configured correctly

 debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/debian/postinst b/debian/postinst
index 6138ef6d..267a62ae 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
     fi
 }
 
+set_ceph_crash_conf() {
+    PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
+    PVE_CEPH_CONFDIR='/etc/pve/ceph'
+    PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
+    PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
+
+    # ceph isn't installed -> nothing to do
+    if ! which ceph > /dev/null 2>&1; then
+        return 0
+    fi
+
+    # ceph isn't configured -> nothing to do
+    if test ! -f "${PVE_CEPH_CONFFILE}"; then
+        return 0
+    fi
+
+    CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
+
+    # ceph is installed and possibly configured, but no connection to RADOS
+    # -> assume no monitor was created, nothing to do
+    if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
+        return 0
+    fi
+
+    SECTION_RE='^\[\S+\]$'
+    CRASH_SECTION_RE='^\[client\.crash\]$'
+
+    if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
+        DO_RESTART_UNIT=0
+        CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
+
+        if test ! -d "${PVE_CEPH_CONFDIR}"; then
+            mkdir -p "${PVE_CEPH_CONFDIR}"
+        fi
+
+        # keyring file doesn't exist or contains wrong key
+        if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
+            echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
+            echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
+            DO_RESTART_UNIT=1
+        fi
+
+        # 'client.crash' section is in conf file
+        if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
+            IFS=''
+            NEW_PVE_CEPH_CONFFILE=''
+            IN_CRASH_SECTION=0
+            HAS_KEYRING=0
+            REPLACED_KEYRING=0
+
+            # look for 'keyring' key in 'client.crash' section
+            # -> replace it if it points to the wrong location
+            while read -r LINE; do
+                if test "${IN_CRASH_SECTION}" = "1"; then
+                    if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
+                        IN_CRASH_SECTION=0
+                    elif echo "${LINE}" | grep -q -E '\s+keyring'; then
+                        HAS_KEYRING=1
+
+                        if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
+                            echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
+                            LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
+                            REPLACED_KEYRING=1
+                        fi
+                    fi
+                elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
+                    IN_CRASH_SECTION=1
+                fi
+
+                NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
+            done < "${PVE_CEPH_CONFFILE}"
+
+            unset IFS
+
+            if test "${HAS_KEYRING}" = "1"; then
+                # 'keyring' key was replaced -> write to file
+                if test "${REPLACED_KEYRING}" = "1"; then
+                    echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
+                    DO_RESTART_UNIT=1
+                fi
+
+            # client.crash section exists, but contained no 'keyring' key
+            # -> put 'keyring' key into 'client.crash' section
+            else
+                sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
+                    "${PVE_CEPH_CONFFILE}"
+                DO_RESTART_UNIT=1
+            fi
+
+        # 'client.crash' section doesn't exist -> add it
+        else
+            echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
+            printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
+                >> "${PVE_CEPH_CONFFILE}"
+            DO_RESTART_UNIT=1
+        fi
+
+        if test "${DO_RESTART_UNIT}" = "1"; then
+            UNIT='ceph-crash.service'
+
+            if systemctl -q is-enabled "${UNIT}"; then
+                echo "Restarting ceph-crash.service"
+                deb-systemd-invoke restart "${UNIT}"
+            fi
+        fi
+
+    else
+        echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
+        printf '%s\n\n' "${CEPH_AUTH_RES}"
+    fi
+}
+
 case "$1" in
   triggered)
     # We don't print a status message here, as dpkg already said
@@ -189,6 +301,7 @@ case "$1" in
     fi
 
     set_lvm_conf
+    set_ceph_crash_conf
 
     if test ! -e /proxmox_install_mode; then
         # modeled after code generated by dh_start
-- 
2.39.2

Re: [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
> 
> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * use `find` instead of for-loop
> 
>  ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
>  patches/series                                |  1 +
>  2 files changed, 51 insertions(+)
>  create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> 
> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> new file mode 100644
> index 000000000..7445f3945
> --- /dev/null
> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> @@ -0,0 +1,50 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Thu, 1 Feb 2024 18:43:36 +0100
> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
> +
> +A rather recent PR made ceph-crash run as "ceph" user instead of
> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
> +ceph-crash cannot actually post any crash logs now.
> +
> +This commit fixes this by also updating the permissions of
> +/var/lib/ceph/*/* - the subdirectories and files of the directories in
> +/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
> +
> +[0]: https://github.com/ceph/ceph/pull/48713
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + debian/ceph-base.postinst | 16 +++++++++-------
> + 1 file changed, 9 insertions(+), 7 deletions(-)
> +
> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
> +index 75eeb59c624..70d07977f82 100644
> +--- a/debian/ceph-base.postinst
> ++++ b/debian/ceph-base.postinst
> +@@ -33,13 +33,15 @@ case "$1" in
> + 	rm -f /etc/init/ceph.conf
> + 	[ -x /sbin/start ] && start ceph-all || :
> + 
> +-        # adjust file and directory permissions
> +-	for DIR in /var/lib/ceph/* ; do
> +-	    if ! dpkg-statoverride --list $DIR >/dev/null
> +-	    then
> +-		chown $SERVER_USER:$SERVER_GROUP $DIR
> +-	    fi
> +-	done
> ++	PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"

this doesn't quote {} properly, files with spaces or other things in
them can cause issues including shell command injection as root when
this is executed by dpkg..

> ++
> ++	# adjust directory permissions
> ++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
> ++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
> ++
> ++	# adjust file permissions
> ++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
> ++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
> +     ;;

do we need the depth stuff or the split by type? we want to make
everything under /var/lib/ceph owned by ceph:ceph, unless the admin has
specifically overriden a particular path.

a simple 

find /var/lib/ceph -print0 | ...

should work and be much simpler (or if we want to limit to dirs and
files, that can also be simply done in one go by ORing the two checks)

> +     abort-upgrade|abort-remove|abort-deconfigure)
> + 	:
> +-- 
> +2.39.2
> +
> diff --git a/patches/series b/patches/series
> index 865caf23d..cf8f1ea31 100644
> --- a/patches/series
> +++ b/patches/series
> @@ -12,3 +12,4 @@
>  0012-backport-mgr-dashboard-simplify-authentication-proto.patch
>  0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>  0014-rocksb-inherit-parent-cmake-cxx-flags.patch
> +0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 quincy-stable-8 ceph 03/11] debian: add patch to fix ceph crash dir permissions in postinst hook

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
> 
> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * use `find` instead of for-loop

same comments as for patch #1 apply here ;)

Re: [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> This patch makes it so that `ceph-crash` attempts to use the
> non-host-specific keyring before anything else, which avoids
> unnecessary error messages landing in the systemd-journal in our case.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

worst case, a user that has already manually setup client.crash.$host
will now get spurios warnings, but crashes woulds till get posted
anyhow..

> ---
> Changes v1 --> v2:
>   * new
> 
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
>  patches/series                                |  1 +
>  2 files changed, 31 insertions(+)
>  create mode 100644 patches/0016-ceph-crash-change-order-of-client-names.patch
> 
> diff --git a/patches/0016-ceph-crash-change-order-of-client-names.patch b/patches/0016-ceph-crash-change-order-of-client-names.patch
> new file mode 100644
> index 000000000..cc0bdccdb
> --- /dev/null
> +++ b/patches/0016-ceph-crash-change-order-of-client-names.patch
> @@ -0,0 +1,30 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Mon, 5 Feb 2024 11:44:14 +0100
> +Subject: [PATCH] ceph-crash: change order of client names
> +
> +This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + src/ceph-crash.in | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/src/ceph-crash.in b/src/ceph-crash.in
> +index 010d6ea2e82..d1d9f4123e6 100755
> +--- a/src/ceph-crash.in
> ++++ b/src/ceph-crash.in
> +@@ -16,8 +16,8 @@ import time
> + logging.basicConfig(level=logging.INFO)
> + log = logging.getLogger('ceph-crash')
> + 
> +-auth_names = ['client.crash.%s' % socket.gethostname(),
> +-              'client.crash',
> ++auth_names = ['client.crash',
> ++              'client.crash.%s' % socket.gethostname(),
> +               'client.admin']
> + 
> + 
> +-- 
> +2.39.2
> +
> diff --git a/patches/series b/patches/series
> index cf8f1ea31..1c78e62ba 100644
> --- a/patches/series
> +++ b/patches/series
> @@ -13,3 +13,4 @@
>  0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>  0014-rocksb-inherit-parent-cmake-cxx-flags.patch
>  0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> +0016-ceph-crash-change-order-of-client-names.patch
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> This patch makes it so that `ceph-crash` attempts to use the
> non-host-specific keyring before anything else, which avoids
> unnecessary error messages landing in the systemd-journal in our case.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> ---
> Changes v1 --> v2:
>   * new
> 
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
>  patches/series                                |  1 +
>  2 files changed, 31 insertions(+)
>  create mode 100644 patches/0025-ceph-crash-change-order-of-client-names.patch
> 
> diff --git a/patches/0025-ceph-crash-change-order-of-client-names.patch b/patches/0025-ceph-crash-change-order-of-client-names.patch
> new file mode 100644
> index 000000000..cc0bdccdb
> --- /dev/null
> +++ b/patches/0025-ceph-crash-change-order-of-client-names.patch
> @@ -0,0 +1,30 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Mon, 5 Feb 2024 11:44:14 +0100
> +Subject: [PATCH] ceph-crash: change order of client names
> +
> +This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + src/ceph-crash.in | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/src/ceph-crash.in b/src/ceph-crash.in
> +index 010d6ea2e82..d1d9f4123e6 100755
> +--- a/src/ceph-crash.in
> ++++ b/src/ceph-crash.in
> +@@ -16,8 +16,8 @@ import time
> + logging.basicConfig(level=logging.INFO)
> + log = logging.getLogger('ceph-crash')
> + 
> +-auth_names = ['client.crash.%s' % socket.gethostname(),
> +-              'client.crash',
> ++auth_names = ['client.crash',
> ++              'client.crash.%s' % socket.gethostname(),
> +               'client.admin']
> + 
> + 
> +-- 
> +2.39.2
> +
> diff --git a/patches/series b/patches/series
> index 34185c7da..8160de645 100644
> --- a/patches/series
> +++ b/patches/series
> @@ -17,3 +17,4 @@
>  0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>  0023-rocksb-inherit-parent-cmake-cxx-flags.patch
>  0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> +0025-ceph-crash-change-order-of-client-names.patch
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
>  1. Comments, irrespective of whether they start with '#' or ';' are
>     now treated the same. Otherwise, sections and key-value pairs with
>     a trailing comment starting with ';' are still parsed. Consider
>     this example:
> 
>       [some.section] # inline comment after section
>       foo = bar ; inline comment after value
> 
>      The '[some.section]' section in the example above would otherwise
>      not be parsed at all, while in the key-value definition 'foo'
>      parses as the key, which is correct, but 'bar ; inline comment
>      after value' parses as value, which is incorrect according to
>      Ceph's grammar [0][1].
> 
>  2. Sections may now contain any character, including whitespace, but
>     not '\n' or a comment literal '#' or ';'. The case for comment
>     literals is handled in 1. above.

these seem sensible - what about line continuations? ;)

> 
>  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
>     are treated the same, like in Ceph's parser [2].

the ceph docs state something else - which is wrong? ;)

https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/ says:

> Each of the Ceph configuration options has a unique name that consists
> of words formed with lowercase characters and connected with
> underscore characters (_).

this would seem to agree

> When option names are specified on the command line, underscore (_)
> and dash (-) characters can be used interchangeably (for example,
> --mon-host is equivalent to --mon_host).

okay, this is CLI which might just have its own mapping

> When option names appear in configuration files, spaces can also be
> used in place of underscores or dashes. However, for the sake of
> clarity and convenience, we suggest that you consistently use
> underscores, as we do throughout this documentation.

but this now says that dash in config files is OK?

> [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
> [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
> [2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * new
> 
>  src/PVE/CephConfig.pm | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 6b10d46..77b745f 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
>  		  \&parse_ceph_config,
>  		  \&write_ceph_config);
>  
> +# For more details on how Ceph's config parser works, see:
> +# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
>  sub parse_ceph_config {
>      my ($filename, $raw) = @_;
>  
> @@ -20,14 +22,13 @@ sub parse_ceph_config {
>  
>      my $section;
>  
> -    foreach my $line (@lines) {
> -	$line =~ s/#.*$//;
> +    for my $line (@lines) {
> +	$line =~ s/(#|;).*$//;
>  	$line =~ s/^\s+//;
> -	$line =~ s/^;.*$//;
>  	$line =~ s/\s+$//;
>  	next if !$line;
>  
> -	$section = $1 if $line =~ m/^\[(\S+)\]$/;
> +	$section = $1 if $line =~ m/^\[(.+)\]$/;
>  	if (!$section) {
>  	    warn "no section - skip: $line\n";
>  	    next;
> @@ -35,11 +36,10 @@ sub parse_ceph_config {
>  
>  	if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
>  	    my ($key, $val) = ($1, $2);
> -	    # ceph treats ' ', '_' and '-' in keys the same, so lets do too
> -	    $key =~ s/[-\ ]/_/g;
> +	    # ceph treats ' ' and '_' in keys the same, so lets do too
> +	    $key =~ s/ /_/g;
>  	    $cfg->{$section}->{$key} = $val;
>  	}
> -
>      }
>  
>      return $cfg;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> This adds support for writing arbitrary sections to 'ceph.conf' while
> ensuring that already written sections are not duplicated.
> 
> Sections that are associated with the client, for example
> '[client.foo]', are written directly after the '[client]' section.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

would have been easier to parse if the style cleanup and actual
behaviour change would have been split..

style change:

> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 6b10d46..34c3107 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -65,10 +65,10 @@ sub write_ceph_config {
>      my $cond_write_sec = sub {
>  	my $re = shift;
>  
> -	foreach my $section (sort keys %$cfg) {
> +	for my $section (sort keys %$cfg) {
>  	    next if $section !~ m/^$re$/;
>  	    $out .= "[$section]\n";
> -	    foreach my $key (sort keys %{$cfg->{$section}}) {
> +	    for my $key (sort keys %{$cfg->{$section}}) {
>  		$out .= "\t $key = $cfg->{$section}->{$key}\n";
>  	    }
>  	    $out .= "\n";

actual change (small nits inline!):

> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 34c3107..24bc78c 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
>  sub write_ceph_config {
>      my ($filename, $cfg) = @_;
>  
> +    my $written_sections = {};
>      my $out = '';
>  
>      my $cond_write_sec = sub {
>  	my $re = shift;
>  
>  	for my $section (sort keys %$cfg) {
> -	    next if $section !~ m/^$re$/;
> +	    if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
> +		next;
> +	    }

these two could be more clearly written as

next if $written_sections->{section};
next if $section !~ m/$re$/;

since the two checks are not related in any way, other than both having
the same effect if true (skipping that section). the second line is then
unchanged, and both the diff and the resulting code is easier to parse
IMHO.

> +
>  	    $out .= "[$section]\n";
>  	    for my $key (sort keys %{$cfg->{$section}}) {
> -		$out .= "\t $key = $cfg->{$section}->{$key}\n";
> +		$out .= "\t$key = $cfg->{$section}->{$key}\n";

this part here is not mentioned at all, and I might have missed it if I
hadn't split the diffs ;)

>  	    }
>  	    $out .= "\n";
> +
> +	    $written_sections->{$section} = 1;
>  	}
>      };
>  
>      &$cond_write_sec('global');
>      &$cond_write_sec('client');
> +    &$cond_write_sec('client\..*');
>  
>      &$cond_write_sec('mds');
>      &$cond_write_sec('mon');
> @@ -88,6 +95,8 @@ sub write_ceph_config {
>      &$cond_write_sec('osd\..*');
>      &$cond_write_sec('mgr\..*');
>  
> +    &$cond_write_sec('.*');
> +
>      return $out;
>  }

Re: [pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> cephconfig: allow writing arbitrary sections
> 
> This adds support for writing arbitrary sections to 'ceph.conf' while
> ensuring that already written sections are not duplicated.
> 
> Sections that are associated with the client, for example
> '[client.foo]', are written directly after the '[client]' section.
> 
> Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
> written directly after their associated section.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> NOTE: This amend really just changes the order of the sections below and
>       may be dropped if not desired.

does the order matter at all? the docs would imply that it doesn't,
since there is a clear precedence between [global], [$type], and
[$type.$id] (e.g., global -> client -> client.crash)..

https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#configuration-sections

> 
> Changes v1 --> v2:
>   * new
> 
>  src/PVE/CephConfig.pm | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 86d3079..00b1f35 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -82,17 +82,20 @@ sub write_ceph_config {
>      };
>  
>      &$cond_write_sec('global');
> +
>      &$cond_write_sec('client');
>      &$cond_write_sec('client\..*');
>  
>      &$cond_write_sec('mds');
> -    &$cond_write_sec('mon');
> -    &$cond_write_sec('osd');
> -    &$cond_write_sec('mgr');
> -
>      &$cond_write_sec('mds\..*');
> +
> +    &$cond_write_sec('mon');
>      &$cond_write_sec('mon\..*');
> +
> +    &$cond_write_sec('osd');
>      &$cond_write_sec('osd\..*');
> +
> +    &$cond_write_sec('mgr');
>      &$cond_write_sec('mgr\..*');
>  
>      &$cond_write_sec('.*');
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] applied: [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge

[Fabian Grünbichler]

applied this one with a small style follow-up:

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 273a3eb63..ee6c515cb 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -87,10 +87,7 @@ sub get_cluster_versions {
 sub get_config {
     my $key = shift;
 
-    my $value = $config_values->{$key};
-    if (! defined($value)) {
-	$value = $config_files->{$key};
-    }
+    my $value = $config_values->{$key} // $config_files->{$key};
 
     die "no such ceph config '$key'" if ! defined($value);
 

On February 5, 2024 6:54 pm, Max Carrara wrote:
> Having a file named e.g. "60" in your current directory will cause it
> to be deleted when executing `pveceph purge`. This commit fixes that
> by making the config hash differ between which values represent file
> paths and which don't.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:as
>   * use two hashes instead of one large nested hash to differ between
>     config values that correspond to files and adapt surrounding logic
>     correspondingly
> 
>  PVE/Ceph/Tools.pm | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index a1458b40..273a3eb6 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -35,15 +35,18 @@ my $ceph_service = {
>      ceph_volume => '/usr/sbin/ceph-volume',
>  };
>  
> -my $config_hash = {
> +my $config_values = {
>      ccname => $ccname,
> +    ceph_mds_data_dir => $ceph_mds_data_dir,
> +    long_rados_timeout => 60,
> +};
> +
> +my $config_files = {
>      pve_ceph_cfgpath => $pve_ceph_cfgpath,
>      pve_mon_key_path => $pve_mon_key_path,
>      pve_ckeyring_path => $pve_ckeyring_path,
>      ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
>      ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
> -    ceph_mds_data_dir => $ceph_mds_data_dir,
> -    long_rados_timeout => 60,
>      ceph_cfgpath => $ceph_cfgpath,
>  };
>  
> @@ -84,9 +87,12 @@ sub get_cluster_versions {
>  sub get_config {
>      my $key = shift;
>  
> -    my $value = $config_hash->{$key};
> +    my $value = $config_values->{$key};
> +    if (! defined($value)) {
> +	$value = $config_files->{$key};
> +    }
>  
> -    die "no such ceph config '$key'" if !$value;
> +    die "no such ceph config '$key'" if ! defined($value);
>  
>      return $value;
>  }
> @@ -123,7 +129,7 @@ sub purge_all_ceph_files {
>  	warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
>      } else {
>  	print "Removing config & keyring files\n";
> -	foreach my $file (%$config_hash) {
> +	for my $file (%$config_files) {
>  	    unlink $file if (-e $file);
>  	}
>      }
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> when creating the cluster's first monitor.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * do not enable/restart `ceph-crash` anymore when creating first mon
>   * drop changes to function `ceph_service_cmd` as they are no longer
>     needed
>   * create keyring for `ceph-crash` before modifying 'ceph.conf'
>   * always set keyring for 'client.crash' section instead of only
>     if section doesn't exist already
>   * only modify the keyring file in `get_or_create_crash_keyring()`
>     if the content differs from the output of `ceph auth get-or-create`

you kinda ignored my comment about this adding yet another create keyring helper ;)

>  PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
>  PVE/Ceph/Tools.pm    | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..ae12a2d3 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>  	    });
>  	    die $@ if $@;
>  	    # automatically create manager after the first monitor is created
> +	    # and set up keyring and config for ceph-crash.service
>  	    if ($is_first_monitor) {
>  		PVE::API2::Ceph::MGR->createmgr({
>  		    node => $param->{node},
>  		    id => $param->{node}
> -		})
> +		});
> +
> +		eval {
> +		    PVE::Ceph::Tools::get_or_create_crash_keyring();

this is called get_or_create, but it actually returns the path to, not
the key(ring).. nothing uses the return value anyway, so it could also
be called differently I guess and not return anything, but just from the
name, I'd expect the key to be returned.

> +		};
> +		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
> +
> +		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
> +		    my $cfg = cfs_read_file('ceph.conf');
> +
> +		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';

I guess this doesn't make anything worse (since we starting from
"ceph-crash is totally broken"), but if querying or creating the keyring
failed, does it make sense to reference it in the config?

> +
> +		    cfs_write_file('ceph.conf', $cfg);
> +		});
> +		die $@ if $@;

we could move this whole part up to where we do the monitor changes, and
only lock and read and write the config once..

>  	    }
>  	};
>  
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 273a3eb6..02a932e3 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
>  my $ceph_cfgdir = "/etc/ceph";
>  my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
>  my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>  
> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
>  my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>  my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>  my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
> @@ -37,12 +39,14 @@ my $ceph_service = {
>  
>  my $config_values = {
>      ccname => $ccname,
> +    pve_ceph_cfgdir => $pve_ceph_cfgdir,
>      ceph_mds_data_dir => $ceph_mds_data_dir,
>      long_rados_timeout => 60,
>  };
>  
>  my $config_files = {
>      pve_ceph_cfgpath => $pve_ceph_cfgpath,
> +    pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
>      pve_mon_key_path => $pve_mon_key_path,
>      pve_ckeyring_path => $pve_ckeyring_path,
>      ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>      return $pve_ckeyring_path;
>  }
>  
> +# requires connection to existing monitor
> +sub get_or_create_crash_keyring {
> +    my ($rados) = @_;
> +
> +    if (!defined($rados)) {
> +	$rados = PVE::RADOS->new();
> +    }
> +
> +    my $output = $rados->mon_command({
> +	prefix => 'auth get-or-create',
> +	entity => 'client.crash',
> +	caps => [
> +	    mon => 'profile crash',
> +	    mgr => 'profile crash',
> +	],
> +	format => 'plain',
> +    });
> +
> +    if (! -d $pve_ceph_cfgdir) {
> +	File::Path::make_path($pve_ceph_cfgdir);
> +    }
> +
> +    if (-f $pve_ceph_crash_key_path) {
> +	my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
> +
> +	if ($contents ne $output) {
> +	    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> +	}
> +    } else {
> +	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> +    }
> +
> +    return $pve_ceph_crash_key_path;
> +}
> +
>  # get ceph-volume managed osds
>  sub ceph_volume_list {
>      my $result = {};
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key

[Fabian Grünbichler]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> This commit adds the `set_ceph_crash_conf` function, which dynamically
> adapts the host's Ceph configuration in order to allow the Ceph crash
> module's daemon to run without elevated privileges.
> 
> This adaptation is only performed if:
>  * Ceph is installed
>  * Ceph is configured ('/etc/pve/ceph.conf' exists)
>  * Connection to RADOS is successful
> 
> If the above conditions are met, the function will ensure that:
>  * Ceph possesses a key named 'client.crash'
>  * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>  * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>  * The 'client.crash' section has a key named 'keyring' which
>    references '/etc/pve/ceph/ceph.client.crash.keyring'
> 
> Furthermore, if a key named 'client.crash' already exists within the
> cluster, it shall be reused and not regenerated. Also, the
> configuration is not altered if the conditions above are already met.
> 
> This way the keyring file is available as read-only in
> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
> Because the `ceph` user has been made part of said `www-data` group
> [0], it may access the file without requiring any additional
> privileges.
> 
> Thus, the configuration for the Ceph crash daemon is safely adapted as
> expected by PVE tooling and also shared via pmxcfs across one's
> cluster.

I still don't think this is a good idea, even a simple perl -e '..'
invocation or two (or a small helper script that doesn't live in $PATH)
for doing the two steps we want (initialize key if missing, lock+modify
config if key was missing) would be better (although compared to the
"hidden" or regular command approach, it has the downside that somebody
might miss the calls here when refactoring),

among other things the code below
- doesn't lock /etc/pve/ceph.conf but modifies it
- implements yet another broken parser for ceph.conf (e.g., it doesn't
  handle the stuff you fix in the perl variant in this series!)
- duplicates constants from the perl code that risk running out of sync,
  like paths or the key profile
- still has issues that you fixed in the perl code between v1 and v2
  (restarting services)

 I haven't reviewed the bash code in detail for that reason!

another issue - IMHO this should be version-guarded, since any new setup
would already gain it when setting up a monitor, and we avoid access to
pmxcfs in the upgrade hot path which can cause problems (cluster
non-quorate, ..).

> 
> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * fix 'keyring' key being appended to 'client.crash' section even
>     if it already exists and configured correctly
> 
>  debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 6138ef6d..267a62ae 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
>      fi
>  }
>  
> +set_ceph_crash_conf() {
> +    PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
> +    PVE_CEPH_CONFDIR='/etc/pve/ceph'
> +    PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
> +    PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
> +
> +    # ceph isn't installed -> nothing to do
> +    if ! which ceph > /dev/null 2>&1; then
> +        return 0
> +    fi
> +
> +    # ceph isn't configured -> nothing to do
> +    if test ! -f "${PVE_CEPH_CONFFILE}"; then
> +        return 0
> +    fi
> +
> +    CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
> +
> +    # ceph is installed and possibly configured, but no connection to RADOS
> +    # -> assume no monitor was created, nothing to do
> +    if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
> +        return 0
> +    fi
> +
> +    SECTION_RE='^\[\S+\]$'
> +    CRASH_SECTION_RE='^\[client\.crash\]$'
> +
> +    if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
> +        DO_RESTART_UNIT=0
> +        CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
> +
> +        if test ! -d "${PVE_CEPH_CONFDIR}"; then
> +            mkdir -p "${PVE_CEPH_CONFDIR}"
> +        fi
> +
> +        # keyring file doesn't exist or contains wrong key
> +        if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
> +            echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
> +            echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
> +            DO_RESTART_UNIT=1
> +        fi
> +
> +        # 'client.crash' section is in conf file
> +        if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
> +            IFS=''
> +            NEW_PVE_CEPH_CONFFILE=''
> +            IN_CRASH_SECTION=0
> +            HAS_KEYRING=0
> +            REPLACED_KEYRING=0
> +
> +            # look for 'keyring' key in 'client.crash' section
> +            # -> replace it if it points to the wrong location
> +            while read -r LINE; do
> +                if test "${IN_CRASH_SECTION}" = "1"; then
> +                    if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
> +                        IN_CRASH_SECTION=0
> +                    elif echo "${LINE}" | grep -q -E '\s+keyring'; then
> +                        HAS_KEYRING=1
> +
> +                        if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
> +                            echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
> +                            LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
> +                            REPLACED_KEYRING=1
> +                        fi
> +                    fi
> +                elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
> +                    IN_CRASH_SECTION=1
> +                fi
> +
> +                NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
> +            done < "${PVE_CEPH_CONFFILE}"
> +
> +            unset IFS
> +
> +            if test "${HAS_KEYRING}" = "1"; then
> +                # 'keyring' key was replaced -> write to file
> +                if test "${REPLACED_KEYRING}" = "1"; then
> +                    echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
> +                    DO_RESTART_UNIT=1
> +                fi
> +
> +            # client.crash section exists, but contained no 'keyring' key
> +            # -> put 'keyring' key into 'client.crash' section
> +            else
> +                sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
> +                    "${PVE_CEPH_CONFFILE}"
> +                DO_RESTART_UNIT=1
> +            fi
> +
> +        # 'client.crash' section doesn't exist -> add it
> +        else
> +            echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
> +            printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
> +                >> "${PVE_CEPH_CONFFILE}"
> +            DO_RESTART_UNIT=1
> +        fi
> +
> +        if test "${DO_RESTART_UNIT}" = "1"; then
> +            UNIT='ceph-crash.service'
> +
> +            if systemctl -q is-enabled "${UNIT}"; then
> +                echo "Restarting ceph-crash.service"
> +                deb-systemd-invoke restart "${UNIT}"
> +            fi
> +        fi
> +
> +    else
> +        echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
> +        printf '%s\n\n' "${CEPH_AUTH_RES}"
> +    fi
> +}
> +
>  case "$1" in
>    triggered)
>      # We don't print a status message here, as dpkg already said
> @@ -189,6 +301,7 @@ case "$1" in
>      fi
>  
>      set_lvm_conf
> +    set_ceph_crash_conf
>  
>      if test ! -e /proxmox_install_mode; then
>          # modeled after code generated by dh_start
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook

[Max Carrara]

On 2/12/24 14:32, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
>> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
>>
>> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>>   * use `find` instead of for-loop
>>
>>  ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
>>  patches/series                                |  1 +
>>  2 files changed, 51 insertions(+)
>>  create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>>
>> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> new file mode 100644
>> index 000000000..7445f3945
>> --- /dev/null
>> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> @@ -0,0 +1,50 @@
>> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
>> +From: Max Carrara <m.carrara@proxmox.com>
>> +Date: Thu, 1 Feb 2024 18:43:36 +0100
>> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
>> +
>> +A rather recent PR made ceph-crash run as "ceph" user instead of
>> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
>> +ceph-crash cannot actually post any crash logs now.
>> +
>> +This commit fixes this by also updating the permissions of
>> +/var/lib/ceph/*/* - the subdirectories and files of the directories in
>> +/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
>> +
>> +[0]: https://github.com/ceph/ceph/pull/48713
>> +
>> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> +---
>> + debian/ceph-base.postinst | 16 +++++++++-------
>> + 1 file changed, 9 insertions(+), 7 deletions(-)
>> +
>> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
>> +index 75eeb59c624..70d07977f82 100644
>> +--- a/debian/ceph-base.postinst
>> ++++ b/debian/ceph-base.postinst
>> +@@ -33,13 +33,15 @@ case "$1" in
>> + 	rm -f /etc/init/ceph.conf
>> + 	[ -x /sbin/start ] && start ceph-all || :
>> + 
>> +-        # adjust file and directory permissions
>> +-	for DIR in /var/lib/ceph/* ; do
>> +-	    if ! dpkg-statoverride --list $DIR >/dev/null
>> +-	    then
>> +-		chown $SERVER_USER:$SERVER_GROUP $DIR
>> +-	    fi
>> +-	done
>> ++	PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
> 
> this doesn't quote {} properly, files with spaces or other things in
> them can cause issues including shell command injection as root when
> this is executed by dpkg..

Noted, will be corrected in v3!

> 
>> ++
>> ++	# adjust directory permissions
>> ++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
>> ++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> ++
>> ++	# adjust file permissions
>> ++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
>> ++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> +     ;;
> 
> do we need the depth stuff or the split by type? we want to make
> everything under /var/lib/ceph owned by ceph:ceph, unless the admin has
> specifically overriden a particular path.

I added `-mindepth 1` because the '/var/lib/ceph' dir would be included in
`find`'s results otherwise, which isn't the case in the original code.
`-maxdepth 2` is to limit it to the depth that I had originally intended in
v1 - files and subdirectories, as well as sub-subdirectories and files in
those.

I agree that everything (in those two levels?) could just be `chown`ed
instead though - would make the above a little less verbose and only
need one invocation.

> 
> a simple 
> 
> find /var/lib/ceph -print0 | ...
> 
> should work and be much simpler (or if we want to limit to dirs and
> files, that can also be simply done in one go by ORing the two checks)

ORing the two checks didn't work in my case - turns out I just held `find`
wrongly.

Will correct all the above in v3 - thanks for your feedback!

> 
>> +     abort-upgrade|abort-remove|abort-deconfigure)
>> + 	:
>> +-- 
>> +2.39.2
>> +
>> diff --git a/patches/series b/patches/series
>> index 865caf23d..cf8f1ea31 100644
>> --- a/patches/series
>> +++ b/patches/series
>> @@ -12,3 +12,4 @@
>>  0012-backport-mgr-dashboard-simplify-authentication-proto.patch
>>  0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>>  0014-rocksb-inherit-parent-cmake-cxx-flags.patch
>> +0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
> 
> 

Re: [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser

[Max Carrara]

On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>>  1. Comments, irrespective of whether they start with '#' or ';' are
>>     now treated the same. Otherwise, sections and key-value pairs with
>>     a trailing comment starting with ';' are still parsed. Consider
>>     this example:
>>
>>       [some.section] # inline comment after section
>>       foo = bar ; inline comment after value
>>
>>      The '[some.section]' section in the example above would otherwise
>>      not be parsed at all, while in the key-value definition 'foo'
>>      parses as the key, which is correct, but 'bar ; inline comment
>>      after value' parses as value, which is incorrect according to
>>      Ceph's grammar [0][1].
>>
>>  2. Sections may now contain any character, including whitespace, but
>>     not '\n' or a comment literal '#' or ';'. The case for comment
>>     literals is handled in 1. above.
> 
> these seem sensible - what about line continuations? ;)

Yeah, those get swallowed. Will be corrected in v3, thanks for pointing
this out!

> 
>>
>>  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
>>     are treated the same, like in Ceph's parser [2].
> 
> the ceph docs state something else - which is wrong? ;)
> 
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/ says:
> 
>> Each of the Ceph configuration options has a unique name that consists
>> of words formed with lowercase characters and connected with
>> underscore characters (_).
> 
> this would seem to agree
> 
>> When option names are specified on the command line, underscore (_)
>> and dash (-) characters can be used interchangeably (for example,
>> --mon-host is equivalent to --mon_host).
> 
> okay, this is CLI which might just have its own mapping
> 
>> When option names appear in configuration files, spaces can also be
>> used in place of underscores or dashes. However, for the sake of
>> clarity and convenience, we suggest that you consistently use
>> underscores, as we do throughout this documentation.
> 
> but this now says that dash in config files is OK?

The docs and the code don't seem to agree in that regard; at least the
INI parser in C++ does *not* treat `-` the same [0]:

> /* Normalize a key name.
>  *
>  * Normalized key names have no leading or trailing whitespace, and all
>  * whitespace is stored as underscores.  The main reason for selecting this
>  * normal form is so that in common/config.cc, we can use a macro to stringify
>  * the field names of md_config_t and get a key in normal form.
>  */

(The code also actually does what it says on the tin. ;) )

So, I think it would be sensible to stick to how the Code does it, in this
case. Or was the original treatment of dashes in our code included in order
to guard against user errors, perhaps?

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294

> 
>> [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
>> [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
>> [2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>>   * new
>>
>>  src/PVE/CephConfig.pm | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..77b745f 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
>>  		  \&parse_ceph_config,
>>  		  \&write_ceph_config);
>>  
>> +# For more details on how Ceph's config parser works, see:
>> +# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
>>  sub parse_ceph_config {
>>      my ($filename, $raw) = @_;
>>  
>> @@ -20,14 +22,13 @@ sub parse_ceph_config {
>>  
>>      my $section;
>>  
>> -    foreach my $line (@lines) {
>> -	$line =~ s/#.*$//;
>> +    for my $line (@lines) {
>> +	$line =~ s/(#|;).*$//;
>>  	$line =~ s/^\s+//;
>> -	$line =~ s/^;.*$//;
>>  	$line =~ s/\s+$//;
>>  	next if !$line;
>>  
>> -	$section = $1 if $line =~ m/^\[(\S+)\]$/;
>> +	$section = $1 if $line =~ m/^\[(.+)\]$/;
>>  	if (!$section) {
>>  	    warn "no section - skip: $line\n";
>>  	    next;
>> @@ -35,11 +36,10 @@ sub parse_ceph_config {
>>  
>>  	if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
>>  	    my ($key, $val) = ($1, $2);
>> -	    # ceph treats ' ', '_' and '-' in keys the same, so lets do too
>> -	    $key =~ s/[-\ ]/_/g;
>> +	    # ceph treats ' ' and '_' in keys the same, so lets do too
>> +	    $key =~ s/ /_/g;
>>  	    $cfg->{$section}->{$key} = $val;
>>  	}
>> -
>>      }
>>  
>>      return $cfg;
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
> 
> 

Re: [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections

[Max Carrara]

On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> 
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> 
> would have been easier to parse if the style cleanup and actual
> behaviour change would have been split..

Fair point, acknowledged!

> 
> style change:
> 
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..34c3107 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -65,10 +65,10 @@ sub write_ceph_config {
>>      my $cond_write_sec = sub {
>>  	my $re = shift;
>>  
>> -	foreach my $section (sort keys %$cfg) {
>> +	for my $section (sort keys %$cfg) {
>>  	    next if $section !~ m/^$re$/;
>>  	    $out .= "[$section]\n";
>> -	    foreach my $key (sort keys %{$cfg->{$section}}) {
>> +	    for my $key (sort keys %{$cfg->{$section}}) {
>>  		$out .= "\t $key = $cfg->{$section}->{$key}\n";
>>  	    }
>>  	    $out .= "\n";
> 
> actual change (small nits inline!):
> 
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 34c3107..24bc78c 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
>>  sub write_ceph_config {
>>      my ($filename, $cfg) = @_;
>>  
>> +    my $written_sections = {};
>>      my $out = '';
>>  
>>      my $cond_write_sec = sub {
>>  	my $re = shift;
>>  
>>  	for my $section (sort keys %$cfg) {
>> -	    next if $section !~ m/^$re$/;
>> +	    if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
>> +		next;
>> +	    }
> 
> these two could be more clearly written as
> 
> next if $written_sections->{section};
> next if $section !~ m/$re$/;
> 
> since the two checks are not related in any way, other than both having
> the same effect if true (skipping that section). the second line is then
> unchanged, and both the diff and the resulting code is easier to parse
> IMHO.
> 
>> +
>>  	    $out .= "[$section]\n";
>>  	    for my $key (sort keys %{$cfg->{$section}}) {
>> -		$out .= "\t $key = $cfg->{$section}->{$key}\n";
>> +		$out .= "\t$key = $cfg->{$section}->{$key}\n";
> 
> this part here is not mentioned at all, and I might have missed it if I
> hadn't split the diffs ;)

Mea culpa!

Will clean this up in v3 and split the changes accordingly.

> 
>>  	    }
>>  	    $out .= "\n";
>> +
>> +	    $written_sections->{$section} = 1;
>>  	}
>>      };
>>  
>>      &$cond_write_sec('global');
>>      &$cond_write_sec('client');
>> +    &$cond_write_sec('client\..*');
>>  
>>      &$cond_write_sec('mds');
>>      &$cond_write_sec('mon');
>> @@ -88,6 +95,8 @@ sub write_ceph_config {
>>      &$cond_write_sec('osd\..*');
>>      &$cond_write_sec('mgr\..*');
>>  
>> +    &$cond_write_sec('.*');
>> +
>>      return $out;
>>  }
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections

[Max Carrara]

On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> cephconfig: allow writing arbitrary sections
>>
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
>> written directly after their associated section.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> NOTE: This amend really just changes the order of the sections below and
>>       may be dropped if not desired.
> 
> does the order matter at all? the docs would imply that it doesn't,
> since there is a clear precedence between [global], [$type], and
> [$type.$id] (e.g., global -> client -> client.crash)..

No, it doesn't - I perhaps should've worded this a little more clearly;
this change is purely cosmetic and just makes it so each '[$type]' is followed
by its respective '[$type.$id]'.

> 
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#configuration-sections
> 
>>
>> Changes v1 --> v2:
>>   * new
>>
>>  src/PVE/CephConfig.pm | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 86d3079..00b1f35 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -82,17 +82,20 @@ sub write_ceph_config {
>>      };
>>  
>>      &$cond_write_sec('global');
>> +
>>      &$cond_write_sec('client');
>>      &$cond_write_sec('client\..*');
>>  
>>      &$cond_write_sec('mds');
>> -    &$cond_write_sec('mon');
>> -    &$cond_write_sec('osd');
>> -    &$cond_write_sec('mgr');
>> -
>>      &$cond_write_sec('mds\..*');
>> +
>> +    &$cond_write_sec('mon');
>>      &$cond_write_sec('mon\..*');
>> +
>> +    &$cond_write_sec('osd');
>>      &$cond_write_sec('osd\..*');
>> +
>> +    &$cond_write_sec('mgr');
>>      &$cond_write_sec('mgr\..*');
>>  
>>      &$cond_write_sec('.*');
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
> 
> 

Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service

[Max Carrara]

On 2/12/24 14:34, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> when creating the cluster's first monitor.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>>   * do not enable/restart `ceph-crash` anymore when creating first mon
>>   * drop changes to function `ceph_service_cmd` as they are no longer
>>     needed
>>   * create keyring for `ceph-crash` before modifying 'ceph.conf'
>>   * always set keyring for 'client.crash' section instead of only
>>     if section doesn't exist already
>>   * only modify the keyring file in `get_or_create_crash_keyring()`
>>     if the content differs from the output of `ceph auth get-or-create`
> 
> you kinda ignored my comment about this adding yet another create keyring helper ;)

Saw your other reply - will disregard this as noted ;)

> 
>>  PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
>>  PVE/Ceph/Tools.pm    | 39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>> index 1e959ef3..ae12a2d3 100644
>> --- a/PVE/API2/Ceph/MON.pm
>> +++ b/PVE/API2/Ceph/MON.pm
>> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>>  	    });
>>  	    die $@ if $@;
>>  	    # automatically create manager after the first monitor is created
>> +	    # and set up keyring and config for ceph-crash.service
>>  	    if ($is_first_monitor) {
>>  		PVE::API2::Ceph::MGR->createmgr({
>>  		    node => $param->{node},
>>  		    id => $param->{node}
>> -		})
>> +		});
>> +
>> +		eval {
>> +		    PVE::Ceph::Tools::get_or_create_crash_keyring();
> 
> this is called get_or_create, but it actually returns the path to, not
> the key(ring).. nothing uses the return value anyway, so it could also
> be called differently I guess and not return anything, but just from the
> name, I'd expect the key to be returned.

I agree; I initially wanted this helper to be similar to `get_or_create_ceph_admin_keyring`,
but in this case I'll just unify the helper(s) once and for all in v3.

No need to beat around the bush if I'm gonna unify them anyway, so might
as well do it sooner than later ;)

> 
>> +		};
>> +		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
>> +
>> +		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
>> +		    my $cfg = cfs_read_file('ceph.conf');
>> +
>> +		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
> 
> I guess this doesn't make anything worse (since we starting from
> "ceph-crash is totally broken"), but if querying or creating the keyring
> failed, does it make sense to reference it in the config?

I guess in this case it doesn't, but we would need some kind of way to
re-init this part of the configuration in case the 'keyring' key doesn't
get set here.

Originally I planned to do this in a separate series to keep the scope of
this series focused on bug #4759, so perhaps it's best (at least for the
time being) to just not set the 'keyring' at all at the moment if querying
or creating the crash key fails.

So, I would change it to that in v3 and then later on supply a separate
series for the 're-init' part, if that's alright.

> 
>> +
>> +		    cfs_write_file('ceph.conf', $cfg);
>> +		});
>> +		die $@ if $@;
> 
> we could move this whole part up to where we do the monitor changes, and
> only lock and read and write the config once..

Good point, will be done in v3.

Thanks again for the thorough reviews! :)

> 
>>  	    }
>>  	};
>>  
>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>> index 273a3eb6..02a932e3 100644
>> --- a/PVE/Ceph/Tools.pm
>> +++ b/PVE/Ceph/Tools.pm
>> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
>>  my $ceph_cfgdir = "/etc/ceph";
>>  my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
>>  my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
>> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>>  
>> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
>>  my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>>  my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>>  my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
>> @@ -37,12 +39,14 @@ my $ceph_service = {
>>  
>>  my $config_values = {
>>      ccname => $ccname,
>> +    pve_ceph_cfgdir => $pve_ceph_cfgdir,
>>      ceph_mds_data_dir => $ceph_mds_data_dir,
>>      long_rados_timeout => 60,
>>  };
>>  
>>  my $config_files = {
>>      pve_ceph_cfgpath => $pve_ceph_cfgpath,
>> +    pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
>>      pve_mon_key_path => $pve_mon_key_path,
>>      pve_ckeyring_path => $pve_ckeyring_path,
>>      ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
>> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>>      return $pve_ckeyring_path;
>>  }
>>  
>> +# requires connection to existing monitor
>> +sub get_or_create_crash_keyring {
>> +    my ($rados) = @_;
>> +
>> +    if (!defined($rados)) {
>> +	$rados = PVE::RADOS->new();
>> +    }
>> +
>> +    my $output = $rados->mon_command({
>> +	prefix => 'auth get-or-create',
>> +	entity => 'client.crash',
>> +	caps => [
>> +	    mon => 'profile crash',
>> +	    mgr => 'profile crash',
>> +	],
>> +	format => 'plain',
>> +    });
>> +
>> +    if (! -d $pve_ceph_cfgdir) {
>> +	File::Path::make_path($pve_ceph_cfgdir);
>> +    }
>> +
>> +    if (-f $pve_ceph_crash_key_path) {
>> +	my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
>> +
>> +	if ($contents ne $output) {
>> +	    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>> +	}
>> +    } else {
>> +	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>> +    }
>> +
>> +    return $pve_ceph_crash_key_path;
>> +}
>> +
>>  # get ceph-volume managed osds
>>  sub ceph_volume_list {
>>      my $result = {};
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
> 
> 

Re: [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key

[Max Carrara]

On 2/12/24 14:34, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> This commit adds the `set_ceph_crash_conf` function, which dynamically
>> adapts the host's Ceph configuration in order to allow the Ceph crash
>> module's daemon to run without elevated privileges.
>>
>> This adaptation is only performed if:
>>  * Ceph is installed
>>  * Ceph is configured ('/etc/pve/ceph.conf' exists)
>>  * Connection to RADOS is successful
>>
>> If the above conditions are met, the function will ensure that:
>>  * Ceph possesses a key named 'client.crash'
>>  * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>>  * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>>  * The 'client.crash' section has a key named 'keyring' which
>>    references '/etc/pve/ceph/ceph.client.crash.keyring'
>>
>> Furthermore, if a key named 'client.crash' already exists within the
>> cluster, it shall be reused and not regenerated. Also, the
>> configuration is not altered if the conditions above are already met.
>>
>> This way the keyring file is available as read-only in
>> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
>> Because the `ceph` user has been made part of said `www-data` group
>> [0], it may access the file without requiring any additional
>> privileges.
>>
>> Thus, the configuration for the Ceph crash daemon is safely adapted as
>> expected by PVE tooling and also shared via pmxcfs across one's
>> cluster.
> 
> I still don't think this is a good idea, even a simple perl -e '..'
> invocation or two (or a small helper script that doesn't live in $PATH)
> for doing the two steps we want (initialize key if missing, lock+modify
> config if key was missing) would be better (although compared to the
> "hidden" or regular command approach, it has the downside that somebody
> might miss the calls here when refactoring),
> 
> among other things the code below
> - doesn't lock /etc/pve/ceph.conf but modifies it
> - implements yet another broken parser for ceph.conf (e.g., it doesn't
>   handle the stuff you fix in the perl variant in this series!)
> - duplicates constants from the perl code that risk running out of sync,
>   like paths or the key profile
> - still has issues that you fixed in the perl code between v1 and v2
>   (restarting services)
> 
>  I haven't reviewed the bash code in detail for that reason!
> 
> another issue - IMHO this should be version-guarded, since any new setup
> would already gain it when setting up a monitor, and we avoid access to
> pmxcfs in the upgrade hot path which can cause problems (cluster
> non-quorate, ..).

I hadn't considered the points you mentioned above - I agree with all of them,
actually. I'll see if I can rewrite all this in Perl (would probably be easier
than BASH anyway).

As discussed off-list, instead of writing an inline Perl script, a separate
helper script (as an executable) would probably be better - a sensible place
for this script would be in '/usr/share/pve-manager/helpers'. That way we
can also point users to this script if something goes wrong during the update.

> 
>>
>> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>>   * fix 'keyring' key being appended to 'client.crash' section even
>>     if it already exists and configured correctly
>>
>>  debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 113 insertions(+)
>>
>> diff --git a/debian/postinst b/debian/postinst
>> index 6138ef6d..267a62ae 100755
>> --- a/debian/postinst
>> +++ b/debian/postinst
>> @@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
>>      fi
>>  }
>>  
>> +set_ceph_crash_conf() {
>> +    PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
>> +    PVE_CEPH_CONFDIR='/etc/pve/ceph'
>> +    PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
>> +    PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
>> +
>> +    # ceph isn't installed -> nothing to do
>> +    if ! which ceph > /dev/null 2>&1; then
>> +        return 0
>> +    fi
>> +
>> +    # ceph isn't configured -> nothing to do
>> +    if test ! -f "${PVE_CEPH_CONFFILE}"; then
>> +        return 0
>> +    fi
>> +
>> +    CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
>> +
>> +    # ceph is installed and possibly configured, but no connection to RADOS
>> +    # -> assume no monitor was created, nothing to do
>> +    if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
>> +        return 0
>> +    fi
>> +
>> +    SECTION_RE='^\[\S+\]$'
>> +    CRASH_SECTION_RE='^\[client\.crash\]$'
>> +
>> +    if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
>> +        DO_RESTART_UNIT=0
>> +        CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
>> +
>> +        if test ! -d "${PVE_CEPH_CONFDIR}"; then
>> +            mkdir -p "${PVE_CEPH_CONFDIR}"
>> +        fi
>> +
>> +        # keyring file doesn't exist or contains wrong key
>> +        if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
>> +            echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
>> +            echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
>> +            DO_RESTART_UNIT=1
>> +        fi
>> +
>> +        # 'client.crash' section is in conf file
>> +        if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
>> +            IFS=''
>> +            NEW_PVE_CEPH_CONFFILE=''
>> +            IN_CRASH_SECTION=0
>> +            HAS_KEYRING=0
>> +            REPLACED_KEYRING=0
>> +
>> +            # look for 'keyring' key in 'client.crash' section
>> +            # -> replace it if it points to the wrong location
>> +            while read -r LINE; do
>> +                if test "${IN_CRASH_SECTION}" = "1"; then
>> +                    if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
>> +                        IN_CRASH_SECTION=0
>> +                    elif echo "${LINE}" | grep -q -E '\s+keyring'; then
>> +                        HAS_KEYRING=1
>> +
>> +                        if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
>> +                            echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
>> +                            LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
>> +                            REPLACED_KEYRING=1
>> +                        fi
>> +                    fi
>> +                elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
>> +                    IN_CRASH_SECTION=1
>> +                fi
>> +
>> +                NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
>> +            done < "${PVE_CEPH_CONFFILE}"
>> +
>> +            unset IFS
>> +
>> +            if test "${HAS_KEYRING}" = "1"; then
>> +                # 'keyring' key was replaced -> write to file
>> +                if test "${REPLACED_KEYRING}" = "1"; then
>> +                    echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
>> +                    DO_RESTART_UNIT=1
>> +                fi
>> +
>> +            # client.crash section exists, but contained no 'keyring' key
>> +            # -> put 'keyring' key into 'client.crash' section
>> +            else
>> +                sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
>> +                    "${PVE_CEPH_CONFFILE}"
>> +                DO_RESTART_UNIT=1
>> +            fi
>> +
>> +        # 'client.crash' section doesn't exist -> add it
>> +        else
>> +            echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
>> +            printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
>> +                >> "${PVE_CEPH_CONFFILE}"
>> +            DO_RESTART_UNIT=1
>> +        fi
>> +
>> +        if test "${DO_RESTART_UNIT}" = "1"; then
>> +            UNIT='ceph-crash.service'
>> +
>> +            if systemctl -q is-enabled "${UNIT}"; then
>> +                echo "Restarting ceph-crash.service"
>> +                deb-systemd-invoke restart "${UNIT}"
>> +            fi
>> +        fi
>> +
>> +    else
>> +        echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
>> +        printf '%s\n\n' "${CEPH_AUTH_RES}"
>> +    fi
>> +}
>> +
>>  case "$1" in
>>    triggered)
>>      # We don't print a status message here, as dpkg already said
>> @@ -189,6 +301,7 @@ case "$1" in
>>      fi
>>  
>>      set_lvm_conf
>> +    set_ceph_crash_conf
>>  
>>      if test ! -e /proxmox_install_mode; then
>>          # modeled after code generated by dh_start
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
> 
> 

Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service

[Max Carrara]

On 2/13/24 10:09, Max Carrara wrote:
> On 2/12/24 14:34, Fabian Grünbichler wrote:
>> On February 5, 2024 6:54 pm, Max Carrara wrote:
>>> when creating the cluster's first monitor.
>>>
>>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>>> ---
>>> Changes v1 --> v2:
>>>   * do not enable/restart `ceph-crash` anymore when creating first mon
>>>   * drop changes to function `ceph_service_cmd` as they are no longer
>>>     needed
>>>   * create keyring for `ceph-crash` before modifying 'ceph.conf'
>>>   * always set keyring for 'client.crash' section instead of only
>>>     if section doesn't exist already
>>>   * only modify the keyring file in `get_or_create_crash_keyring()`
>>>     if the content differs from the output of `ceph auth get-or-create`
>>
>> you kinda ignored my comment about this adding yet another create keyring helper ;)
> 
> Saw your other reply - will disregard this as noted ;)
> 
>>
>>>  PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
>>>  PVE/Ceph/Tools.pm    | 39 +++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>>> index 1e959ef3..ae12a2d3 100644
>>> --- a/PVE/API2/Ceph/MON.pm
>>> +++ b/PVE/API2/Ceph/MON.pm
>>> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>>>  	    });
>>>  	    die $@ if $@;
>>>  	    # automatically create manager after the first monitor is created
>>> +	    # and set up keyring and config for ceph-crash.service
>>>  	    if ($is_first_monitor) {
>>>  		PVE::API2::Ceph::MGR->createmgr({
>>>  		    node => $param->{node},
>>>  		    id => $param->{node}
>>> -		})
>>> +		});
>>> +
>>> +		eval {
>>> +		    PVE::Ceph::Tools::get_or_create_crash_keyring();
>>
>> this is called get_or_create, but it actually returns the path to, not
>> the key(ring).. nothing uses the return value anyway, so it could also
>> be called differently I guess and not return anything, but just from the
>> name, I'd expect the key to be returned.
> 
> I agree; I initially wanted this helper to be similar to `get_or_create_ceph_admin_keyring`,
> but in this case I'll just unify the helper(s) once and for all in v3.
> 
> No need to beat around the bush if I'm gonna unify them anyway, so might
> as well do it sooner than later ;)

Now that I've taken a more detailed look at all the existing code, in particular
usages of `ceph auth get-or-create` (via RADOS) and `ceph-authtool` (as command),
it's become clear to me that there's no actual benefit in creating an auth-helper
in that regard *at all*.

`ceph auth get-or-create` is only used via RADOS (`$rados->mon_command(...)`)
and `ceph-authtool` is only invoked in three places, each with separate use cases.

So, creating the keyring for 'client.crash' is (yet) an(other) exception rather
than something that benefits from a helper, so in this case, I'll think of a
different solution.

> 
>>
>>> +		};
>>> +		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
>>> +
>>> +		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
>>> +		    my $cfg = cfs_read_file('ceph.conf');
>>> +
>>> +		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
>>
>> I guess this doesn't make anything worse (since we starting from
>> "ceph-crash is totally broken"), but if querying or creating the keyring
>> failed, does it make sense to reference it in the config?
> 
> I guess in this case it doesn't, but we would need some kind of way to
> re-init this part of the configuration in case the 'keyring' key doesn't
> get set here.
> 
> Originally I planned to do this in a separate series to keep the scope of
> this series focused on bug #4759, so perhaps it's best (at least for the
> time being) to just not set the 'keyring' at all at the moment if querying
> or creating the crash key fails.
> 
> So, I would change it to that in v3 and then later on supply a separate
> series for the 're-init' part, if that's alright.
> 
>>
>>> +
>>> +		    cfs_write_file('ceph.conf', $cfg);
>>> +		});
>>> +		die $@ if $@;
>>
>> we could move this whole part up to where we do the monitor changes, and
>> only lock and read and write the config once..
> 
> Good point, will be done in v3.
> 
> Thanks again for the thorough reviews! :)
> 
>>
>>>  	    }
>>>  	};
>>>  
>>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>>> index 273a3eb6..02a932e3 100644
>>> --- a/PVE/Ceph/Tools.pm
>>> +++ b/PVE/Ceph/Tools.pm
>>> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
>>>  my $ceph_cfgdir = "/etc/ceph";
>>>  my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
>>>  my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
>>> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>>>  
>>> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
>>>  my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>>>  my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>>>  my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
>>> @@ -37,12 +39,14 @@ my $ceph_service = {
>>>  
>>>  my $config_values = {
>>>      ccname => $ccname,
>>> +    pve_ceph_cfgdir => $pve_ceph_cfgdir,
>>>      ceph_mds_data_dir => $ceph_mds_data_dir,
>>>      long_rados_timeout => 60,
>>>  };
>>>  
>>>  my $config_files = {
>>>      pve_ceph_cfgpath => $pve_ceph_cfgpath,
>>> +    pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
>>>      pve_mon_key_path => $pve_mon_key_path,
>>>      pve_ckeyring_path => $pve_ckeyring_path,
>>>      ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
>>> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>>>      return $pve_ckeyring_path;
>>>  }
>>>  
>>> +# requires connection to existing monitor
>>> +sub get_or_create_crash_keyring {
>>> +    my ($rados) = @_;
>>> +
>>> +    if (!defined($rados)) {
>>> +	$rados = PVE::RADOS->new();
>>> +    }
>>> +
>>> +    my $output = $rados->mon_command({
>>> +	prefix => 'auth get-or-create',
>>> +	entity => 'client.crash',
>>> +	caps => [
>>> +	    mon => 'profile crash',
>>> +	    mgr => 'profile crash',
>>> +	],
>>> +	format => 'plain',
>>> +    });
>>> +
>>> +    if (! -d $pve_ceph_cfgdir) {
>>> +	File::Path::make_path($pve_ceph_cfgdir);
>>> +    }
>>> +
>>> +    if (-f $pve_ceph_crash_key_path) {
>>> +	my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
>>> +
>>> +	if ($contents ne $output) {
>>> +	    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>>> +	}
>>> +    } else {
>>> +	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>>> +    }
>>> +
>>> +    return $pve_ceph_crash_key_path;
>>> +}
>>> +
>>>  # get ceph-volume managed osds
>>>  sub ceph_volume_list {
>>>      my $result = {};
>>> -- 
>>> 2.39.2
>>>
>>>
>>>
>>> _______________________________________________
>>> 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
>>
>>
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel