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

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

[Max Carrara]

Fix #4759: Configure Permissions for ceph-crash.service - Version 4
===================================================================

Notable changes since v3
------------------------

  * Both parser and writer for 'ceph.conf' now have unit tests which run
    during make targets like e.g. `make deb`, thanks to `dh_auto_test`
  * The parser for 'ceph.conf' now correctly un-escapes comment literals
    (found while developing unit tests)
  * The writer for 'ceph.conf' now correctly escapes comment literals
    (found while developing unit tests)
  * The helper script called in 'postinst' of pve-manager for updating
    'ceph.crash' in 'ceph.conf' now correctly handles an existing key
    being referenced directly and removes it (thanks Friedrich!)
  * The aforementioned helper script has more verbose output, showing
    explicitly what's being done to the configuration
  * The 'postinst' hook now prints an empty line before and after it
    runs to make it a little more visible
  * The 'postinst' hook now also restarts 'ceph-crash.service' if the
    user hasn't disabled it (thanks Friedrich!)

For a detailed list of changes, please see the comments in the
individual patches.


Older Versions
--------------

v1: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061546.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061646.html
v3: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061802.html

Summary of Changes
------------------

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

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
 patches/series                                |  2 +
 3 files changed, 86 insertions(+)
 create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
 create mode 100644 patches/0017-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

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
 patches/series                                |  2 +
 3 files changed, 86 insertions(+)
 create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
 create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch


pve-storage:

Max Carrara (9):
  cephconfig: align our parser more with Ceph's parser
  cephconfig: support line-continuations in parser
  cephconfig: allow writing arbitrary sections
  cephconfig: support escaped comment literals
  cephconfig: emit warning for lines that fail to parse
  cephconfig: change code style inside config writer
  cephconfig: change order of written sections
  cephconfig: remove leading whitespace on write to Ceph config
  test: add tests for 'ceph.conf' parser and writer

 src/Makefile                               |   1 +
 src/PVE/CephConfig.pm                      |  95 +++--
 src/PVE/Makefile                           |   4 +
 src/PVE/test/Makefile                      |   9 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 402 +++++++++++++++++++++
 5 files changed, 490 insertions(+), 21 deletions(-)
 create mode 100644 src/PVE/test/Makefile
 create mode 100755 src/PVE/test/ceph_conf_parse_write_test.pl


pve-manager:

Max Carrara (3):
  ceph: introduce '/etc/pve/ceph'
  fix #4759: ceph: configure ceph-crash.service and its key
  bin/make: gather helper scripts in separate variable

 PVE/API2/Ceph.pm        |   5 ++
 PVE/API2/Ceph/MON.pm    |   8 +++
 PVE/Ceph/Tools.pm       |  47 ++++++++++++++-
 bin/Makefile            |   6 +-
 bin/pve-init-ceph-crash | 129 ++++++++++++++++++++++++++++++++++++++++
 debian/postinst         |  26 ++++++++
 6 files changed, 218 insertions(+), 3 deletions(-)
 create mode 100755 bin/pve-init-ceph-crash

-- 
2.39.2

[pve-devel] [PATCH v4 master ceph 1/16] 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 the contents of
'/var/lib/ceph/crash'.

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

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * use `find` instead of for-loop
Changes v2 --> v3:
  * rebased on master
  * `chown` all kinds of entries, not just files and directories
    (as discussed off-list) 
  * instead of `chown`-ing '/var/lib/ceph/**/*', recusively call `chown`
    on the contents of `/var/lib/ceph/crash` (as discussed off-list)
Changes v3 --> v4:
  * none

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch

diff --git a/patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch b/patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
new file mode 100644
index 000000000..36f4df3aa
--- /dev/null
+++ b/patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
@@ -0,0 +1,54 @@
+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: recursively adjust permissions of /var/lib/ceph/crash
+
+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 recursively updating the permissions of
+'/var/lib/ceph/crash', which ensures that all files and directories
+used by 'ceph-crash.service' are actually owned by the user configured
+for Ceph.
+
+The previously existing loop has also been replaced by an invocation
+of `find | xargs`.
+
+[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..424c2c889d5 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 file and directory permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 1 -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++	# adjust permissions so ceph-crash.service can post reports
++	find /var/lib/ceph/crash -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 6ad754713..83a168ec9 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-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
+0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
-- 
2.39.2

[pve-devel] [PATCH v4 master ceph 2/16] 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>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * rebased on master
Changes v3 --> v4:
  * none

Note: I preseved Fabian's 'Reviewed-by' trailer as no changes have been
made to the patch this commit adds. I hope that's okay.

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

diff --git a/patches/0017-ceph-crash-change-order-of-client-names.patch b/patches/0017-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..8131fced5
--- /dev/null
+++ b/patches/0017-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 0e02837fadd..713080a4dc1 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 83a168ec9..9bde2a241 100644
--- a/patches/series
+++ b/patches/series
@@ -14,3 +14,4 @@
 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
 0015-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
 0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
+0017-ceph-crash-change-order-of-client-names.patch
-- 
2.39.2

[pve-devel] [PATCH v4 quincy-stable-8 ceph 3/16] 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 the contents of
'/var/lib/ceph/crash'.

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

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * use `find` instead of for-loop
Changes v2 --> v3:
  * rebased on quincy-stable-8
  * `chown` all kinds of entries, not just files and directories
    (as discussed off-list) 
  * instead of `chown`-ing '/var/lib/ceph/**/*', recusively call `chown`
    on the contents of `/var/lib/ceph/crash` (as discussed off-list)
Changes v3 --> v4:
  * none

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch

diff --git a/patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch b/patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
new file mode 100644
index 000000000..36f4df3aa
--- /dev/null
+++ b/patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
@@ -0,0 +1,54 @@
+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: recursively adjust permissions of /var/lib/ceph/crash
+
+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 recursively updating the permissions of
+'/var/lib/ceph/crash', which ensures that all files and directories
+used by 'ceph-crash.service' are actually owned by the user configured
+for Ceph.
+
+The previously existing loop has also been replaced by an invocation
+of `find | xargs`.
+
+[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..424c2c889d5 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 file and directory permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 1 -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++	# adjust permissions so ceph-crash.service can post reports
++	find /var/lib/ceph/crash -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 30fc83ec0..132bfa739 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-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
+0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
-- 
2.39.2

[pve-devel] [PATCH v4 quincy-stable-8 ceph 4/16] 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>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * rebased on quincy-stable-8
Changes v3 --> v4:
  * none

Note: I preseved Fabian's 'Reviewed-by' trailer as no changes have been
made to the patch this commit adds. I hope that's okay.

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

diff --git a/patches/0026-ceph-crash-change-order-of-client-names.patch b/patches/0026-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..8131fced5
--- /dev/null
+++ b/patches/0026-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 0e02837fadd..713080a4dc1 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 132bfa739..406d4624a 100644
--- a/patches/series
+++ b/patches/series
@@ -18,3 +18,4 @@
 0023-rocksb-inherit-parent-cmake-cxx-flags.patch
 0024-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
 0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
+0026-ceph-crash-change-order-of-client-names.patch
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 05/16] 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].

 4. Although not crucial for Ceph, our parser now also supports empty
    sections. When a section header is successfully parsed, it gets
    added to the configuration hash and the parser continues operating
    on the next line.

[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
Changes v2 --> v3:
  * support comment literals (4.)
Changes v3 --> v4:
  * support empty sections
  * fix and move support for comment literals to separate patch

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b10d46..74a92eb 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,18 @@ 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+)\]$/;
+	if ($line =~ m/^\[(.+)\]$/) {
+	    $section = $1;
+	    $cfg->{$section} = {} if !exists($cfg->{$section});
+	    next;
+	}
+
 	if (!$section) {
 	    warn "no section - skip: $line\n";
 	    next;
@@ -35,11 +41,12 @@ 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;
-	}
 
+	    next;
+	}
     }
 
     return $cfg;
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 06/16] cephconfig: support line-continuations in parser

[Max Carrara]

Ceph's docs state the following [0]:
> The backslash character `\` is used as the line-continuation marker
> that combines the next line with the current one.

This commit implements the support of such line-continuations in our
parser.

The line following a line ending with a '\' has its whitespace
preserved, which matches the behaviour in Ceph's original
implementation [1]. In other words, leading and trailing whitespace is
not stripped from a continued line.

[0]: https://docs.ceph.com/en/reef/rados/configuration/ceph-conf/#changes-introduced-in-octopus
[1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l262

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

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 74a92eb..80f71b0 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -19,13 +19,33 @@ sub parse_ceph_config {
     return $cfg if !defined($raw);
 
     my @lines = split /\n/, $raw;
+    my @lines_normalized;
+
+    my $re_comment_not_escaped = qr/(?<!\\)(#|;).*$/;
+    my $re_leading_ws = qr/^\s+/;
+    my $re_trailing_ws = qr/\s+$/;
+
+    while (scalar(@lines)) {
+	my $line = shift(@lines);
+	$line =~ s/$re_comment_not_escaped//;
+	$line =~ s/$re_leading_ws//;
+	$line =~ s/$re_trailing_ws//;
+	next if !$line;
+
+	# merge lines ending with continuation character '\'
+	while ($line =~ s/\\$//) {
+	    my $next_line = shift(@lines);
+	    $next_line =~ s/$re_comment_not_escaped//;
+	    $next_line =~ s/$re_trailing_ws//;
+	    $line .= $next_line;
+	}
+
+	push(@lines_normalized, $line);
+    }
 
     my $section;
 
-    for my $line (@lines) {
-	$line =~ s/(?<!\\)(#|;).*$//;
-	$line =~ s/^\s+//;
-	$line =~ s/\s+$//;
+    for my $line (@lines_normalized) {
 	next if !$line;
 
 	if ($line =~ m/^\[(.+)\]$/) {
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 07/16] 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
Changes v2 --> v3:
  * this patch now only contains the changes actually mentioned in the
    commit message; the other changes have been put into separate
    patches
Changes v3 --> v4:
  * none

 src/PVE/CephConfig.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 80f71b0..32ea544 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -87,6 +87,7 @@ my $parse_ceph_file = sub {
 sub write_ceph_config {
     my ($filename, $cfg) = @_;
 
+    my $written_sections = {};
     my $out = '';
 
     my $cond_write_sec = sub {
@@ -94,16 +95,21 @@ sub write_ceph_config {
 
 	foreach my $section (sort keys %$cfg) {
 	    next if $section !~ m/^$re$/;
+	    next if exists($written_sections->{$section});
+
 	    $out .= "[$section]\n";
 	    foreach 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');
@@ -115,6 +121,8 @@ sub write_ceph_config {
     &$cond_write_sec('osd\..*');
     &$cond_write_sec('mgr\..*');
 
+    &$cond_write_sec('.*');
+
     return $out;
 }
 
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 08/16] cephconfig: support escaped comment literals

[Max Carrara]

This commit allows our parser and writer implementations to support
escaping and un-escaping the comment literals '#' and ';' [0].

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

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

 src/PVE/CephConfig.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 32ea544..6b7bbd4 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -40,6 +40,9 @@ sub parse_ceph_config {
 	    $line .= $next_line;
 	}
 
+	# un-escape escaped comment literals
+	$line =~ s/\\(#|;)/$1/g;
+
 	push(@lines_normalized, $line);
     }
 
@@ -99,7 +102,11 @@ sub write_ceph_config {
 
 	    $out .= "[$section]\n";
 	    foreach my $key (sort keys %{$cfg->{$section}}) {
-		$out .= "\t $key = $cfg->{$section}->{$key}\n";
+		my $value = $cfg->{$section}->{$key};
+		# escape comment literals
+		$value =~ s/(#|;)/\\$1/g;
+
+		$out .= "\t $key = $value\n";
 	    }
 	    $out .= "\n";
 
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 09/13] cephconfig: emit warning for lines that fail to parse

[Max Carrara]

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

 src/PVE/CephConfig.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b7bbd4..da28940 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -70,6 +70,8 @@ sub parse_ceph_config {
 
 	    next;
 	}
+
+	warn "failed to parse line - skip: $line\n";
     }
 
     return $cfg;
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 10/16] cephconfig: change code style inside config writer

[Max Carrara]

This commit changes the code style of subroutine `write_ceph_config`
to match our style guide [0] more.

Furthermore, the repeated calls to the inner subroutine are replaced
with a loop, while the regular expressions used by the inner sub are
now quoted with `qr` to prevent any accidental mis-quotings in the
future.

[0]: https://pve.proxmox.com/wiki/Perl_Style_Guide

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new; initial style changes moved from patch 06 of series v2
  * `qr` operator for regexes to avoid character escaping issues
  * call `$cond_write_sec` in loop
Changes v3 --> v4:
  * use postfix deref ($hashref->%* instead of %$hashref)
  * rebased due to previous changes

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index da28940..a361a5b 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -98,12 +98,12 @@ 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$/;
 	    next if exists($written_sections->{$section});
 
 	    $out .= "[$section]\n";
-	    foreach my $key (sort keys %{$cfg->{$section}}) {
+	    for my $key (sort keys $cfg->{$section}->%*) {
 		my $value = $cfg->{$section}->{$key};
 		# escape comment literals
 		$value =~ s/(#|;)/\\$1/g;
@@ -116,21 +116,27 @@ sub write_ceph_config {
 	}
     };
 
-    &$cond_write_sec('global');
-    &$cond_write_sec('client');
-    &$cond_write_sec('client\..*');
+    my @rexprs = (
+	qr/global/,
+	qr/client/,
+	qr/client\..*/,
 
-    &$cond_write_sec('mds');
-    &$cond_write_sec('mon');
-    &$cond_write_sec('osd');
-    &$cond_write_sec('mgr');
+	qr/mds/,
+	qr/mon/,
+	qr/osd/,
+	qr/mgr/,
 
-    &$cond_write_sec('mds\..*');
-    &$cond_write_sec('mon\..*');
-    &$cond_write_sec('osd\..*');
-    &$cond_write_sec('mgr\..*');
+	qr/mds\..*/,
+	qr/mon\..*/,
+	qr/osd\..*/,
+	qr/mgr\..*/,
 
-    &$cond_write_sec('.*');
+	qr/.*/,
+    );
+
+    for my $re (@rexprs) {
+	$cond_write_sec->($re);
+    }
 
     return $out;
 }
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 11/16] cephconfig: change order of written sections

[Max Carrara]

in order to group related sections together.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new; originally patch 07 of series v2, now adapted to this series 
Changes v3 --> v4:
  * none

 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 a361a5b..c84b78f 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -118,17 +118,20 @@ sub write_ceph_config {
 
     my @rexprs = (
 	qr/global/,
+
 	qr/client/,
 	qr/client\..*/,
 
 	qr/mds/,
-	qr/mon/,
-	qr/osd/,
-	qr/mgr/,
-
 	qr/mds\..*/,
+
+	qr/mon/,
 	qr/mon\..*/,
+
+	qr/osd/,
 	qr/osd\..*/,
+
+	qr/mgr/,
 	qr/mgr\..*/,
 
 	qr/.*/,
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 12/16] cephconfig: remove leading whitespace on write to Ceph config

[Max Carrara]

Because continued lines (lines following lines that end with '\')
preserve whitespace, this commit removes any leading whitespace that
is added by our config writer.

This is done in order to make continued lines look less "awkward" when
manually changing values in 'ceph.conf' after the file has been
modified by our tooling. Before this commit, continued lines would
have to be added as follows:

[some_section]
	 some_key = Lorem ipsum dolor sit amet, consectetur \
adipiscing elit, sed do eiusmod tempor incididunt ut labore et \
dolore magna aliqua.

Due to whitespace being preserved, continued lines cannot be on the
same indentation level as `some_key`. Furthermore, the indentation
level might lead some users to mistakenly believe that leading
whitespace is ignored.

Thus, this commit removes the leading whitespace that is added by our
config writer.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new
Changes v3 --> v4:
  * rebased due to previous changes

Note: This patch may be dropped if not desired.

 src/PVE/CephConfig.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index c84b78f..888be08 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -108,7 +108,7 @@ sub write_ceph_config {
 		# escape comment literals
 		$value =~ s/(#|;)/\\$1/g;
 
-		$out .= "\t $key = $value\n";
+		$out .= "$key = $value\n";
 	    }
 	    $out .= "\n";
 
-- 
2.39.2

[pve-devel] [PATCH v4 pve-storage 13/16] test: add tests for 'ceph.conf' parser and writer

[Max Carrara]

These tests attempt to cover all syntax quirks that the format of
'ceph.conf' currently allows.

One known exception however is the handling of "quoted" and "unquoted"
strings, as Ceph's own parser appears to not actually differ between
them either. Curiously, if a "quoted string" isn't able to be
parsed by Ceph's implementation, it goes on to parse it as an
"unquoted string" anyway. [0] In both cases, the result is the same -
the string is parsed with quotes.

Each test case is first tested against the parser - if the resulting
hash matches the expected hash, it is consequently passed into the
writer. The writer's result is then parsed another time and compared
against the expected hash once more.

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

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

 src/Makefile                               |   1 +
 src/PVE/Makefile                           |   4 +
 src/PVE/test/Makefile                      |   9 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 402 +++++++++++++++++++++
 4 files changed, 416 insertions(+)
 create mode 100644 src/PVE/test/Makefile
 create mode 100755 src/PVE/test/ceph_conf_parse_write_test.pl

diff --git a/src/Makefile b/src/Makefile
index 1eba51e..a322f46 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -15,6 +15,7 @@ install: PVE bin udev-rbd
 test:
 	perl -I. -T -e "use PVE::CLI::pvesm; PVE::CLI::pvesm->verify_api();"
 	$(MAKE) -C test
+	$(MAKE) -C PVE test
 
 .PHONY: clean
 clean:
diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index 5fe4a0a..d438804 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -9,4 +9,8 @@ install:
 	make -C API2 install
 	make -C CLI install
 
+.PHONY: test
+test:
+	$(MAKE) -C test test
+
 clean:
diff --git a/src/PVE/test/Makefile b/src/PVE/test/Makefile
new file mode 100644
index 0000000..f9c6c79
--- /dev/null
+++ b/src/PVE/test/Makefile
@@ -0,0 +1,9 @@
+all:
+
+.PHONY: test
+test: test_ceph_conf_parse_write
+
+.PHONY: test_ceph_conf_parse_write
+test_ceph_conf_parse_write: ceph_conf_parse_write_test.pl
+	./ceph_conf_parse_write_test.pl
+
diff --git a/src/PVE/test/ceph_conf_parse_write_test.pl b/src/PVE/test/ceph_conf_parse_write_test.pl
new file mode 100755
index 0000000..f76a877
--- /dev/null
+++ b/src/PVE/test/ceph_conf_parse_write_test.pl
@@ -0,0 +1,402 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(../..);
+
+use Test::More;
+
+use PVE::CephConfig;
+
+
+# An array of test cases.
+# Each test case is comprised of the following keys:
+#   description => to identify a single test
+#   expected    => the hash that parse_ceph_config should return
+#   raw		=> the raw content of the file to test
+#
+# A case is tested as follows:
+#   1. The 'raw' key is fed into parse_ceph_config
+#   2. The result is compared with 'expected'
+#   3. If correct, the resulting hash is fed into write_ceph_config
+#   4. It's output is then fed to parse_ceph_config again for one last
+#      comparison with 'expected'
+my $tests = [
+    {
+	description => 'empty file',
+	expected => {},
+	raw => <<~EOF,
+	EOF
+    },
+    {
+	description => 'file without section',
+	expected => {},
+	raw => <<~EOF,
+	foo = bar
+	arbitrary text can go here
+
+	Rust is better than Perl
+	EOF
+    },
+    {
+	description => 'single section',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, no key-value pairs',
+	expected => {
+	    foo => {},
+	},
+	raw => <<~EOF,
+	[foo]
+	EOF
+    },
+    {
+	description => 'single section, whitespace before key',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	     \t     bar = baz
+	EOF
+    },
+    {
+	description => 'single section, section header with preceding whitespace',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	  \t    [foo]
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, section header with comment',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo] # some comment
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, section header ' .
+	    'with preceding whitespace and comment',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	  \t  [foo] ; some comment
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, arbitrary text before section',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	Rust is better than Perl
+
+	This text is ignored by our parser, because it makes things simpler
+	[foo]
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, invalid key-value pairs',
+	expected => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	this here will cause a warning and is ignored
+	bar = baz
+	as well as this
+	EOF
+    },
+    {
+	description => 'single section, multiple key-value pairs',
+	expected => {
+	    foo => {
+		one => 1,
+		two => 2,
+		three => 3,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	one = 1
+	two = 2
+
+	three = 3
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with whitespace',
+	expected => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	       \t  bar  \t   =\t \tbaz
+
+	quo\t=\tqux
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with comments',
+	expected => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	; preceding comment
+	bar = baz # some comment
+	### comment inbetween
+	;; another one for good measure
+	quo = qux ; another comment
+	# trailing comment
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with continued lines',
+	expected => {
+	    foo => {
+		bar => 'baz continued baz',
+		quo => "qux continued \tqux",
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz \\
+	continued baz
+
+	quo =\\
+	qux \\
+	continued \\
+	\tqux
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with ' .
+	    'continued lines and comments',
+	expected => {
+	    foo => {
+		bar => 'baz continued baz',
+		quo => 'qux continued qux',
+		key => 'value',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz \\
+	continued baz # comments are allowed here
+
+	quo =\\
+	qux \\
+	continued \\
+	qux # but this continuation will be ignored, because it's in a comment: \\
+	key = value\\
+	# really weird comment
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with ' .
+	    'escaped commment literals',
+	expected => {
+	    foo => {
+		bar => 'baz#escaped',
+		quo => 'qux;escaped',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz\\#escaped
+	quo = qux\\;escaped
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with ' .
+	    'continued lines and escaped commment literal',
+	expected => {
+	    foo => {
+		bar => 'baz#escaped',
+		quo => 'qux;escaped continued# escaped done',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz\\#escaped
+
+	quo = qux\\;escaped\\
+	 continued\\# escaped \\
+	done
+	EOF
+    },
+    {
+	description => 'multiple sections, multiple key-value pairs',
+	expected => {
+	    foo => {
+		one => 1,
+		two => 2,
+	    },
+	    bar => {
+		three => 3,
+		four => 4,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	one = 1
+	two = 2
+	[bar]
+	three = 3
+	four = 4
+	EOF
+    },
+    {
+	description => 'multiple sections, multiple key-value pairs, '
+	    . 'comments inline and inbetween, escaped comment literals, '
+	    . 'continued lines, arbitrary whitespace',
+	expected => {
+	    global => {
+		auth_client_required => 'cephx',
+		auth_cluster_required => 'cephx',
+		auth_service_required => 'cephx',
+		cluster_network => '172.16.65.0/24',
+		fsid => '0e2f72eb-ffff-ffff-ffff-f480790a5b07',
+		mon_allow_pool_delete => 'true',
+		mon_host => '172.16.65.12 172.16.65.13 172.16.65.11',
+		ms_bind_ipv4 => 'true',
+		osd_pool_default_min_size => '2',
+		osd_pool_default_size => '3',
+		public_network => '172.16.65.0/24',
+	    },
+	    client => {
+		keyring => '/etc/pve/priv/$cluster.$name.keyring',
+	    },
+	    'mon.ceph-01' => {
+		public_addr => '172.16.65.11',
+	    },
+	    'mon.ceph-02' => {
+		public_addr => '172.16.65.12',
+	    },
+	    'mon.ceph-03' => {
+		public_addr => '172.16.65.13',
+	    },
+	},
+	raw => <<~EOF,
+	[global]
+	auth_client_required = cephx
+		auth_cluster_required = \\
+	cephx
+	auth_service_required =           cephx
+			cluster_network =        172.16.65.0/24
+	fsid = 0e2f72eb-ffff-ffff-ffff-f480790a5b07
+	    mon_allow_pool_delete = true
+	mon_host = \\
+	172.16.65.12 \\
+	172.16.65.13 \\
+	172.16.65.11 # why is this one last? nobody knows for sure!
+
+	ms_bind_ipv4 = true
+	osd_pool_default_min_size =\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	2
+	osd_pool_default_size =\\
+					    3
+	public_network = 172.16.65.0/24 # some comment
+
+	[client] ### another comment
+	keyring = /etc/pve/priv/\$cluster.\$name.keyring# cheeky trailing comment
+
+	## mon config ##
+	[mon.ceph-01]
+	public_addr = 172.16.65.11 ; foo
+
+	;; another arbitrary comment ;;
+	[mon.ceph-02] ;; very important comment here
+	public_addr = 172.16.65.12 # bar
+
+	[mon.ceph-03]
+	public_addr = 172.16.65.13			# baz
+	EOF
+    },
+];
+
+plan(tests => scalar($tests->@*) * 2);
+
+for my $tt ($tests->@*) {
+
+    my $has_parse_err = 0;
+    my $parse_res = eval {
+	# suppress warnings here to make output less noisy for certain tests
+	local $SIG{__WARN__} = sub {};
+
+	PVE::CephConfig::parse_ceph_config(undef, $tt->{raw})
+    };
+
+    if ($@) {
+	$parse_res = $@;
+	$has_parse_err = 1;
+    }
+
+    my $parse_desc = "parse: $tt->{description}";
+    is_deeply($parse_res, $tt->{expected}, $parse_desc || diag(explain($parse_res)));
+
+
+    my $second_parse_res = eval {
+	local $SIG{__WARN__} = sub {};
+
+	die "cannot execute test due to previous error\n" if $has_parse_err;
+
+	my $write_res = PVE::CephConfig::write_ceph_config(undef, $parse_res);
+	PVE::CephConfig::parse_ceph_config(undef, $write_res)
+    };
+
+    $second_parse_res = $@ if $@;
+
+    my $write_desc = "write: $tt->{description}";
+    is_deeply($second_parse_res, $tt->{expected}, $write_desc || diag(explain($second_parse_res)));
+}
+
+done_testing();
+
+1;
-- 
2.39.2

[pve-devel] [PATCH v4 pve-manager 14/16] ceph: introduce '/etc/pve/ceph'

[Max Carrara]

This commit adds the '/etc/pve/ceph' directory to our overall expected
Ceph configuration.

This directory is meant to store cluster-wide, non-private
configuration files used by Ceph applications and services that are
executed with lower privileges, such as 'ceph-crash.service'.

The existence of the directory is now also checked for when checking
whether Ceph is configured correctly. This makes it easier for our
other tooling to rely on the directory's existence, reducing the
number of otherwise needless frequent checking.

For new clusters: `pveceph init` now creates '/etc/pve/ceph' when
called.

For existing clusters: The 'postinst' hook this commit adds ensures
that '/etc/pve/ceph' is created upon update.

The 'postinst' hook is also version-guarded and does not run when
upgrading from pve-manager version 8.1.5 and above.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new; originally part of patches 09, 10 of series v2 - decided it's
    better to move all this into a separate patch to make context +
    intention clearer
Changes v3 --> v4:
  * none

 PVE/API2/Ceph.pm  |  5 +++++
 PVE/Ceph/Tools.pm | 12 ++++++++++--
 debian/postinst   | 12 ++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

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
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index ee6c515c..735bb116 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -18,6 +18,7 @@ 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_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
 my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
@@ -37,6 +38,7 @@ 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,
 };
@@ -186,8 +188,14 @@ sub check_ceph_inited {
 
     return undef if !check_ceph_installed('ceph_mon', $noerr);
 
-    if (! -f $pve_ceph_cfgpath) {
-	die "pveceph configuration not initialized\n" if !$noerr;
+    my @errors;
+
+    push(@errors, "missing '$pve_ceph_cfgpath'") if ! -f $pve_ceph_cfgpath;
+    push(@errors, "missing '$pve_ceph_cfgdir'") if ! -d $pve_ceph_cfgdir;
+
+    if (@errors) {
+	my $err = 'pveceph configuration not initialized - ' . join(', ', @errors) . "\n";
+	die $err if !$noerr;
 	return undef;
     }
 
diff --git a/debian/postinst b/debian/postinst
index 6138ef6d..61b50f97 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -80,6 +80,14 @@ EOF
     fi
 }
 
+update_ceph_conf() {
+    CEPH_CONF_DIR='/etc/pve/ceph'
+
+    if test ! -d "${CEPH_CONF_DIR}"; then
+        mkdir -p "${CEPH_CONF_DIR}"
+    fi
+}
+
 migrate_apt_auth_conf() {
     output=""
     removed=""
@@ -190,6 +198,10 @@ case "$1" in
 
     set_lvm_conf
 
+    if test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.5~'; then
+        update_ceph_conf
+    fi
+
     if test ! -e /proxmox_install_mode; then
         # modeled after code generated by dh_start
         for unit in ${UNITS}; do
-- 
2.39.2

[pve-devel] [PATCH v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

[Max Carrara]

Due to Ceph dropping privileges when running the 'ceph-crash' daemon
[0], it is necessary to allow the daemon to authenticate with its
cluster in a safe manner.

In order to avoid exposing sensitive keyrings or somehow escalating
its privileges again, 'ceph-crash' is therefore provided with its own
keyring in the '/etc/pve/ceph' directory. This directory, due to being
on 'pmxcfs', may be read by members of the 'www-data' group, which
'ceph-crash' is made part of [1].


Expected Configuration
----------------------

 1. A keyring file named '/etc/pve/ceph/ceph.client.crash.keyring'
    exists
 2. A section named 'client.crash' exists in '/etc/pve/ceph.conf'
 3. The 'client.crash' section has a key named 'keyring' which
    references the keyring file as '/etc/pve/ceph/$cluster.$name.keyring'
 4. The 'client.crash' section has *no* key named 'key'


New Clusters
------------

The keyring file is created and the conf file is updated after the first
monitor has been created (when calling `pveceph mon create`).


Existing Clusters
-----------------

A new helper script creates and configures the 'client.crash' keyring in
`postinst`, if:
 * Ceph is installed
 * Ceph is initialized ('/etc/pve/ceph.conf' and '/etc/pve/ceph' exist)
 * Connection to RADOS is successful

If the above conditions are met, the helper script ensures that the
existing configuration matches the expected configuration mentioned
above.

The configuration is not changed if it is already as expected.

The helper script may be called again manually if the `postinst` hook
fails. It is installed to '/usr/share/pve-manager/helpers/pve-init-ceph-crash'.


Existing `client.crash` Key
---------------------------

If a key named 'client.crash' already exists within the cluster, it is
reused and not regenerated. 


[0]: https://github.com/ceph/ceph/pull/48713
[1]: 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
Changes v2 --> v3:
  * avoid needless second 'pmxcfs' lock on 'ceph.conf' when configuring
    'client.crash' keyring after first monitor was created
  * change name of helper sub that creates the ceph.crash keyring
  * the helper sub now implicitly expects the /etc/pve/ceph directory to
    exist (as the previous commit enforces its existence)
  * remove BASH part of postinst hook related to `ceph-crash`
  * add Perl helper script and call that instead in postinst hook
  * reword commit message
Changes v3 --> v4:
  * ensure key named 'key' does not exist for 'client.crash' section
    in helper script (thanks Friedrich!)
  * restart ceph-crash.service if it wasn't disabled (thanks Friedrich!)
  * add empty line before and after making changes in `update_ceph_conf`
    in 'postinst' hook
  * clean up the helper script; logic is easier to follow now
  * increase verbosity of output of helper script, showing what's
    exactly done when invoked

 PVE/API2/Ceph/MON.pm    |   8 +++
 PVE/Ceph/Tools.pm       |  35 +++++++++++
 bin/Makefile            |   1 +
 bin/pve-init-ceph-crash | 129 ++++++++++++++++++++++++++++++++++++++++
 debian/postinst         |  14 +++++
 5 files changed, 187 insertions(+)
 create mode 100755 bin/pve-init-ceph-crash

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..8b7ce376 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -450,6 +450,14 @@ __PACKAGE__->register_method ({
 			)
 		    };
 		    warn "$@" if $@;
+
+		    print "Configuring keyring for ceph-crash.service\n";
+		    eval {
+			PVE::Ceph::Tools::create_or_update_crash_keyring_file();
+			$cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
+			cfs_write_file('ceph.conf', $cfg);
+		    };
+		    warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
 		}
 
 		eval { PVE::Ceph::Services::ceph_service_cmd('enable', $monsection) };
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 735bb116..9b97171e 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -20,6 +20,7 @@ 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";
@@ -45,6 +46,7 @@ my $config_values = {
 
 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,
@@ -420,6 +422,39 @@ sub get_or_create_admin_keyring {
     return $pve_ckeyring_path;
 }
 
+# is also used in `pve-init-ceph-crash` helper
+sub create_or_update_crash_keyring_file {
+    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 (-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);
+	    return 1;
+	}
+    } else {
+	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+	return 1;
+    }
+
+    return 0;
+}
+
 # get ceph-volume managed osds
 sub ceph_volume_list {
     my $result = {};
diff --git a/bin/Makefile b/bin/Makefile
index 06d8148e..b221e4b1 100644
--- a/bin/Makefile
+++ b/bin/Makefile
@@ -83,6 +83,7 @@ install: $(SCRIPTS) $(CLI_MANS) $(SERVICE_MANS) $(BASH_COMPLETIONS) $(ZSH_COMPLE
 	install -m 0755 $(SCRIPTS) $(BINDIR)
 	install -d $(USRSHARE)/helpers
 	install -m 0755 pve-startall-delay $(USRSHARE)/helpers
+	install -m 0755 pve-init-ceph-crash $(USRSHARE)/helpers
 	install -d $(MAN1DIR)
 	install -m 0644 $(CLI_MANS) $(MAN1DIR)
 	install -d $(MAN8DIR)
diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
new file mode 100755
index 00000000..c8e9217d
--- /dev/null
+++ b/bin/pve-init-ceph-crash
@@ -0,0 +1,129 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use List::Util qw(first);
+
+use PVE::Ceph::Tools;
+use PVE::Cluster;
+use PVE::RADOS;
+use PVE::RPCEnvironment;
+
+my $ceph_cfg_file = 'ceph.conf';
+my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
+
+my $entity = 'client.crash';
+
+
+sub try_adapt_cfg {
+    my ($cfg) = @_;
+
+    my $changed = 0;
+    print("Checking whether the configuration for '$entity' needs to be updated.\n");
+
+    my $add_keyring = sub {
+	print("Setting keyring path to '$keyring_value'.\n");
+	$cfg->{$entity}->{keyring} = $keyring_value;
+	$changed = 1;
+    };
+
+    if (!exists($cfg->{$entity})) {
+	print("Adding missing section for '$entity'.\n");
+	$add_keyring->();
+	return $changed;
+    }
+
+    if (exists($cfg->{$entity}->{key})) {
+	print("Removing existing usage of key.\n");
+	delete($cfg->{$entity}->{key});
+	$changed = 1;
+    }
+
+    if (!exists($cfg->{$entity}->{keyring})) {
+	print("Keyring path is missing from configuration.\n");
+	$add_keyring->();
+	return $changed;
+    }
+
+    my $current_keyring_value = $cfg->{$entity}->{keyring};
+    if ($current_keyring_value ne $keyring_value) {
+	print("Current keyring path differs from expected path.\n");
+	$add_keyring->();
+	return $changed;
+    }
+
+    return $changed;
+}
+
+
+sub main {
+    my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+    $rpcenv->init_request();
+    $rpcenv->set_language($ENV{LANG});
+    $rpcenv->set_user('root@pam');
+
+    die "Not running as root. Exiting.\n" if $> != 0;
+
+    if (!PVE::Ceph::Tools::check_ceph_installed('ceph_bin', 1)) {
+        print("Ceph is not installed. No action required.\n");
+        exit 0;
+    }
+
+    eval {
+        PVE::Ceph::Tools::check_ceph_inited();
+    };
+    if ($@) {
+        print("Ceph is not initialized. No action required.\n");
+        exit 0;
+    }
+
+    my $rados = eval { PVE::RADOS->new() };
+    $@ = ''; # warnings are displayed regardless
+
+    my $ceph_crash_key_path = PVE::Ceph::Tools::get_config('pve_ceph_crash_key_path');
+
+    PVE::Cluster::cfs_lock_file($ceph_cfg_file, undef, sub {
+        my $cfg = PVE::Cluster::cfs_read_file($ceph_cfg_file);
+
+	if (!defined($rados)) {
+	    my $has_mon_config = defined(first { m/mon\..*/ } keys $cfg->%*);
+	    if ($has_mon_config) {
+		die "Connection to RADOS failed even though a monitor is configured.\n" .
+		    "Please verify whether your configuration is correct.\n"
+	    }
+
+	    print(
+		"Connection to RADOS failed and no monitor is configured. ".
+		"Assume that things are fine. No action required.\n"
+	    );
+	    return;
+	}
+
+	my $updated_keyring = PVE::Ceph::Tools::create_or_update_crash_keyring_file($rados);
+
+	if ($updated_keyring) {
+	    print("Keyring file '$ceph_crash_key_path' was updated.\n");
+	}
+
+	my $changed = try_adapt_cfg($cfg);
+
+	if ($changed) {
+	    print("Committing updated configuration.\n");
+	    PVE::Cluster::cfs_write_file($ceph_cfg_file, $cfg);
+	    print("Successfully updated configuration for 'ceph-crash.service'.\n");
+	} else {
+	    print("Configuration does not need to be updated.\n");
+	}
+    });
+    if ($@) {
+	warn("Failed to configure keyring for 'ceph-crash.service'. Error:\n");
+	warn("$@");
+	exit 1;
+    }
+}
+
+main();
+
+0;
diff --git a/debian/postinst b/debian/postinst
index 61b50f97..c36afa13 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -82,10 +82,24 @@ EOF
 
 update_ceph_conf() {
     CEPH_CONF_DIR='/etc/pve/ceph'
+    UNIT='ceph-crash.service'
+
+    echo ""
 
     if test ! -d "${CEPH_CONF_DIR}"; then
         mkdir -p "${CEPH_CONF_DIR}"
     fi
+
+    # Don't fail in case user has "exotic" configuration where RADOS
+    # isn't available on all nodes for some reason
+    /usr/share/pve-manager/helpers/pve-init-ceph-crash || true
+
+    if systemctl -q is-enabled "$UNIT"; then
+        echo "Restarting '$UNIT'"
+        deb-systemd-invoke restart "$UNIT"
+    fi
+
+    echo ""
 }
 
 migrate_apt_auth_conf() {
-- 
2.39.2

[pve-devel] [PATCH v4 pve-manager 16/16] bin/make: gather helper scripts in separate variable

[Max Carrara]

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

 bin/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/bin/Makefile b/bin/Makefile
index b221e4b1..180a91b5 100644
--- a/bin/Makefile
+++ b/bin/Makefile
@@ -25,6 +25,10 @@ SCRIPTS =  			\
 	pveperf			\
 	pvereport
 
+HELPERS =			\
+	pve-startall-delay	\
+	pve-init-ceph-crash
+
 SERVICE_MANS = $(addsuffix .8, $(SERVICES))
 
 CLI_MANS = 				\
@@ -82,8 +86,7 @@ install: $(SCRIPTS) $(CLI_MANS) $(SERVICE_MANS) $(BASH_COMPLETIONS) $(ZSH_COMPLE
 	install -d $(BINDIR)
 	install -m 0755 $(SCRIPTS) $(BINDIR)
 	install -d $(USRSHARE)/helpers
-	install -m 0755 pve-startall-delay $(USRSHARE)/helpers
-	install -m 0755 pve-init-ceph-crash $(USRSHARE)/helpers
+	install -m 0755 $(HELPERS) $(USRSHARE)/helpers
 	install -d $(MAN1DIR)
 	install -m 0644 $(CLI_MANS) $(MAN1DIR)
 	install -d $(MAN8DIR)
-- 
2.39.2

Re: [pve-devel] [PATCH v4 ceph master, ceph quincy-stable-8, pve-storage, pve-manager 0/16] Fix #4759: Configure Permissions for ceph-crash.service

[Friedrich Weber]

Tested setting up a fresh Reef with patched packages, and tested
updating an existing Reef with the new packages. In both cases, crashes
are posted without noise in the journal and without having to manually
restart ceph-crash. Nice!

Also tested the case where [client.crash] already has a `key` (see my
comment on [1]) -- was properly rewritten to a `keyring` for me.

Don't have a Quincy cluster currently, so didn't test the backport.

One thing I noticed and already discussed with Max off-list: When
updating to the patched pve-manager package on a cluster without Ceph, I
get the following message:

> Setting up pve-manager (8.1.4) ...
> Ceph is not initialized. No action required.
> Failed to get unit file state for ceph-crash.service: No such file or
> directory

It's harmless, but might be confusing to users, so might be good to get
rid of.

Unrelated to this patch series, there are also the following message
from ceph-base:

> Setting system user ceph properties..usermod: no changes
> usermod: unlocking the user's password would result in a passwordless
> account.
> You should set a password with usermod -p to unlock this user's >
password.
> ..done
> chown: cannot access '/var/log/ceph/*.log*': No such file or directory
> Fixing /var/run/ceph ownership....done

Both the `usermod` stuff as well as the `chown` would be nice to
silence, but probably out of scope for this patch series.

[1] https://lists.proxmox.com/pipermail/pve-devel/2024-February/061956.html

On 05/03/2024 16:07, Max Carrara wrote:
> Fix #4759: Configure Permissions for ceph-crash.service - Version 4
> ===================================================================
> 
> Notable changes since v3
> ------------------------
> 
>   * Both parser and writer for 'ceph.conf' now have unit tests which run
>     during make targets like e.g. `make deb`, thanks to `dh_auto_test`
>   * The parser for 'ceph.conf' now correctly un-escapes comment literals
>     (found while developing unit tests)
>   * The writer for 'ceph.conf' now correctly escapes comment literals
>     (found while developing unit tests)
>   * The helper script called in 'postinst' of pve-manager for updating
>     'ceph.crash' in 'ceph.conf' now correctly handles an existing key
>     being referenced directly and removes it (thanks Friedrich!)
>   * The aforementioned helper script has more verbose output, showing
>     explicitly what's being done to the configuration
>   * The 'postinst' hook now prints an empty line before and after it
>     runs to make it a little more visible
>   * The 'postinst' hook now also restarts 'ceph-crash.service' if the
>     user hasn't disabled it (thanks Friedrich!)
> 
> For a detailed list of changes, please see the comments in the
> individual patches.
> 
> 
> Older Versions
> --------------
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061546.html
> v2: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061646.html
> v3: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061802.html
> 
> Summary of Changes
> ------------------
> 
> 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
> 
>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>  patches/series                                |  2 +
>  3 files changed, 86 insertions(+)
>  create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>  create mode 100644 patches/0017-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
> 
>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>  patches/series                                |  2 +
>  3 files changed, 86 insertions(+)
>  create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>  create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch
> 
> 
> pve-storage:
> 
> Max Carrara (9):
>   cephconfig: align our parser more with Ceph's parser
>   cephconfig: support line-continuations in parser
>   cephconfig: allow writing arbitrary sections
>   cephconfig: support escaped comment literals
>   cephconfig: emit warning for lines that fail to parse
>   cephconfig: change code style inside config writer
>   cephconfig: change order of written sections
>   cephconfig: remove leading whitespace on write to Ceph config
>   test: add tests for 'ceph.conf' parser and writer
> 
>  src/Makefile                               |   1 +
>  src/PVE/CephConfig.pm                      |  95 +++--
>  src/PVE/Makefile                           |   4 +
>  src/PVE/test/Makefile                      |   9 +
>  src/PVE/test/ceph_conf_parse_write_test.pl | 402 +++++++++++++++++++++
>  5 files changed, 490 insertions(+), 21 deletions(-)
>  create mode 100644 src/PVE/test/Makefile
>  create mode 100755 src/PVE/test/ceph_conf_parse_write_test.pl
> 
> 
> pve-manager:
> 
> Max Carrara (3):
>   ceph: introduce '/etc/pve/ceph'
>   fix #4759: ceph: configure ceph-crash.service and its key
>   bin/make: gather helper scripts in separate variable
> 
>  PVE/API2/Ceph.pm        |   5 ++
>  PVE/API2/Ceph/MON.pm    |   8 +++
>  PVE/Ceph/Tools.pm       |  47 ++++++++++++++-
>  bin/Makefile            |   6 +-
>  bin/pve-init-ceph-crash | 129 ++++++++++++++++++++++++++++++++++++++++
>  debian/postinst         |  26 ++++++++
>  6 files changed, 218 insertions(+), 3 deletions(-)
>  create mode 100755 bin/pve-init-ceph-crash
> 

[pve-devel] partially-applied-series: [PATCH v4 ceph master, ceph quincy-stable-8, pve-storage, pve-manager 0/16] Fix #4759: Configure Permissions for ceph-crash.service

[Thomas Lamprecht]

Am 05/03/2024 um 16:07 schrieb Max Carrara:
> 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
> 
>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>  patches/series                                |  2 +
>  3 files changed, 86 insertions(+)
>  create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>  create mode 100644 patches/0017-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
> 
>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>  patches/series                                |  2 +
>  3 files changed, 86 insertions(+)
>  create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>  create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch


applied above ceph related commits, with slight change to subject and
adding a reference to your upstream PR, thanks!

Re: [pve-devel] [PATCH v4 pve-storage 13/16] test: add tests for 'ceph.conf' parser and writer

[Fabian Grünbichler]

On March 5, 2024 4:07 pm, Max Carrara wrote:
> These tests attempt to cover all syntax quirks that the format of
> 'ceph.conf' currently allows.
> 
> One known exception however is the handling of "quoted" and "unquoted"
> strings, as Ceph's own parser appears to not actually differ between
> them either. Curiously, if a "quoted string" isn't able to be
> parsed by Ceph's implementation, it goes on to parse it as an
> "unquoted string" anyway. [0] In both cases, the result is the same -
> the string is parsed with quotes.

I don't think this is true - quoted strings do behave differently w.r.t.
in-line comments and line continuations inside the quoted string (both
break parsing on the ceph side ;)), and in general a missing closing
quote seems to be fatal.

> Each test case is first tested against the parser - if the resulting
> hash matches the expected hash, it is consequently passed into the
> writer. The writer's result is then parsed another time and compared
> against the expected hash once more.

I think it would make a lot of sense to also test our parser against
`ceph-conf -c TEST_INPUT_FILE [--name X.Y] --show-config-value KEY`

in particular for all the edge cases surrounding quoting and so on it
might help find some more discrepancies, but also help keeping current
with ceph changes..

> [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l189
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Re: [pve-devel] [PATCH v4 pve-storage 06/16] cephconfig: support line-continuations in parser

[Fabian Grünbichler]

On March 5, 2024 4:07 pm, Max Carrara wrote:
> Ceph's docs state the following [0]:
>> The backslash character `\` is used as the line-continuation marker
>> that combines the next line with the current one.
> 
> This commit implements the support of such line-continuations in our
> parser.
> 
> The line following a line ending with a '\' has its whitespace
> preserved, which matches the behaviour in Ceph's original
> implementation [1]. In other words, leading and trailing whitespace is
> not stripped from a continued line.

it's actually a bit more complicated.. ceph only supports line
continuations inside values (well, in key value lines after the key ;)),
and only if they are unquoted..

> 
> [0]: https://docs.ceph.com/en/reef/rados/configuration/ceph-conf/#changes-introduced-in-octopus
> [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l262
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v2 --> v3:
>   * new
> Changes v3 --> v4:
>   * none
> 
>  src/PVE/CephConfig.pm | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 74a92eb..80f71b0 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -19,13 +19,33 @@ sub parse_ceph_config {
>      return $cfg if !defined($raw);
>  
>      my @lines = split /\n/, $raw;
> +    my @lines_normalized;
> +
> +    my $re_comment_not_escaped = qr/(?<!\\)(#|;).*$/;
> +    my $re_leading_ws = qr/^\s+/;
> +    my $re_trailing_ws = qr/\s+$/;
> +
> +    while (scalar(@lines)) {
> +	my $line = shift(@lines);
> +	$line =~ s/$re_comment_not_escaped//;
> +	$line =~ s/$re_leading_ws//;
> +	$line =~ s/$re_trailing_ws//;
> +	next if !$line;
> +
> +	# merge lines ending with continuation character '\'
> +	while ($line =~ s/\\$//) {
> +	    my $next_line = shift(@lines);
> +	    $next_line =~ s/$re_comment_not_escaped//;
> +	    $next_line =~ s/$re_trailing_ws//;
> +	    $line .= $next_line;
> +	}
> +
> +	push(@lines_normalized, $line);
> +    }
>  
>      my $section;
>  
> -    for my $line (@lines) {
> -	$line =~ s/(?<!\\)(#|;).*$//;
> -	$line =~ s/^\s+//;
> -	$line =~ s/\s+$//;
> +    for my $line (@lines_normalized) {
>  	next if !$line;
>  
>  	if ($line =~ m/^\[(.+)\]$/) {
> -- 
> 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 v4 pve-storage 05/16] cephconfig: align our parser more with Ceph's parser

[Fabian Grünbichler]

On March 5, 2024 4:07 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.
> 
>  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
>     are treated the same, like in Ceph's parser [2].
> 
>  4. Although not crucial for Ceph, our parser now also supports empty
>     sections. When a section header is successfully parsed, it gets
>     added to the configuration hash and the parser continues operating
>     on the next line.
> 
> [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
> Changes v2 --> v3:
>   * support comment literals (4.)
> Changes v3 --> v4:
>   * support empty sections
>   * fix and move support for comment literals to separate patch
> 
>  src/PVE/CephConfig.pm | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 6b10d46..74a92eb 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,18 @@ sub parse_ceph_config {
>  
>      my $section;
>  
> -    foreach my $line (@lines) {
> -	$line =~ s/#.*$//;
> +    for my $line (@lines) {
> +	$line =~ s/(?<!\\)(#|;).*$//;

nit: [#;] instead of the group works as well, and is a bit more
idiomatic IMHO.

so this seems to agree with the parser grammar, but it kind of disagrees
with the docs that for example state:

> mon_host
> 
> This is a list of IP addresses or hostnames that are separated by commas, whitespace, or semicolons. 

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

well, they don't mention there that you need to escape the `;` for that
to work, else only the first "entry" takes, and the rest is silently
interpreted as comment. if you quote the "list", then it's even a parse
error since the closing quote is interpreted as part of the comment ;)

but unfortunately there is one more issue:

\\;

still marks the start of a comment (since it's the backlash that is
being escaped) in ceph's parser (same applies to '#' as well).

what a mess this file format is..

>  	$line =~ s/^\s+//;
> -	$line =~ s/^;.*$//;
>  	$line =~ s/\s+$//;
>  	next if !$line;
>  
> -	$section = $1 if $line =~ m/^\[(\S+)\]$/;
> +	if ($line =~ m/^\[(.+)\]$/) {
> +	    $section = $1;
> +	    $cfg->{$section} = {} if !exists($cfg->{$section});
> +	    next;
> +	}
> +
>  	if (!$section) {
>  	    warn "no section - skip: $line\n";
>  	    next;
> @@ -35,11 +41,12 @@ 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;
> -	}
>  
> +	    next;

this next serves no purpose? (ah, it does with one of the later patches
that adds code below, so please move this to that patch!)

> +	}
>      }
>  
>      return $cfg;
> -- 
> 2.39.2

Re: [pve-devel] [PATCH v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

[Fabian Grünbichler]

On March 5, 2024 4:07 pm, Max Carrara wrote:
> Due to Ceph dropping privileges when running the 'ceph-crash' daemon
> [0], it is necessary to allow the daemon to authenticate with its
> cluster in a safe manner.
> 
> In order to avoid exposing sensitive keyrings or somehow escalating
> its privileges again, 'ceph-crash' is therefore provided with its own
> keyring in the '/etc/pve/ceph' directory. This directory, due to being
> on 'pmxcfs', may be read by members of the 'www-data' group, which
> 'ceph-crash' is made part of [1].
> 
> 
> Expected Configuration
> ----------------------
> 
>  1. A keyring file named '/etc/pve/ceph/ceph.client.crash.keyring'
>     exists
>  2. A section named 'client.crash' exists in '/etc/pve/ceph.conf'
>  3. The 'client.crash' section has a key named 'keyring' which
>     references the keyring file as '/etc/pve/ceph/$cluster.$name.keyring'
>  4. The 'client.crash' section has *no* key named 'key'
> 
> 
> New Clusters
> ------------
> 
> The keyring file is created and the conf file is updated after the first
> monitor has been created (when calling `pveceph mon create`).
> 
> 
> Existing Clusters
> -----------------
> 
> A new helper script creates and configures the 'client.crash' keyring in
> `postinst`, if:
>  * Ceph is installed
>  * Ceph is initialized ('/etc/pve/ceph.conf' and '/etc/pve/ceph' exist)
>  * Connection to RADOS is successful
> 
> If the above conditions are met, the helper script ensures that the
> existing configuration matches the expected configuration mentioned
> above.
> 
> The configuration is not changed if it is already as expected.
> 
> The helper script may be called again manually if the `postinst` hook
> fails. It is installed to '/usr/share/pve-manager/helpers/pve-init-ceph-crash'.
> 
> 
> Existing `client.crash` Key
> ---------------------------
> 
> If a key named 'client.crash' already exists within the cluster, it is
> reused and not regenerated. 
> 
> 
> [0]: https://github.com/ceph/ceph/pull/48713
> [1]: 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
> Changes v2 --> v3:
>   * avoid needless second 'pmxcfs' lock on 'ceph.conf' when configuring
>     'client.crash' keyring after first monitor was created
>   * change name of helper sub that creates the ceph.crash keyring
>   * the helper sub now implicitly expects the /etc/pve/ceph directory to
>     exist (as the previous commit enforces its existence)
>   * remove BASH part of postinst hook related to `ceph-crash`
>   * add Perl helper script and call that instead in postinst hook
>   * reword commit message
> Changes v3 --> v4:
>   * ensure key named 'key' does not exist for 'client.crash' section
>     in helper script (thanks Friedrich!)
>   * restart ceph-crash.service if it wasn't disabled (thanks Friedrich!)
>   * add empty line before and after making changes in `update_ceph_conf`
>     in 'postinst' hook
>   * clean up the helper script; logic is easier to follow now
>   * increase verbosity of output of helper script, showing what's
>     exactly done when invoked
> 
>  PVE/API2/Ceph/MON.pm    |   8 +++
>  PVE/Ceph/Tools.pm       |  35 +++++++++++
>  bin/Makefile            |   1 +
>  bin/pve-init-ceph-crash | 129 ++++++++++++++++++++++++++++++++++++++++
>  debian/postinst         |  14 +++++
>  5 files changed, 187 insertions(+)
>  create mode 100755 bin/pve-init-ceph-crash
> 
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..8b7ce376 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -450,6 +450,14 @@ __PACKAGE__->register_method ({
>  			)
>  		    };
>  		    warn "$@" if $@;
> +
> +		    print "Configuring keyring for ceph-crash.service\n";
> +		    eval {
> +			PVE::Ceph::Tools::create_or_update_crash_keyring_file();
> +			$cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
> +			cfs_write_file('ceph.conf', $cfg);
> +		    };
> +		    warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
>  		}
>  
>  		eval { PVE::Ceph::Services::ceph_service_cmd('enable', $monsection) };
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 735bb116..9b97171e 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -20,6 +20,7 @@ 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";
> @@ -45,6 +46,7 @@ my $config_values = {
>  
>  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,
> @@ -420,6 +422,39 @@ sub get_or_create_admin_keyring {
>      return $pve_ckeyring_path;
>  }
>  
> +# is also used in `pve-init-ceph-crash` helper
> +sub create_or_update_crash_keyring_file {
> +    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 (-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);
> +	    return 1;
> +	}
> +    } else {
> +	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> +	return 1;
> +    }
> +
> +    return 0;
> +}
> +
>  # get ceph-volume managed osds
>  sub ceph_volume_list {
>      my $result = {};
> diff --git a/bin/Makefile b/bin/Makefile
> index 06d8148e..b221e4b1 100644
> --- a/bin/Makefile
> +++ b/bin/Makefile
> @@ -83,6 +83,7 @@ install: $(SCRIPTS) $(CLI_MANS) $(SERVICE_MANS) $(BASH_COMPLETIONS) $(ZSH_COMPLE
>  	install -m 0755 $(SCRIPTS) $(BINDIR)
>  	install -d $(USRSHARE)/helpers
>  	install -m 0755 pve-startall-delay $(USRSHARE)/helpers
> +	install -m 0755 pve-init-ceph-crash $(USRSHARE)/helpers
>  	install -d $(MAN1DIR)
>  	install -m 0644 $(CLI_MANS) $(MAN1DIR)
>  	install -d $(MAN8DIR)
> diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
> new file mode 100755
> index 00000000..c8e9217d
> --- /dev/null
> +++ b/bin/pve-init-ceph-crash
> @@ -0,0 +1,129 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use List::Util qw(first);
> +
> +use PVE::Ceph::Tools;
> +use PVE::Cluster;
> +use PVE::RADOS;
> +use PVE::RPCEnvironment;
> +
> +my $ceph_cfg_file = 'ceph.conf';
> +my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
> +
> +my $entity = 'client.crash';

nit: this could be inlined?

> +
> +
> +sub try_adapt_cfg {
> +    my ($cfg) = @_;
> +
> +    my $changed = 0;
> +    print("Checking whether the configuration for '$entity' needs to be updated.\n");
> +
> +    my $add_keyring = sub {
> +	print("Setting keyring path to '$keyring_value'.\n");
> +	$cfg->{$entity}->{keyring} = $keyring_value;
> +	$changed = 1;

this can be removed, you always return right after calling this sub..

> +    };
> +
> +    if (!exists($cfg->{$entity})) {
> +	print("Adding missing section for '$entity'.\n");
> +	$add_keyring->();
> +	return $changed;

and all of these can be changed to uncondtionally return 1

> +    }
> +
> +    if (exists($cfg->{$entity}->{key})) {
> +	print("Removing existing usage of key.\n");
> +	delete($cfg->{$entity}->{key});
> +	$changed = 1;
> +    }
> +
> +    if (!exists($cfg->{$entity}->{keyring})) {
> +	print("Keyring path is missing from configuration.\n");
> +	$add_keyring->();
> +	return $changed;
> +    }
> +
> +    my $current_keyring_value = $cfg->{$entity}->{keyring};
> +    if ($current_keyring_value ne $keyring_value) {
> +	print("Current keyring path differs from expected path.\n");
> +	$add_keyring->();
> +	return $changed;
> +    }
> +
> +    return $changed;

$changed only serves the purpose of returning 1 iff only the 'key'
value/entry was removed. so it could be renamed to reflect that ;)

> +}
> +
> +
> +sub main {
> +    my $rpcenv = PVE::RPCEnvironment->init('cli');
> +
> +    $rpcenv->init_request();
> +    $rpcenv->set_language($ENV{LANG});
> +    $rpcenv->set_user('root@pam');

why do we need an rpcenv here?

> +
> +    die "Not running as root. Exiting.\n" if $> != 0;
> +
> +    if (!PVE::Ceph::Tools::check_ceph_installed('ceph_bin', 1)) {
> +        print("Ceph is not installed. No action required.\n");
> +        exit 0;
> +    }
> +
> +    eval {
> +        PVE::Ceph::Tools::check_ceph_inited();
> +    };
> +    if ($@) {
> +        print("Ceph is not initialized. No action required.\n");
> +        exit 0;
> +    }
> +
> +    my $rados = eval { PVE::RADOS->new() };
> +    $@ = ''; # warnings are displayed regardless

what's this line for?

> +    my $ceph_crash_key_path = PVE::Ceph::Tools::get_config('pve_ceph_crash_key_path');
> +
> +    PVE::Cluster::cfs_lock_file($ceph_cfg_file, undef, sub {

if you let this sub return a boolean value (worked/failed)

> +        my $cfg = PVE::Cluster::cfs_read_file($ceph_cfg_file);
> +
> +	if (!defined($rados)) {
> +	    my $has_mon_config = defined(first { m/mon\..*/ } keys $cfg->%*);
> +	    if ($has_mon_config) {
> +		die "Connection to RADOS failed even though a monitor is configured.\n" .
> +		    "Please verify whether your configuration is correct.\n"
> +	    }
> +
> +	    print(
> +		"Connection to RADOS failed and no monitor is configured. ".
> +		"Assume that things are fine. No action required.\n"
> +	    );

not sure if it wouldn't be better to check mon_host here? that's the
thing actually needed to contact the mon, everything else is optional..
and even that could be moved to DNS, but we don't support that for
PVE-managed clusters..

> +	    return;
> +	}
> +
> +	my $updated_keyring = PVE::Ceph::Tools::create_or_update_crash_keyring_file($rados);
> +
> +	if ($updated_keyring) {
> +	    print("Keyring file '$ceph_crash_key_path' was updated.\n");
> +	}
> +
> +	my $changed = try_adapt_cfg($cfg);
> +
> +	if ($changed) {
> +	    print("Committing updated configuration.\n");
> +	    PVE::Cluster::cfs_write_file($ceph_cfg_file, $cfg);
> +	    print("Successfully updated configuration for 'ceph-crash.service'.\n");
> +	} else {
> +	    print("Configuration does not need to be updated.\n");
> +	}
> +    });

then you can just check the return value (undef means cfs_lock set an
error, true means it worked, false means it failed without dieing, or
whatever you want to return)

> +    if ($@) {
> +	warn("Failed to configure keyring for 'ceph-crash.service'. Error:\n");

nit: I think I'd prefer the "Error: to be on the next line with the error message.."

> +	warn("$@");
> +	exit 1;
> +    }
> +}
> +
> +main();
> +
> +0;

why?

> diff --git a/debian/postinst b/debian/postinst
> index 61b50f97..c36afa13 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -82,10 +82,24 @@ EOF
>  
>  update_ceph_conf() {
>      CEPH_CONF_DIR='/etc/pve/ceph'
> +    UNIT='ceph-crash.service'
> +
> +    echo ""
>  
>      if test ! -d "${CEPH_CONF_DIR}"; then
>          mkdir -p "${CEPH_CONF_DIR}"
>      fi
> +
> +    # Don't fail in case user has "exotic" configuration where RADOS
> +    # isn't available on all nodes for some reason
> +    /usr/share/pve-manager/helpers/pve-init-ceph-crash || true
> +
> +    if systemctl -q is-enabled "$UNIT"; then

like Friedrich said, please silence this (`2>/dev/null`)

> +        echo "Restarting '$UNIT'"
> +        deb-systemd-invoke restart "$UNIT"

|| true please!

> +    fi
> +
> +    echo ""
>  }
>  
>  migrate_apt_auth_conf() {
> -- 
> 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 v4 pve-manager 14/16] ceph: introduce '/etc/pve/ceph'

[Fabian Grünbichler]

On March 5, 2024 4:07 pm, Max Carrara wrote:
> This commit adds the '/etc/pve/ceph' directory to our overall expected
> Ceph configuration.
> 
> This directory is meant to store cluster-wide, non-private
> configuration files used by Ceph applications and services that are
> executed with lower privileges, such as 'ceph-crash.service'.
> 
> The existence of the directory is now also checked for when checking
> whether Ceph is configured correctly. This makes it easier for our
> other tooling to rely on the directory's existence, reducing the
> number of otherwise needless frequent checking.
> 
> For new clusters: `pveceph init` now creates '/etc/pve/ceph' when
> called.
> 
> For existing clusters: The 'postinst' hook this commit adds ensures
> that '/etc/pve/ceph' is created upon update.
> 
> The 'postinst' hook is also version-guarded and does not run when
> upgrading from pve-manager version 8.1.5 and above.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v2 --> v3:
>   * new; originally part of patches 09, 10 of series v2 - decided it's
>     better to move all this into a separate patch to make context +
>     intention clearer
> Changes v3 --> v4:
>   * none
> 
>  PVE/API2/Ceph.pm  |  5 +++++
>  PVE/Ceph/Tools.pm | 12 ++++++++++--
>  debian/postinst   | 12 ++++++++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> 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
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index ee6c515c..735bb116 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -18,6 +18,7 @@ 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_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>  my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
> @@ -37,6 +38,7 @@ 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,
>  };
> @@ -186,8 +188,14 @@ sub check_ceph_inited {
>  
>      return undef if !check_ceph_installed('ceph_mon', $noerr);
>  
> -    if (! -f $pve_ceph_cfgpath) {
> -	die "pveceph configuration not initialized\n" if !$noerr;
> +    my @errors;
> +
> +    push(@errors, "missing '$pve_ceph_cfgpath'") if ! -f $pve_ceph_cfgpath;
> +    push(@errors, "missing '$pve_ceph_cfgdir'") if ! -d $pve_ceph_cfgdir;
> +
> +    if (@errors) {
> +	my $err = 'pveceph configuration not initialized - ' . join(', ', @errors) . "\n";
> +	die $err if !$noerr;
>  	return undef;
>      }
>  
> diff --git a/debian/postinst b/debian/postinst
> index 6138ef6d..61b50f97 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -80,6 +80,14 @@ EOF
>      fi
>  }
>  
> +update_ceph_conf() {
> +    CEPH_CONF_DIR='/etc/pve/ceph'
> +
> +    if test ! -d "${CEPH_CONF_DIR}"; then
> +        mkdir -p "${CEPH_CONF_DIR}"
> +    fi
> +}
> +
>  migrate_apt_auth_conf() {
>      output=""
>      removed=""
> @@ -190,6 +198,10 @@ case "$1" in
>  
>      set_lvm_conf
>  
> +    if test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.5~'; then

nit: never hard code such versions in patches unless you know they will be
applied right away ;) instead, have a placeholder here and call it out
prominently in the patch notes and cover letter.

> +        update_ceph_conf
> +    fi
> +
>      if test ! -e /proxmox_install_mode; then
>          # modeled after code generated by dh_start
>          for unit in ${UNITS}; do
> -- 
> 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 v4 pve-storage 05/16] cephconfig: align our parser more with Ceph's parser

[Max Carrara]

On Tue Mar 19, 2024 at 10:38 AM CET, Fabian Grünbichler wrote:
> On March 5, 2024 4:07 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.
> > 
> >  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
> >     are treated the same, like in Ceph's parser [2].
> > 
> >  4. Although not crucial for Ceph, our parser now also supports empty
> >     sections. When a section header is successfully parsed, it gets
> >     added to the configuration hash and the parser continues operating
> >     on the next line.
> > 
> > [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
> > Changes v2 --> v3:
> >   * support comment literals (4.)
> > Changes v3 --> v4:
> >   * support empty sections
> >   * fix and move support for comment literals to separate patch
> > 
> >  src/PVE/CephConfig.pm | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> > index 6b10d46..74a92eb 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,18 @@ sub parse_ceph_config {
> >  
> >      my $section;
> >  
> > -    foreach my $line (@lines) {
> > -	$line =~ s/#.*$//;
> > +    for my $line (@lines) {
> > +	$line =~ s/(?<!\\)(#|;).*$//;
>
> nit: [#;] instead of the group works as well, and is a bit more
> idiomatic IMHO.

Good catch, thanks!

>
> so this seems to agree with the parser grammar, but it kind of disagrees
> with the docs that for example state:
>
> > mon_host
> > 
> > This is a list of IP addresses or hostnames that are separated by commas, whitespace, or semicolons. 
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#confval-mon_host
>
> well, they don't mention there that you need to escape the `;` for that
> to work, else only the first "entry" takes, and the rest is silently
> interpreted as comment. if you quote the "list", then it's even a parse
> error since the closing quote is interpreted as part of the comment ;)
>
> but unfortunately there is one more issue:
>
> \\;
>
> still marks the start of a comment (since it's the backlash that is
> being escaped) in ceph's parser (same applies to '#' as well).

Oh, that's interesting. Very good catch, thank you! Seems like I'll have
to revise the whole parsing logic again in the next series, *sigh* ...

>
> what a mess this file format is..

I wholeheartedly agree.

>
> >  	$line =~ s/^\s+//;
> > -	$line =~ s/^;.*$//;
> >  	$line =~ s/\s+$//;
> >  	next if !$line;
> >  
> > -	$section = $1 if $line =~ m/^\[(\S+)\]$/;
> > +	if ($line =~ m/^\[(.+)\]$/) {
> > +	    $section = $1;
> > +	    $cfg->{$section} = {} if !exists($cfg->{$section});
> > +	    next;
> > +	}
> > +
> >  	if (!$section) {
> >  	    warn "no section - skip: $line\n";
> >  	    next;
> > @@ -35,11 +41,12 @@ 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;
> > -	}
> >  
> > +	    next;
>
> this next serves no purpose? (ah, it does with one of the later patches
> that adds code below, so please move this to that patch!)

My bad, will do!

>
> > +	}
> >      }
> >  
> >      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 v4 pve-storage 06/16] cephconfig: support line-continuations in parser

[Max Carrara]

On Tue Mar 19, 2024 at 10:37 AM CET, Fabian Grünbichler wrote:
> On March 5, 2024 4:07 pm, Max Carrara wrote:
> > Ceph's docs state the following [0]:
> >> The backslash character `\` is used as the line-continuation marker
> >> that combines the next line with the current one.
> > 
> > This commit implements the support of such line-continuations in our
> > parser.
> > 
> > The line following a line ending with a '\' has its whitespace
> > preserved, which matches the behaviour in Ceph's original
> > implementation [1]. In other words, leading and trailing whitespace is
> > not stripped from a continued line.
>
> it's actually a bit more complicated.. ceph only supports line
> continuations inside values (well, in key value lines after the key ;)),
> and only if they are unquoted..

As mentioned in my other reply, I'll probably have to revise the whole
parsing logic to take that into account... but thanks for being so
thorough!

>
> > 
> > [0]: https://docs.ceph.com/en/reef/rados/configuration/ceph-conf/#changes-introduced-in-octopus
> > [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l262
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> > Changes v2 --> v3:
> >   * new
> > Changes v3 --> v4:
> >   * none
> > 
> >  src/PVE/CephConfig.pm | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> > index 74a92eb..80f71b0 100644
> > --- a/src/PVE/CephConfig.pm
> > +++ b/src/PVE/CephConfig.pm
> > @@ -19,13 +19,33 @@ sub parse_ceph_config {
> >      return $cfg if !defined($raw);
> >  
> >      my @lines = split /\n/, $raw;
> > +    my @lines_normalized;
> > +
> > +    my $re_comment_not_escaped = qr/(?<!\\)(#|;).*$/;
> > +    my $re_leading_ws = qr/^\s+/;
> > +    my $re_trailing_ws = qr/\s+$/;
> > +
> > +    while (scalar(@lines)) {
> > +	my $line = shift(@lines);
> > +	$line =~ s/$re_comment_not_escaped//;
> > +	$line =~ s/$re_leading_ws//;
> > +	$line =~ s/$re_trailing_ws//;
> > +	next if !$line;
> > +
> > +	# merge lines ending with continuation character '\'
> > +	while ($line =~ s/\\$//) {
> > +	    my $next_line = shift(@lines);
> > +	    $next_line =~ s/$re_comment_not_escaped//;
> > +	    $next_line =~ s/$re_trailing_ws//;
> > +	    $line .= $next_line;
> > +	}
> > +
> > +	push(@lines_normalized, $line);
> > +    }
> >  
> >      my $section;
> >  
> > -    for my $line (@lines) {
> > -	$line =~ s/(?<!\\)(#|;).*$//;
> > -	$line =~ s/^\s+//;
> > -	$line =~ s/\s+$//;
> > +    for my $line (@lines_normalized) {
> >  	next if !$line;
> >  
> >  	if ($line =~ m/^\[(.+)\]$/) {
> > -- 
> > 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 v4 pve-storage 13/16] test: add tests for 'ceph.conf' parser and writer

[Max Carrara]

On Tue Mar 19, 2024 at 10:36 AM CET, Fabian Grünbichler wrote:
> On March 5, 2024 4:07 pm, Max Carrara wrote:
> > These tests attempt to cover all syntax quirks that the format of
> > 'ceph.conf' currently allows.
> > 
> > One known exception however is the handling of "quoted" and "unquoted"
> > strings, as Ceph's own parser appears to not actually differ between
> > them either. Curiously, if a "quoted string" isn't able to be
> > parsed by Ceph's implementation, it goes on to parse it as an
> > "unquoted string" anyway. [0] In both cases, the result is the same -
> > the string is parsed with quotes.
>
> I don't think this is true - quoted strings do behave differently w.r.t.
> in-line comments and line continuations inside the quoted string (both
> break parsing on the ceph side ;)), and in general a missing closing
> quote seems to be fatal.

See my other replies.

>
> > Each test case is first tested against the parser - if the resulting
> > hash matches the expected hash, it is consequently passed into the
> > writer. The writer's result is then parsed another time and compared
> > against the expected hash once more.
>
> I think it would make a lot of sense to also test our parser against
> `ceph-conf -c TEST_INPUT_FILE [--name X.Y] --show-config-value KEY`
>
> in particular for all the edge cases surrounding quoting and so on it
> might help find some more discrepancies, but also help keeping current
> with ceph changes..

Very good point, I will add this in a separate commit in v5. Thanks!

>
> > [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l189
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v4 pve-manager 14/16] ceph: introduce '/etc/pve/ceph'

[Max Carrara]

On Tue Mar 19, 2024 at 11:04 AM CET, Fabian Grünbichler wrote:
> On March 5, 2024 4:07 pm, Max Carrara wrote:
> > This commit adds the '/etc/pve/ceph' directory to our overall expected
> > Ceph configuration.
> > 
> > This directory is meant to store cluster-wide, non-private
> > configuration files used by Ceph applications and services that are
> > executed with lower privileges, such as 'ceph-crash.service'.
> > 
> > The existence of the directory is now also checked for when checking
> > whether Ceph is configured correctly. This makes it easier for our
> > other tooling to rely on the directory's existence, reducing the
> > number of otherwise needless frequent checking.
> > 
> > For new clusters: `pveceph init` now creates '/etc/pve/ceph' when
> > called.
> > 
> > For existing clusters: The 'postinst' hook this commit adds ensures
> > that '/etc/pve/ceph' is created upon update.
> > 
> > The 'postinst' hook is also version-guarded and does not run when
> > upgrading from pve-manager version 8.1.5 and above.
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> > Changes v2 --> v3:
> >   * new; originally part of patches 09, 10 of series v2 - decided it's
> >     better to move all this into a separate patch to make context +
> >     intention clearer
> > Changes v3 --> v4:
> >   * none
> > 
> >  PVE/API2/Ceph.pm  |  5 +++++
> >  PVE/Ceph/Tools.pm | 12 ++++++++++--
> >  debian/postinst   | 12 ++++++++++++
> >  3 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > 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
> > diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> > index ee6c515c..735bb116 100644
> > --- a/PVE/Ceph/Tools.pm
> > +++ b/PVE/Ceph/Tools.pm
> > @@ -18,6 +18,7 @@ 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_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
> >  my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
> > @@ -37,6 +38,7 @@ 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,
> >  };
> > @@ -186,8 +188,14 @@ sub check_ceph_inited {
> >  
> >      return undef if !check_ceph_installed('ceph_mon', $noerr);
> >  
> > -    if (! -f $pve_ceph_cfgpath) {
> > -	die "pveceph configuration not initialized\n" if !$noerr;
> > +    my @errors;
> > +
> > +    push(@errors, "missing '$pve_ceph_cfgpath'") if ! -f $pve_ceph_cfgpath;
> > +    push(@errors, "missing '$pve_ceph_cfgdir'") if ! -d $pve_ceph_cfgdir;
> > +
> > +    if (@errors) {
> > +	my $err = 'pveceph configuration not initialized - ' . join(', ', @errors) . "\n";
> > +	die $err if !$noerr;
> >  	return undef;
> >      }
> >  
> > diff --git a/debian/postinst b/debian/postinst
> > index 6138ef6d..61b50f97 100755
> > --- a/debian/postinst
> > +++ b/debian/postinst
> > @@ -80,6 +80,14 @@ EOF
> >      fi
> >  }
> >  
> > +update_ceph_conf() {
> > +    CEPH_CONF_DIR='/etc/pve/ceph'
> > +
> > +    if test ! -d "${CEPH_CONF_DIR}"; then
> > +        mkdir -p "${CEPH_CONF_DIR}"
> > +    fi
> > +}
> > +
> >  migrate_apt_auth_conf() {
> >      output=""
> >      removed=""
> > @@ -190,6 +198,10 @@ case "$1" in
> >  
> >      set_lvm_conf
> >  
> > +    if test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.5~'; then
>
> nit: never hard code such versions in patches unless you know they will be
> applied right away ;) instead, have a placeholder here and call it out
> prominently in the patch notes and cover letter.

Thanks, will do!

>
> > +        update_ceph_conf
> > +    fi
> > +
> >      if test ! -e /proxmox_install_mode; then
> >          # modeled after code generated by dh_start
> >          for unit in ${UNITS}; do
> > -- 
> > 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 v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

[Max Carrara]

On Tue Mar 19, 2024 at 11:04 AM CET, Fabian Grünbichler wrote:
> On March 5, 2024 4:07 pm, Max Carrara wrote:
> > Due to Ceph dropping privileges when running the 'ceph-crash' daemon
> > [0], it is necessary to allow the daemon to authenticate with its
> > cluster in a safe manner.
> > 
> > In order to avoid exposing sensitive keyrings or somehow escalating
> > its privileges again, 'ceph-crash' is therefore provided with its own
> > keyring in the '/etc/pve/ceph' directory. This directory, due to being
> > on 'pmxcfs', may be read by members of the 'www-data' group, which
> > 'ceph-crash' is made part of [1].
> > 
> > 
> > Expected Configuration
> > ----------------------
> > 
> >  1. A keyring file named '/etc/pve/ceph/ceph.client.crash.keyring'
> >     exists
> >  2. A section named 'client.crash' exists in '/etc/pve/ceph.conf'
> >  3. The 'client.crash' section has a key named 'keyring' which
> >     references the keyring file as '/etc/pve/ceph/$cluster.$name.keyring'
> >  4. The 'client.crash' section has *no* key named 'key'
> > 
> > 
> > New Clusters
> > ------------
> > 
> > The keyring file is created and the conf file is updated after the first
> > monitor has been created (when calling `pveceph mon create`).
> > 
> > 
> > Existing Clusters
> > -----------------
> > 
> > A new helper script creates and configures the 'client.crash' keyring in
> > `postinst`, if:
> >  * Ceph is installed
> >  * Ceph is initialized ('/etc/pve/ceph.conf' and '/etc/pve/ceph' exist)
> >  * Connection to RADOS is successful
> > 
> > If the above conditions are met, the helper script ensures that the
> > existing configuration matches the expected configuration mentioned
> > above.
> > 
> > The configuration is not changed if it is already as expected.
> > 
> > The helper script may be called again manually if the `postinst` hook
> > fails. It is installed to '/usr/share/pve-manager/helpers/pve-init-ceph-crash'.
> > 
> > 
> > Existing `client.crash` Key
> > ---------------------------
> > 
> > If a key named 'client.crash' already exists within the cluster, it is
> > reused and not regenerated. 
> > 
> > 
> > [0]: https://github.com/ceph/ceph/pull/48713
> > [1]: 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
> > Changes v2 --> v3:
> >   * avoid needless second 'pmxcfs' lock on 'ceph.conf' when configuring
> >     'client.crash' keyring after first monitor was created
> >   * change name of helper sub that creates the ceph.crash keyring
> >   * the helper sub now implicitly expects the /etc/pve/ceph directory to
> >     exist (as the previous commit enforces its existence)
> >   * remove BASH part of postinst hook related to `ceph-crash`
> >   * add Perl helper script and call that instead in postinst hook
> >   * reword commit message
> > Changes v3 --> v4:
> >   * ensure key named 'key' does not exist for 'client.crash' section
> >     in helper script (thanks Friedrich!)
> >   * restart ceph-crash.service if it wasn't disabled (thanks Friedrich!)
> >   * add empty line before and after making changes in `update_ceph_conf`
> >     in 'postinst' hook
> >   * clean up the helper script; logic is easier to follow now
> >   * increase verbosity of output of helper script, showing what's
> >     exactly done when invoked
> > 
> >  PVE/API2/Ceph/MON.pm    |   8 +++
> >  PVE/Ceph/Tools.pm       |  35 +++++++++++
> >  bin/Makefile            |   1 +
> >  bin/pve-init-ceph-crash | 129 ++++++++++++++++++++++++++++++++++++++++
> >  debian/postinst         |  14 +++++
> >  5 files changed, 187 insertions(+)
> >  create mode 100755 bin/pve-init-ceph-crash
> > 
> > diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> > index 1e959ef3..8b7ce376 100644
> > --- a/PVE/API2/Ceph/MON.pm
> > +++ b/PVE/API2/Ceph/MON.pm
> > @@ -450,6 +450,14 @@ __PACKAGE__->register_method ({
> >  			)
> >  		    };
> >  		    warn "$@" if $@;
> > +
> > +		    print "Configuring keyring for ceph-crash.service\n";
> > +		    eval {
> > +			PVE::Ceph::Tools::create_or_update_crash_keyring_file();
> > +			$cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
> > +			cfs_write_file('ceph.conf', $cfg);
> > +		    };
> > +		    warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
> >  		}
> >  
> >  		eval { PVE::Ceph::Services::ceph_service_cmd('enable', $monsection) };
> > diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> > index 735bb116..9b97171e 100644
> > --- a/PVE/Ceph/Tools.pm
> > +++ b/PVE/Ceph/Tools.pm
> > @@ -20,6 +20,7 @@ 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";
> > @@ -45,6 +46,7 @@ my $config_values = {
> >  
> >  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,
> > @@ -420,6 +422,39 @@ sub get_or_create_admin_keyring {
> >      return $pve_ckeyring_path;
> >  }
> >  
> > +# is also used in `pve-init-ceph-crash` helper
> > +sub create_or_update_crash_keyring_file {
> > +    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 (-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);
> > +	    return 1;
> > +	}
> > +    } else {
> > +	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> > +	return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  # get ceph-volume managed osds
> >  sub ceph_volume_list {
> >      my $result = {};
> > diff --git a/bin/Makefile b/bin/Makefile
> > index 06d8148e..b221e4b1 100644
> > --- a/bin/Makefile
> > +++ b/bin/Makefile
> > @@ -83,6 +83,7 @@ install: $(SCRIPTS) $(CLI_MANS) $(SERVICE_MANS) $(BASH_COMPLETIONS) $(ZSH_COMPLE
> >  	install -m 0755 $(SCRIPTS) $(BINDIR)
> >  	install -d $(USRSHARE)/helpers
> >  	install -m 0755 pve-startall-delay $(USRSHARE)/helpers
> > +	install -m 0755 pve-init-ceph-crash $(USRSHARE)/helpers
> >  	install -d $(MAN1DIR)
> >  	install -m 0644 $(CLI_MANS) $(MAN1DIR)
> >  	install -d $(MAN8DIR)
> > diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
> > new file mode 100755
> > index 00000000..c8e9217d
> > --- /dev/null
> > +++ b/bin/pve-init-ceph-crash
> > @@ -0,0 +1,129 @@
> > +#!/usr/bin/perl
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use List::Util qw(first);
> > +
> > +use PVE::Ceph::Tools;
> > +use PVE::Cluster;
> > +use PVE::RADOS;
> > +use PVE::RPCEnvironment;
> > +
> > +my $ceph_cfg_file = 'ceph.conf';
> > +my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
> > +
> > +my $entity = 'client.crash';
>
> nit: this could be inlined?

Inlined as in use 'client.crash' as the key directly? Eh, I'm more a fan
of using a variable here, as constantly spelling it out while changing a
bunch of things gets a little painful ...

If you meant that I should put it in `try_adapt_cfg` - sure, I missed
that that's the only `sub` in which it's being used, woops!

>
> > +
> > +
> > +sub try_adapt_cfg {
> > +    my ($cfg) = @_;
> > +
> > +    my $changed = 0;
> > +    print("Checking whether the configuration for '$entity' needs to be updated.\n");
> > +
> > +    my $add_keyring = sub {
> > +	print("Setting keyring path to '$keyring_value'.\n");
> > +	$cfg->{$entity}->{keyring} = $keyring_value;
> > +	$changed = 1;
>
> this can be removed, you always return right after calling this sub..
>
> > +    };
> > +
> > +    if (!exists($cfg->{$entity})) {
> > +	print("Adding missing section for '$entity'.\n");
> > +	$add_keyring->();
> > +	return $changed;
>
> and all of these can be changed to uncondtionally return 1

Fair!

>
> > +    }
> > +
> > +    if (exists($cfg->{$entity}->{key})) {
> > +	print("Removing existing usage of key.\n");
> > +	delete($cfg->{$entity}->{key});
> > +	$changed = 1;
> > +    }
> > +
> > +    if (!exists($cfg->{$entity}->{keyring})) {
> > +	print("Keyring path is missing from configuration.\n");
> > +	$add_keyring->();
> > +	return $changed;
> > +    }
> > +
> > +    my $current_keyring_value = $cfg->{$entity}->{keyring};
> > +    if ($current_keyring_value ne $keyring_value) {
> > +	print("Current keyring path differs from expected path.\n");
> > +	$add_keyring->();
> > +	return $changed;
> > +    }
> > +
> > +    return $changed;
>
> $changed only serves the purpose of returning 1 iff only the 'key'
> value/entry was removed. so it could be renamed to reflect that ;)

Good point.

>
> > +}
> > +
> > +
> > +sub main {
> > +    my $rpcenv = PVE::RPCEnvironment->init('cli');
> > +
> > +    $rpcenv->init_request();
> > +    $rpcenv->set_language($ENV{LANG});
> > +    $rpcenv->set_user('root@pam');
>
> why do we need an rpcenv here?

Double-checked, just to be sure: `librados-perl` requires an
`RPCEnvironment` to do some handling regarding forks -
`RPCEnvironment::get()` will die if no env was initialized.

>
> > +
> > +    die "Not running as root. Exiting.\n" if $> != 0;
> > +
> > +    if (!PVE::Ceph::Tools::check_ceph_installed('ceph_bin', 1)) {
> > +        print("Ceph is not installed. No action required.\n");
> > +        exit 0;
> > +    }
> > +
> > +    eval {
> > +        PVE::Ceph::Tools::check_ceph_inited();
> > +    };
> > +    if ($@) {
> > +        print("Ceph is not initialized. No action required.\n");
> > +        exit 0;
> > +    }
> > +
> > +    my $rados = eval { PVE::RADOS->new() };
> > +    $@ = ''; # warnings are displayed regardless
>
> what's this line for?

Now that you mention it, I think this was a remnant from the previous
series, where the error handling was different. Will most likely remove
this in v5.

>
> > +    my $ceph_crash_key_path = PVE::Ceph::Tools::get_config('pve_ceph_crash_key_path');
> > +
> > +    PVE::Cluster::cfs_lock_file($ceph_cfg_file, undef, sub {
>
> if you let this sub return a boolean value (worked/failed)
>
> > +        my $cfg = PVE::Cluster::cfs_read_file($ceph_cfg_file);
> > +
> > +	if (!defined($rados)) {
> > +	    my $has_mon_config = defined(first { m/mon\..*/ } keys $cfg->%*);
> > +	    if ($has_mon_config) {
> > +		die "Connection to RADOS failed even though a monitor is configured.\n" .
> > +		    "Please verify whether your configuration is correct.\n"
> > +	    }
> > +
> > +	    print(
> > +		"Connection to RADOS failed and no monitor is configured. ".
> > +		"Assume that things are fine. No action required.\n"
> > +	    );
>
> not sure if it wouldn't be better to check mon_host here? that's the
> thing actually needed to contact the mon, everything else is optional..
> and even that could be moved to DNS, but we don't support that for
> PVE-managed clusters..

Hmm... I'll see what I can do / how it behaves.

>
> > +	    return;
> > +	}
> > +
> > +	my $updated_keyring = PVE::Ceph::Tools::create_or_update_crash_keyring_file($rados);
> > +
> > +	if ($updated_keyring) {
> > +	    print("Keyring file '$ceph_crash_key_path' was updated.\n");
> > +	}
> > +
> > +	my $changed = try_adapt_cfg($cfg);
> > +
> > +	if ($changed) {
> > +	    print("Committing updated configuration.\n");
> > +	    PVE::Cluster::cfs_write_file($ceph_cfg_file, $cfg);
> > +	    print("Successfully updated configuration for 'ceph-crash.service'.\n");
> > +	} else {
> > +	    print("Configuration does not need to be updated.\n");
> > +	}
> > +    });
>
> then you can just check the return value (undef means cfs_lock set an
> error, true means it worked, false means it failed without dieing, or
> whatever you want to return)

Thanks for the suggestion, will incorporate this in v5!

>
> > +    if ($@) {
> > +	warn("Failed to configure keyring for 'ceph-crash.service'. Error:\n");
>
> nit: I think I'd prefer the "Error: to be on the next line with the error message.."

ACK ;)

>
> > +	warn("$@");
> > +	exit 1;
> > +    }
> > +}
> > +
> > +main();
> > +
> > +0;
>
> why?

Not sure anymore... my bad! I think this was part of some module testing
that I was doing (where it's a convention to put a `1;` at the end of
the file to confirm that the module loaded successfully). Regardless,
this shouldn't be here, so will remove it in v5.

>
> > diff --git a/debian/postinst b/debian/postinst
> > index 61b50f97..c36afa13 100755
> > --- a/debian/postinst
> > +++ b/debian/postinst
> > @@ -82,10 +82,24 @@ EOF
> >  
> >  update_ceph_conf() {
> >      CEPH_CONF_DIR='/etc/pve/ceph'
> > +    UNIT='ceph-crash.service'
> > +
> > +    echo ""
> >  
> >      if test ! -d "${CEPH_CONF_DIR}"; then
> >          mkdir -p "${CEPH_CONF_DIR}"
> >      fi
> > +
> > +    # Don't fail in case user has "exotic" configuration where RADOS
> > +    # isn't available on all nodes for some reason
> > +    /usr/share/pve-manager/helpers/pve-init-ceph-crash || true
> > +
> > +    if systemctl -q is-enabled "$UNIT"; then
>
> like Friedrich said, please silence this (`2>/dev/null`)

ACK!

>
> > +        echo "Restarting '$UNIT'"
> > +        deb-systemd-invoke restart "$UNIT"
>
> || true please!

ACK! .. and thanks for the thorough review!

>
> > +    fi
> > +
> > +    echo ""
> >  }
> >  
> >  migrate_apt_auth_conf() {
> > -- 
> > 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 v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

[Fabian Grünbichler]

> Max Carrara <m.carrara@proxmox.com> hat am 19.03.2024 18:41 CET geschrieben:
> On Tue Mar 19, 2024 at 11:04 AM CET, Fabian Grünbichler wrote:
> > On March 5, 2024 4:07 pm, Max Carrara wrote:
> > > diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
> > > new file mode 100755
> > > index 00000000..c8e9217d
> > > --- /dev/null
> > > +++ b/bin/pve-init-ceph-crash
> > > @@ -0,0 +1,129 @@
> > > +#!/usr/bin/perl
> > > +
> > > +use strict;
> > > +use warnings;
> > > +
> > > +use List::Util qw(first);
> > > +
> > > +use PVE::Ceph::Tools;
> > > +use PVE::Cluster;
> > > +use PVE::RADOS;
> > > +use PVE::RPCEnvironment;
> > > +
> > > +my $ceph_cfg_file = 'ceph.conf';
> > > +my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
> > > +
> > > +my $entity = 'client.crash';
> >
> > nit: this could be inlined?
> 
> Inlined as in use 'client.crash' as the key directly? Eh, I'm more a fan
> of using a variable here, as constantly spelling it out while changing a
> bunch of things gets a little painful ...
> 
> If you meant that I should put it in `try_adapt_cfg` - sure, I missed
> that that's the only `sub` in which it's being used, woops!

both would be fine for me :)

> > > +
> > > +

[..]

> > > +
> > > +sub main {
> > > +    my $rpcenv = PVE::RPCEnvironment->init('cli');
> > > +
> > > +    $rpcenv->init_request();
> > > +    $rpcenv->set_language($ENV{LANG});
> > > +    $rpcenv->set_user('root@pam');
> >
> > why do we need an rpcenv here?
> 
> Double-checked, just to be sure: `librados-perl` requires an
> `RPCEnvironment` to do some handling regarding forks -
> `RPCEnvironment::get()` will die if no env was initialized.

good to know. maybe a comment makes sense - the rpcenv is not used by anything else here it seems, and the fact that librados uses it because it forks itself and wants to clean up in case it is called in an API server process is not obvious ;)

you could replace those lines with a call to setup_default_cli_env that does all of the above and the "am I root" check in one ;)

Re: [pve-devel] [PATCH v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

[Max Carrara]

On Wed Mar 20, 2024 at 9:05 AM CET, Fabian Grünbichler wrote:
> > Max Carrara <m.carrara@proxmox.com> hat am 19.03.2024 18:41 CET geschrieben:
> > On Tue Mar 19, 2024 at 11:04 AM CET, Fabian Grünbichler wrote:
> > > On March 5, 2024 4:07 pm, Max Carrara wrote:
> > > > diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
> > > > new file mode 100755
> > > > index 00000000..c8e9217d
> > > > --- /dev/null
> > > > +++ b/bin/pve-init-ceph-crash
> > > > @@ -0,0 +1,129 @@
> > > > +#!/usr/bin/perl
> > > > +
> > > > +use strict;
> > > > +use warnings;
> > > > +
> > > > +use List::Util qw(first);
> > > > +
> > > > +use PVE::Ceph::Tools;
> > > > +use PVE::Cluster;
> > > > +use PVE::RADOS;
> > > > +use PVE::RPCEnvironment;
> > > > +
> > > > +my $ceph_cfg_file = 'ceph.conf';
> > > > +my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
> > > > +
> > > > +my $entity = 'client.crash';
> > >
> > > nit: this could be inlined?
> > 
> > Inlined as in use 'client.crash' as the key directly? Eh, I'm more a fan
> > of using a variable here, as constantly spelling it out while changing a
> > bunch of things gets a little painful ...
> > 
> > If you meant that I should put it in `try_adapt_cfg` - sure, I missed
> > that that's the only `sub` in which it's being used, woops!
>
> both would be fine for me :)

ACK!

>
> > > > +
> > > > +
>
> [..]
>
> > > > +
> > > > +sub main {
> > > > +    my $rpcenv = PVE::RPCEnvironment->init('cli');
> > > > +
> > > > +    $rpcenv->init_request();
> > > > +    $rpcenv->set_language($ENV{LANG});
> > > > +    $rpcenv->set_user('root@pam');
> > >
> > > why do we need an rpcenv here?
> > 
> > Double-checked, just to be sure: `librados-perl` requires an
> > `RPCEnvironment` to do some handling regarding forks -
> > `RPCEnvironment::get()` will die if no env was initialized.
>
> good to know. maybe a comment makes sense - the rpcenv is not used by anything else here it seems, and the fact that librados uses it because it forks itself and wants to clean up in case it is called in an API server process is not obvious ;)
>
> you could replace those lines with a call to setup_default_cli_env that does all of the above and the "am I root" check in one ;)

Also ACK! Thanks again for your input!

Re: [pve-devel] [PATCH v4 pve-storage 06/16] cephconfig: support line-continuations in parser

[Max Carrara]

On Tue Mar 19, 2024 at 4:59 PM CET, Max Carrara wrote:
> On Tue Mar 19, 2024 at 10:37 AM CET, Fabian Grünbichler wrote:
> > On March 5, 2024 4:07 pm, Max Carrara wrote:
> > > Ceph's docs state the following [0]:
> > >> The backslash character `\` is used as the line-continuation marker
> > >> that combines the next line with the current one.
> > > 
> > > This commit implements the support of such line-continuations in our
> > > parser.
> > > 
> > > The line following a line ending with a '\' has its whitespace
> > > preserved, which matches the behaviour in Ceph's original
> > > implementation [1]. In other words, leading and trailing whitespace is
> > > not stripped from a continued line.
> >
> > it's actually a bit more complicated.. ceph only supports line
> > continuations inside values (well, in key value lines after the key ;)),
> > and only if they are unquoted..

Upon further research and confirming the behaviour via `ceph-conf`
(thanks for the tip btw!) line continuations are in fact supported in
different parts as well.

Consider the following example 'ceph.conf' file:
```
[clie\
nt]      # some comment
foo\
\
\
\
= \
bar
```

The continued `client` section header does actually get parsed by
`ceph-conf` without any issues - the trailing comment and whitespace
are also ignored.

Where it gets really interesting is the continuation right after 'foo':
Because keys are defined using `raw[]` [0], whatever is skipped by the
parser is still included in the parsed output [1].

This has the consequence that the four continued lines are in fact not
skipped and instead read as literal newline characters.

After the equals sign, the line continuation is skipped as expected.

By providing literal newlines via the shell, the above can easily be
verified:

$ ceph-conf -c ceph_cancer.conf -s client foo^M^M^M^M
bar

(The ^M is a literal newline and can usually be obtained by typing
CTRL+V, Enter in your shell.)

To make matters even worse, quoted values may in fact be *directly*
followed by continuations (`ceph-conf` fails otherwise):

```
[client]
foo = "bar"\

baz = qux
```

The above is considered "correct" because the escaped newline counts as
whitespace. If you were to put some spaces into the empty line after the
"foo" key, these would be skipped as well.

For completeness's sake, this also parses:

```
[client]
foo = "bar"\
   # some comment
baz = qux
```

However, the following is invalid:

```
[client]
foo = "bar"\
baz = qux
```

... because the parser sees:

```
[client]
foo = "bar"baz = qux
```

... which is not allowed, because a quoted value may only be followed
what the grammar defines as "empty_line" [2].

So, this doesn't really make the parsing logic regarding
line continuations any simpler:

  1. Section headers may contain line continuations
  2. Section headers may be followed by whitespace + comments (after ']'
  3. Keys are parsed "raw" and may therefore be continued
     --> Will probably just not handle this case, as there are no config
     keys that contain newline characters or anything of the sort
     - why would there be? Why would a user need this?
  4. Unquoted values may contain line continuations
  5. Quoted values may be *directly* followed by a line continuation
     character, as long as the remaining stuff is whitespace or a
     comment
  6. Bonus point: Quoted values MUST NOT *contain* line continuations,
     as they're parsed as `lexeme[]`s [3]

... so, see you in v5 ;)

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l182
[1]: https://www.boost.org/doc/libs/1_53_0/libs/spirit/doc/html/spirit/qi/reference/directive/raw.html
[2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l188
[3]: https://www.boost.org/doc/libs/1_53_0/libs/spirit/doc/html/spirit/qi/reference/directive/lexeme.html

>
> As mentioned in my other reply, I'll probably have to revise the whole
> parsing logic to take that into account... but thanks for being so
> thorough!
>
> >
> > > 
> > > [0]: https://docs.ceph.com/en/reef/rados/configuration/ceph-conf/#changes-introduced-in-octopus
> > > [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l262
> > > 
> > > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > > ---
> > > Changes v2 --> v3:
> > >   * new
> > > Changes v3 --> v4:
> > >   * none
> > > 
> > >  src/PVE/CephConfig.pm | 28 ++++++++++++++++++++++++----
> > >  1 file changed, 24 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> > > index 74a92eb..80f71b0 100644
> > > --- a/src/PVE/CephConfig.pm
> > > +++ b/src/PVE/CephConfig.pm
> > > @@ -19,13 +19,33 @@ sub parse_ceph_config {
> > >      return $cfg if !defined($raw);
> > >  
> > >      my @lines = split /\n/, $raw;
> > > +    my @lines_normalized;
> > > +
> > > +    my $re_comment_not_escaped = qr/(?<!\\)(#|;).*$/;
> > > +    my $re_leading_ws = qr/^\s+/;
> > > +    my $re_trailing_ws = qr/\s+$/;
> > > +
> > > +    while (scalar(@lines)) {
> > > +	my $line = shift(@lines);
> > > +	$line =~ s/$re_comment_not_escaped//;
> > > +	$line =~ s/$re_leading_ws//;
> > > +	$line =~ s/$re_trailing_ws//;
> > > +	next if !$line;
> > > +
> > > +	# merge lines ending with continuation character '\'
> > > +	while ($line =~ s/\\$//) {
> > > +	    my $next_line = shift(@lines);
> > > +	    $next_line =~ s/$re_comment_not_escaped//;
> > > +	    $next_line =~ s/$re_trailing_ws//;
> > > +	    $line .= $next_line;
> > > +	}
> > > +
> > > +	push(@lines_normalized, $line);
> > > +    }
> > >  
> > >      my $section;
> > >  
> > > -    for my $line (@lines) {
> > > -	$line =~ s/(?<!\\)(#|;).*$//;
> > > -	$line =~ s/^\s+//;
> > > -	$line =~ s/\s+$//;
> > > +    for my $line (@lines_normalized) {
> > >  	next if !$line;
> > >  
> > >  	if ($line =~ m/^\[(.+)\]$/) {
> > > -- 
> > > 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