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

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

[Max Carrara]

Introduction
------------

This series fixes #4759 [0], an issue where Ceph's crash daemon is
unable to post crash logs due to insufficient permissions, through an
adaptation of our `pveceph` CLI as well as an accompanying Debian
postinst hook.

In essence, this series ensures that the crash daemon can authenticate
with its Ceph cluster without requiring elevated privileges. 

For this to work, the following conditions required:
  1.  A key named 'client.crash' must be stored in the Ceph cluster
      itself
  2.  The key must be saved to a '.keyring' file which can be read by
      the `ceph` user (in order to authenticate with the cluster)
  3.  A reference to the '.keyring' file's location must be provided in
      a 'client.crash' section within the '/etc/pve/ceph.conf' file


Implementation
--------------

When creating a cluster's first monitor via `pveceph create mon`, the
'client.crash' key is automatically generated and saved to
'/etc/pve/ceph/ceph.client.crash.keyring'. This file is then referenced
via the new '[client.crash]' section in '/etc/pve/ceph.conf'.

To allow the crash daemon to actually send its crash logs to the
cluster, a postinst hook for both Ceph Reef and Ceph Quincy is provided
respectively in patches 1 and 2.

In order to support the new '[client.crash]' section within our tooling,
the writer for '/etc/pve/ceph.conf' is updated in patch 3.

Furthermore, the 'keyring' file's directory, '/etc/pve/ceph/', is added
for future non-sensitive configuration files regarding Ceph which the
`ceph` user should be allowed to read without requiring elevated
privileges (and to avoid clutter in '/etc/pve/').


Updating Existing Clusters' Configuration
-----------------------------------------

Existing clusters' configuration is adapted via a Debian postinst hook
added in patch 8. This hook ensures that every existing cluster's
configuration follows the methodolody introduced in the previous
section.

Most importantly, the hook does not generate a new key if one is
already known to Ceph. However, it will still ensure that the key is
saved to '/etc/pve/ceph/ceph.client.crash.keyring' and referenced
accordingly in '/etc/pve/ceph.conf'.

The hook will also not alter any files if the cluster's configuration
already meets the required criteria.


Testing
-------

The CLI as well as the Debian postinst hook have both been thoroughly
tested by going through several scenarios that might exist in the wild.
The postinst hook specifically accounts for:
  * Ceph not being installed or configured
  * Connection to RADOS failing
  * An already existing 'client.crash' key in Ceph
  * An already existing '/etc/pve/ceph/ceph.client.crash.keyring' file
    with expected or unexpected contents
  * A missing '[client.crash]' section in '/etc/pve/ceph.conf'
  * A '[client.crash]' section in '/etc/pve/ceph.conf' which doesn't
    reference any key or references a different key


[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=4759



ceph (master):

Max Carrara (1):
  debian: add patch to fix ceph crash dir permissions in postinst hook

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


ceph (quincy-stable-8):

Max Carrara (1):
  debian: add patch to fix ceph crash dir permissions in postinst hook

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


pve-storage:

Max Carrara (1):
  cephconfig: support sections in the format of [client.$NAME]

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


pve-manager:

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

 PVE/API2/Ceph.pm     |   5 ++
 PVE/API2/Ceph/MON.pm |  28 ++++++++++-
 PVE/Ceph/Services.pm |  12 ++++-
 PVE/Ceph/Tools.pm    |  92 ++++++++++++++++++++++++++++++-----
 debian/postinst      | 111 ++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 232 insertions(+), 16 deletions(-)

-- 
2.39.2

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

[Max Carrara]

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

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

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 ...rmissions-of-subdirectories-of-var-l.patch | 42 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch

diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
new file mode 100644
index 000000000..951a2a6ed
--- /dev/null
+++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
@@ -0,0 +1,42 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 11 Jan 2024 14:04:16 +0100
+Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by also updating the permissions of
+/var/lib/ceph/*/* - the subdirectories of the directories in
+/var/lib/ceph.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 8 ++++++++
+ 1 file changed, 8 insertions(+)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..7ca0b9b6c43 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -40,6 +40,14 @@ case "$1" in
+ 		chown $SERVER_USER:$SERVER_GROUP $DIR
+ 	    fi
+ 	done
++
++	# also adjust file and directory permissons for subdirectories
++	for SUBDIR in /var/lib/ceph/*/* ; do
++	    if ! dpkg-statoverride --list $SUBDIR >/dev/null
++	    then
++		chown $SERVER_USER:$SERVER_GROUP $SUBDIR
++	    fi
++	done
+     ;;
+     abort-upgrade|abort-remove|abort-deconfigure)
+ 	:
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 865caf23d..cf8f1ea31 100644
--- a/patches/series
+++ b/patches/series
@@ -12,3 +12,4 @@
 0012-backport-mgr-dashboard-simplify-authentication-proto.patch
 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
+0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
-- 
2.39.2

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

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
> 
> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  ...rmissions-of-subdirectories-of-var-l.patch | 42 +++++++++++++++++++
>  patches/series                                |  1 +
>  2 files changed, 43 insertions(+)
>  create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> 
> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> new file mode 100644
> index 000000000..951a2a6ed
> --- /dev/null
> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> @@ -0,0 +1,42 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Thu, 11 Jan 2024 14:04:16 +0100
> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
> +
> +A rather recent PR made ceph-crash run as "ceph" user instead of
> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
> +ceph-crash cannot actually post any crash logs now.
> +
> +This commit fixes this by also updating the permissions of
> +/var/lib/ceph/*/* - the subdirectories of the directories in
> +/var/lib/ceph.
> +
> +[0]: https://github.com/ceph/ceph/pull/48713
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + debian/ceph-base.postinst | 8 ++++++++
> + 1 file changed, 8 insertions(+)
> +
> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
> +index 75eeb59c624..7ca0b9b6c43 100644
> +--- a/debian/ceph-base.postinst
> ++++ b/debian/ceph-base.postinst
> +@@ -40,6 +40,14 @@ case "$1" in
> + 		chown $SERVER_USER:$SERVER_GROUP $DIR
> + 	    fi
> + 	done
> ++
> ++	# also adjust file and directory permissons for subdirectories
> ++	for SUBDIR in /var/lib/ceph/*/* ; do
> ++	    if ! dpkg-statoverride --list $SUBDIR >/dev/null
> ++	    then
> ++		chown $SERVER_USER:$SERVER_GROUP $SUBDIR
> ++	    fi

this would probably benefit from being merged with the loop above and
being switched to find?

find(utils) is Essential, so its existence is a given..

did you forward this patch upstream? if not, please do so :)

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

[Max Carrara]

On 1/31/24 14:18, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
>> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
>>
>> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  ...rmissions-of-subdirectories-of-var-l.patch | 42 +++++++++++++++++++
>>  patches/series                                |  1 +
>>  2 files changed, 43 insertions(+)
>>  create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>>
>> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> new file mode 100644
>> index 000000000..951a2a6ed
>> --- /dev/null
>> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> @@ -0,0 +1,42 @@
>> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
>> +From: Max Carrara <m.carrara@proxmox.com>
>> +Date: Thu, 11 Jan 2024 14:04:16 +0100
>> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
>> +
>> +A rather recent PR made ceph-crash run as "ceph" user instead of
>> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
>> +ceph-crash cannot actually post any crash logs now.
>> +
>> +This commit fixes this by also updating the permissions of
>> +/var/lib/ceph/*/* - the subdirectories of the directories in
>> +/var/lib/ceph.
>> +
>> +[0]: https://github.com/ceph/ceph/pull/48713
>> +
>> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> +---
>> + debian/ceph-base.postinst | 8 ++++++++
>> + 1 file changed, 8 insertions(+)
>> +
>> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
>> +index 75eeb59c624..7ca0b9b6c43 100644
>> +--- a/debian/ceph-base.postinst
>> ++++ b/debian/ceph-base.postinst
>> +@@ -40,6 +40,14 @@ case "$1" in
>> + 		chown $SERVER_USER:$SERVER_GROUP $DIR
>> + 	    fi
>> + 	done
>> ++
>> ++	# also adjust file and directory permissons for subdirectories
>> ++	for SUBDIR in /var/lib/ceph/*/* ; do
>> ++	    if ! dpkg-statoverride --list $SUBDIR >/dev/null
>> ++	    then
>> ++		chown $SERVER_USER:$SERVER_GROUP $SUBDIR
>> ++	    fi
> 
> this would probably benefit from being merged with the loop above and
> being switched to find?
> 
> find(utils) is Essential, so its existence is a given..

Good point, seems sensible!

> 
> did you forward this patch upstream? if not, please do so :)

Will merge the two loops and use `find` instead and then also see
if I can supply a PR upstream (was initially planning to do that
once this patch is applied on our side).

> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

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

[Max Carrara]

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

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

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 ...rmissions-of-subdirectories-of-var-l.patch | 42 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch

diff --git a/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
new file mode 100644
index 000000000..951a2a6ed
--- /dev/null
+++ b/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
@@ -0,0 +1,42 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 11 Jan 2024 14:04:16 +0100
+Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by also updating the permissions of
+/var/lib/ceph/*/* - the subdirectories of the directories in
+/var/lib/ceph.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 8 ++++++++
+ 1 file changed, 8 insertions(+)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..7ca0b9b6c43 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -40,6 +40,14 @@ case "$1" in
+ 		chown $SERVER_USER:$SERVER_GROUP $DIR
+ 	    fi
+ 	done
++
++	# also adjust file and directory permissons for subdirectories
++	for SUBDIR in /var/lib/ceph/*/* ; do
++	    if ! dpkg-statoverride --list $SUBDIR >/dev/null
++	    then
++		chown $SERVER_USER:$SERVER_GROUP $SUBDIR
++	    fi
++	done
+     ;;
+     abort-upgrade|abort-remove|abort-deconfigure)
+ 	:
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 73f66396c..f549e0837 100644
--- a/patches/series
+++ b/patches/series
@@ -15,3 +15,4 @@
 0020-fix-4759-run-ceph-crash-daemon-with-www-data-group-f.patch
 0021-debian-rules-fix-buildtype.patch
 0022-rocksb-inherit-parent-cmake-cxx-flags.patch
+0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
-- 
2.39.2

[pve-devel] [PATCH pve-storage 3/8] cephconfig: support sections in the format of [client.$NAME]

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/CephConfig.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b10d46..46b92ea 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -77,6 +77,7 @@ sub write_ceph_config {
 
     &$cond_write_sec('global');
     &$cond_write_sec('client');
+    &$cond_write_sec('client\..*');
 
     &$cond_write_sec('mds');
     &$cond_write_sec('mon');
-- 
2.39.2

Re: [pve-devel] [PATCH pve-storage 3/8] cephconfig: support sections in the format of [client.$NAME]

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/CephConfig.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 6b10d46..46b92ea 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -77,6 +77,7 @@ sub write_ceph_config {
>  
>      &$cond_write_sec('global');
>      &$cond_write_sec('client');
> +    &$cond_write_sec('client\..*');
>  
>      &$cond_write_sec('mds');
>      &$cond_write_sec('mon');

this whole code is a bit weird (pre-existing, not your patch in
particular)..

should we maybe switch it to
- keep track of sections which were already written
- write out all not-yet-written sections as a last step?

else, a RMW cycle might lose config sections just because this code is
not aware of them? while we're at it, double-checking how the ceph
parser handles sections with whitespace in their name and other funny
business might be a good idea, just to prevent any discrepancy between
our parser and theirs..

Re: [pve-devel] [PATCH pve-storage 3/8] cephconfig: support sections in the format of [client.$NAME]

[Max Carrara]

On 1/31/24 14:18, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  src/PVE/CephConfig.pm | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..46b92ea 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -77,6 +77,7 @@ sub write_ceph_config {
>>  
>>      &$cond_write_sec('global');
>>      &$cond_write_sec('client');
>> +    &$cond_write_sec('client\..*');
>>  
>>      &$cond_write_sec('mds');
>>      &$cond_write_sec('mon');
> 
> this whole code is a bit weird (pre-existing, not your patch in
> particular)..
> 
> should we maybe switch it to
> - keep track of sections which were already written
> - write out all not-yet-written sections as a last step?

I agree that it's somewhat strange - I initially stumbled across it *after*
finishing patch 5 and I discovered that it wouldn't write the 'client.crash'
section at all. I figured we might only allow certain sections in that case.

`ceph_parse_config` parses the *whole* config anyway, so I don't see why
we couldn't just also support writing "arbitrary" sections.

> 
> else, a RMW cycle might lose config sections just because this code is
> not aware of them? 

That is indeed what happened, but as stated above, I had assumed we only
allow specific sections.

while we're at it, double-checking how the ceph
> parser handles sections with whitespace in their name and other funny
> business might be a good idea, just to prevent any discrepancy between
> our parser and theirs..

Will check, thanks!

Overall, I'll adapt this in v2 to support writing arbitrary sections while
also checking whether it's in line with the way Ceph handles things.

> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

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

[Max Carrara]

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

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/Ceph/Tools.pm | 54 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index a1458b40..3acef11b 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -36,15 +36,42 @@ my $ceph_service = {
 };
 
 my $config_hash = {
-    ccname => $ccname,
-    pve_ceph_cfgpath => $pve_ceph_cfgpath,
-    pve_mon_key_path => $pve_mon_key_path,
-    pve_ckeyring_path => $pve_ckeyring_path,
-    ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
-    ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
-    ceph_mds_data_dir => $ceph_mds_data_dir,
-    long_rados_timeout => 60,
-    ceph_cfgpath => $ceph_cfgpath,
+    ccname => {
+	value => $ccname,
+	is_file => 0,
+    },
+    pve_ceph_cfgpath => {
+	value => $pve_ceph_cfgpath,
+	is_file => 1,
+    },
+    pve_mon_key_path => {
+	value => $pve_mon_key_path,
+	is_file => 1,
+    },
+    pve_ckeyring_path => {
+	value => $pve_ckeyring_path,
+	is_file => 1,
+    },
+    ceph_bootstrap_osd_keyring => {
+	value => $ceph_bootstrap_osd_keyring,
+	is_file => 1,
+    },
+    ceph_bootstrap_mds_keyring => {
+	value => $ceph_bootstrap_mds_keyring,
+	is_file => 1,
+    },
+    ceph_mds_data_dir => {
+	value => $ceph_mds_data_dir,
+	is_file => 0,
+    },
+    long_rados_timeout => {
+	value => 60,
+	is_file => 0,
+    },
+    ceph_cfgpath => {
+	value => $ceph_cfgpath,
+	is_file => 1,
+    },
 };
 
 sub get_local_version {
@@ -84,7 +111,7 @@ sub get_cluster_versions {
 sub get_config {
     my $key = shift;
 
-    my $value = $config_hash->{$key};
+    my $value = $config_hash->{$key}->{value};
 
     die "no such ceph config '$key'" if !$value;
 
@@ -123,8 +150,11 @@ sub purge_all_ceph_files {
 	warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
     } else {
 	print "Removing config & keyring files\n";
-	foreach my $file (%$config_hash) {
-	    unlink $file if (-e $file);
+	for my $conf_value (values %$config_hash) {
+	    if ($conf_value->{is_file}) {
+		my $file = $conf_value->{value};
+		unlink $file if (-e $file);
+	    }
 	}
     }
 }
-- 
2.39.2

Re: [pve-devel] [PATCH pve-manager 4/8] ceph: fix edge case of wrong files being deleted on purge

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> Having a file named e.g. "60" in your current directory will cause it
> to be deleted when executind `pveceph purge`. This commit fixes that
> by making the config hash differ between which values represent file
> paths and which don't.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  PVE/Ceph/Tools.pm | 54 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index a1458b40..3acef11b 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -36,15 +36,42 @@ my $ceph_service = {
>  };
>  
>  my $config_hash = {
> -    ccname => $ccname,
> -    pve_ceph_cfgpath => $pve_ceph_cfgpath,
> -    pve_mon_key_path => $pve_mon_key_path,
> -    pve_ckeyring_path => $pve_ckeyring_path,
> -    ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
> -    ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
> -    ceph_mds_data_dir => $ceph_mds_data_dir,
> -    long_rados_timeout => 60,
> -    ceph_cfgpath => $ceph_cfgpath,
> +    ccname => {
> +	value => $ccname,
> +	is_file => 0,
> +    },
> +    pve_ceph_cfgpath => {
> +	value => $pve_ceph_cfgpath,
> +	is_file => 1,
> +    },
> +    pve_mon_key_path => {
> +	value => $pve_mon_key_path,
> +	is_file => 1,
> +    },
> +    pve_ckeyring_path => {
> +	value => $pve_ckeyring_path,
> +	is_file => 1,
> +    },
> +    ceph_bootstrap_osd_keyring => {
> +	value => $ceph_bootstrap_osd_keyring,
> +	is_file => 1,
> +    },
> +    ceph_bootstrap_mds_keyring => {
> +	value => $ceph_bootstrap_mds_keyring,
> +	is_file => 1,
> +    },
> +    ceph_mds_data_dir => {
> +	value => $ceph_mds_data_dir,
> +	is_file => 0,
> +    },
> +    long_rados_timeout => {
> +	value => 60,
> +	is_file => 0,
> +    },
> +    ceph_cfgpath => {
> +	value => $ceph_cfgpath,
> +	is_file => 1,
> +    },
>  };

this would be less verbose if the hash is just split into two, with
get_config using both, and the removal using the one just containing
files.

>  
>  sub get_local_version {
> @@ -84,7 +111,7 @@ sub get_cluster_versions {
>  sub get_config {
>      my $key = shift;
>  
> -    my $value = $config_hash->{$key};
> +    my $value = $config_hash->{$key}->{value};
>  
>      die "no such ceph config '$key'" if !$value;
>  
> @@ -123,8 +150,11 @@ sub purge_all_ceph_files {
>  	warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
>      } else {
>  	print "Removing config & keyring files\n";
> -	foreach my $file (%$config_hash) {
> -	    unlink $file if (-e $file);
> +	for my $conf_value (values %$config_hash) {
> +	    if ($conf_value->{is_file}) {
> +		my $file = $conf_value->{value};
> +		unlink $file if (-e $file);
> +	    }
>  	}
>      }
>  }
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH pve-manager 4/8] ceph: fix edge case of wrong files being deleted on purge

[Max Carrara]

On 1/31/24 14:18, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> Having a file named e.g. "60" in your current directory will cause it
>> to be deleted when executind `pveceph purge`. This commit fixes that
>> by making the config hash differ between which values represent file
>> paths and which don't.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  PVE/Ceph/Tools.pm | 54 ++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 42 insertions(+), 12 deletions(-)
>>
>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>> index a1458b40..3acef11b 100644
>> --- a/PVE/Ceph/Tools.pm
>> +++ b/PVE/Ceph/Tools.pm
>> @@ -36,15 +36,42 @@ my $ceph_service = {
>>  };
>>  
>>  my $config_hash = {
>> -    ccname => $ccname,
>> -    pve_ceph_cfgpath => $pve_ceph_cfgpath,
>> -    pve_mon_key_path => $pve_mon_key_path,
>> -    pve_ckeyring_path => $pve_ckeyring_path,
>> -    ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
>> -    ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
>> -    ceph_mds_data_dir => $ceph_mds_data_dir,
>> -    long_rados_timeout => 60,
>> -    ceph_cfgpath => $ceph_cfgpath,
>> +    ccname => {
>> +	value => $ccname,
>> +	is_file => 0,
>> +    },
>> +    pve_ceph_cfgpath => {
>> +	value => $pve_ceph_cfgpath,
>> +	is_file => 1,
>> +    },
>> +    pve_mon_key_path => {
>> +	value => $pve_mon_key_path,
>> +	is_file => 1,
>> +    },
>> +    pve_ckeyring_path => {
>> +	value => $pve_ckeyring_path,
>> +	is_file => 1,
>> +    },
>> +    ceph_bootstrap_osd_keyring => {
>> +	value => $ceph_bootstrap_osd_keyring,
>> +	is_file => 1,
>> +    },
>> +    ceph_bootstrap_mds_keyring => {
>> +	value => $ceph_bootstrap_mds_keyring,
>> +	is_file => 1,
>> +    },
>> +    ceph_mds_data_dir => {
>> +	value => $ceph_mds_data_dir,
>> +	is_file => 0,
>> +    },
>> +    long_rados_timeout => {
>> +	value => 60,
>> +	is_file => 0,
>> +    },
>> +    ceph_cfgpath => {
>> +	value => $ceph_cfgpath,
>> +	is_file => 1,
>> +    },
>>  };
> 
> this would be less verbose if the hash is just split into two, with
> get_config using both, and the removal using the one just containing
> files.

Hmm, you really think so? I would personally prefer to keep it all in
one place, as it makes the surrounding logic a bit easier (at least in
my head). I wouldn't mind splitting it up, but I'm not really sure if
that makes it actually less verbose.

I shan't speculate though and just see what it would look like - will
inform you when I have.

Thanks for your input!

> 
>>  
>>  sub get_local_version {
>> @@ -84,7 +111,7 @@ sub get_cluster_versions {
>>  sub get_config {
>>      my $key = shift;
>>  
>> -    my $value = $config_hash->{$key};
>> +    my $value = $config_hash->{$key}->{value};
>>  
>>      die "no such ceph config '$key'" if !$value;
>>  
>> @@ -123,8 +150,11 @@ sub purge_all_ceph_files {
>>  	warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
>>      } else {
>>  	print "Removing config & keyring files\n";
>> -	foreach my $file (%$config_hash) {
>> -	    unlink $file if (-e $file);
>> +	for my $conf_value (values %$config_hash) {
>> +	    if ($conf_value->{is_file}) {
>> +		my $file = $conf_value->{value};
>> +		unlink $file if (-e $file);
>> +	    }
>>  	}
>>      }
>>  }
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

[pve-devel] [PATCH pve-manager 5/8] fix #4759: ceph: configure keyring for ceph-crash.service

[Max Carrara]

when creating the cluster's first monitor.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/API2/Ceph/MON.pm | 28 +++++++++++++++++++++++++++-
 PVE/Ceph/Services.pm | 12 ++++++++++--
 PVE/Ceph/Tools.pm    | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..8d75f5d1 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -459,11 +459,37 @@ __PACKAGE__->register_method ({
 	    });
 	    die $@ if $@;
 	    # automatically create manager after the first monitor is created
+	    # and set up keyring and config for ceph-crash.service
 	    if ($is_first_monitor) {
 		PVE::API2::Ceph::MGR->createmgr({
 		    node => $param->{node},
 		    id => $param->{node}
-		})
+		});
+
+		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
+		    my $cfg = cfs_read_file('ceph.conf');
+
+		    if ($cfg->{'client.crash'}) {
+			return undef;
+		    }
+
+		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
+
+		    cfs_write_file('ceph.conf', $cfg);
+		});
+		die $@ if $@;
+
+		eval {
+		    PVE::Ceph::Tools::get_or_create_crash_keyring();
+		};
+		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
+
+		print "enabling service 'ceph-crash.service'\n";
+		PVE::Ceph::Services::ceph_service_cmd('enable', 'crash');
+		print "starting service 'ceph-crash.service'\n";
+		# ceph-crash already runs by default,
+		# this makes sure the keyring is used
+		PVE::Ceph::Services::ceph_service_cmd('restart', 'crash');
 	    }
 	};
 
diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
index e0f31e8e..5f5986f9 100644
--- a/PVE/Ceph/Services.pm
+++ b/PVE/Ceph/Services.pm
@@ -100,8 +100,16 @@ sub get_cluster_service {
 sub ceph_service_cmd {
     my ($action, $service) = @_;
 
-    if ($service && $service =~ m/^(mon|osd|mds|mgr|radosgw)(\.(${\SERVICE_REGEX}))?$/) {
-	$service = defined($3) ? "ceph-$1\@$3" : "ceph-$1.target";
+    if ($service) {
+	# specific (parameterized) services or targets
+	if ($service =~ m/^(mon|osd|mds|mgr|radosgw)(\.(${\SERVICE_REGEX}))?$/) {
+	    $service = defined($3) ? "ceph-$1\@$3" : "ceph-$1.target";
+	# other services without targets
+	} elsif ($service =~ m/^(crash)$/) {
+	    $service = "ceph-$1.service";
+	} else {
+	    $service = "ceph.target";
+	}
     } else {
 	$service = "ceph.target";
     }
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 3acef11b..cf9f2ed4 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
 my $ceph_cfgdir = "/etc/ceph";
 my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
 my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
+my $pve_ceph_cfgdir = "/etc/pve/ceph";
 
+my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
 my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
 my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
 my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
@@ -32,6 +34,7 @@ my $ceph_service = {
     ceph_mgr => "/usr/bin/ceph-mgr",
     ceph_osd => "/usr/bin/ceph-osd",
     ceph_mds => "/usr/bin/ceph-mds",
+    ceph_crash => '/usr/bin/ceph-crash',
     ceph_volume => '/usr/sbin/ceph-volume',
 };
 
@@ -44,6 +47,14 @@ my $config_hash = {
 	value => $pve_ceph_cfgpath,
 	is_file => 1,
     },
+    pve_ceph_cfgdir => {
+	value => $pve_ceph_cfgdir,
+	is_file => 0,
+    },
+    pve_ceph_crash_key_path => {
+	value => $pve_ceph_crash_key_path,
+	is_file => 1,
+    },
     pve_mon_key_path => {
 	value => $pve_mon_key_path,
 	is_file => 1,
@@ -439,6 +450,33 @@ sub get_or_create_admin_keyring {
     return $pve_ckeyring_path;
 }
 
+# requires connection to existing monitor
+sub get_or_create_crash_keyring {
+    my ($rados) = @_;
+
+    if (!defined($rados)) {
+	$rados = PVE::RADOS->new();
+    }
+
+    my $output = $rados->mon_command({
+	prefix => 'auth get-or-create',
+	entity => 'client.crash',
+	caps => [
+	    mon => 'profile crash',
+	    mgr => 'profile crash',
+	],
+	format => 'plain',
+    });
+
+    if (! -d $pve_ceph_cfgdir) {
+	mkdir $pve_ceph_cfgdir;
+    }
+
+    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+
+    return $pve_ceph_crash_key_path;
+}
+
 # get ceph-volume managed osds
 sub ceph_volume_list {
     my $result = {};
-- 
2.39.2

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

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> when creating the cluster's first monitor.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  PVE/API2/Ceph/MON.pm | 28 +++++++++++++++++++++++++++-
>  PVE/Ceph/Services.pm | 12 ++++++++++--
>  PVE/Ceph/Tools.pm    | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..8d75f5d1 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -459,11 +459,37 @@ __PACKAGE__->register_method ({
>  	    });
>  	    die $@ if $@;
>  	    # automatically create manager after the first monitor is created
> +	    # and set up keyring and config for ceph-crash.service
>  	    if ($is_first_monitor) {
>  		PVE::API2::Ceph::MGR->createmgr({
>  		    node => $param->{node},
>  		    id => $param->{node}
> -		})
> +		});
> +
> +		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
> +		    my $cfg = cfs_read_file('ceph.conf');
> +
> +		    if ($cfg->{'client.crash'}) {
> +			return undef;
> +		    }
> +
> +		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
> +
> +		    cfs_write_file('ceph.conf', $cfg);
> +		});
> +		die $@ if $@;
> +
> +		eval {
> +		    PVE::Ceph::Tools::get_or_create_crash_keyring();
> +		};
> +		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;

the order here should maybe be switched around? first handle the
keyring, then put it in the config?

> +
> +		print "enabling service 'ceph-crash.service'\n";
> +		PVE::Ceph::Services::ceph_service_cmd('enable', 'crash');

shouldn't this already be handled by default?

> +		print "starting service 'ceph-crash.service'\n";
> +		# ceph-crash already runs by default,
> +		# this makes sure the keyring is used
> +		PVE::Ceph::Services::ceph_service_cmd('restart', 'crash');

this should probably be a try-restart to avoid starting it if the admin
explicitly disabled and/or stopped it..

but - AFAICT, the ceph-crash script that is executed by the service
boils down to (as forked process!) "ceph -n XXX ..." where XXX is (in sequence)
client.crash.$HOST, client.crash, client.admin, so a service restart
shouldn't even be needed, since a fresh ceph (client) process will pick
up the config changes anyway?

>  	    }
>  	};
>  
> diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
> index e0f31e8e..5f5986f9 100644
> --- a/PVE/Ceph/Services.pm
> +++ b/PVE/Ceph/Services.pm

> [..]

> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 3acef11b..cf9f2ed4 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm

> [..]

> +    my $output = $rados->mon_command({
> +	prefix => 'auth get-or-create',
> +	entity => 'client.crash',
> +	caps => [
> +	    mon => 'profile crash',
> +	    mgr => 'profile crash',
> +	],
> +	format => 'plain',
> +    });
> +
> +    if (! -d $pve_ceph_cfgdir) {
> +	mkdir $pve_ceph_cfgdir;
> +    }
> +
> +    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> +
> +    return $pve_ceph_crash_key_path;
> +}
> +

we have another helper for creating a keyring (and another inline call
to ceph-authtool when creating a monitor), should we unify them?

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

[Max Carrara]

On 1/31/24 14:17, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> when creating the cluster's first monitor.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  PVE/API2/Ceph/MON.pm | 28 +++++++++++++++++++++++++++-
>>  PVE/Ceph/Services.pm | 12 ++++++++++--
>>  PVE/Ceph/Tools.pm    | 38 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 75 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>> index 1e959ef3..8d75f5d1 100644
>> --- a/PVE/API2/Ceph/MON.pm
>> +++ b/PVE/API2/Ceph/MON.pm
>> @@ -459,11 +459,37 @@ __PACKAGE__->register_method ({
>>  	    });
>>  	    die $@ if $@;
>>  	    # automatically create manager after the first monitor is created
>> +	    # and set up keyring and config for ceph-crash.service
>>  	    if ($is_first_monitor) {
>>  		PVE::API2::Ceph::MGR->createmgr({
>>  		    node => $param->{node},
>>  		    id => $param->{node}
>> -		})
>> +		});
>> +
>> +		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
>> +		    my $cfg = cfs_read_file('ceph.conf');
>> +
>> +		    if ($cfg->{'client.crash'}) {
>> +			return undef;
>> +		    }
>> +
>> +		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
>> +
>> +		    cfs_write_file('ceph.conf', $cfg);
>> +		});
>> +		die $@ if $@;
>> +
>> +		eval {
>> +		    PVE::Ceph::Tools::get_or_create_crash_keyring();
>> +		};
>> +		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
> 
> the order here should maybe be switched around? first handle the
> keyring, then put it in the config?
> 
>> +
>> +		print "enabling service 'ceph-crash.service'\n";
>> +		PVE::Ceph::Services::ceph_service_cmd('enable', 'crash');
> 
> shouldn't this already be handled by default?
> 
>> +		print "starting service 'ceph-crash.service'\n";
>> +		# ceph-crash already runs by default,
>> +		# this makes sure the keyring is used
>> +		PVE::Ceph::Services::ceph_service_cmd('restart', 'crash');
> 
> this should probably be a try-restart to avoid starting it if the admin
> explicitly disabled and/or stopped it..
> 
> but - AFAICT, the ceph-crash script that is executed by the service
> boils down to (as forked process!) "ceph -n XXX ..." where XXX is (in sequence)
> client.crash.$HOST, client.crash, client.admin, so a service restart
> shouldn't even be needed, since a fresh ceph (client) process will pick
> up the config changes anyway?

Good point - I've found the respective code and will just change the order there
as well as drop the calls here to enable/restart `ceph-crash`. Thanks!

> 
>>  	    }
>>  	};
>>  
>> diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
>> index e0f31e8e..5f5986f9 100644
>> --- a/PVE/Ceph/Services.pm
>> +++ b/PVE/Ceph/Services.pm
> 
>> [..]
> 
>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>> index 3acef11b..cf9f2ed4 100644
>> --- a/PVE/Ceph/Tools.pm
>> +++ b/PVE/Ceph/Tools.pm
> 
>> [..]
> 
>> +    my $output = $rados->mon_command({
>> +	prefix => 'auth get-or-create',
>> +	entity => 'client.crash',
>> +	caps => [
>> +	    mon => 'profile crash',
>> +	    mgr => 'profile crash',
>> +	],
>> +	format => 'plain',
>> +    });
>> +
>> +    if (! -d $pve_ceph_cfgdir) {
>> +	mkdir $pve_ceph_cfgdir;
>> +    }
>> +
>> +    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>> +
>> +    return $pve_ceph_crash_key_path;
>> +}
>> +
> 
> we have another helper for creating a keyring (and another inline call
> to ceph-authtool when creating a monitor), should we unify them?

In this case it's better not to, in my opinion - the function for `ceph-crash`
specifically uses `ceph auth get-or-create` as that's quite a bit easier to use
in this scenario, as the key will automatically be generated if it doesn't exist.
This does require a connection to RADOS, but that will exist once the first mon is
set up anyway.

Otherwise we'd have to use `ceph-authtool` and then also import the key to cephx
if it doesn't exist already, like we do in other places.

Ultimately it ends up achieving the same, but the former just seemed more
straightforward IMO.

I did however notice that there are several `run_command` calls to `ceph-authtool`
floating around that could maaaybe benefit from a helper function, but I would
rather implement that in a different patch series, as that's not really relevant
for this one.

> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

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

[Fabian Grünbichler]

On February 5, 2024 12:57 pm, Max Carrara wrote:
> On 1/31/24 14:17, Fabian Grünbichler wrote:
>> we have another helper for creating a keyring (and another inline call
>> to ceph-authtool when creating a monitor), should we unify them?
> 
> In this case it's better not to, in my opinion - the function for `ceph-crash`
> specifically uses `ceph auth get-or-create` as that's quite a bit easier to use
> in this scenario, as the key will automatically be generated if it doesn't exist.
> This does require a connection to RADOS, but that will exist once the first mon is
> set up anyway.
> 
> Otherwise we'd have to use `ceph-authtool` and then also import the key to cephx
> if it doesn't exist already, like we do in other places.
> 
> Ultimately it ends up achieving the same, but the former just seemed more
> straightforward IMO.
> 
> I did however notice that there are several `run_command` calls to `ceph-authtool`
> floating around that could maaaybe benefit from a helper function, but I would
> rather implement that in a different patch series, as that's not really relevant
> for this one.

saw this too late, disregard that comment in my review of v2 ;) it might
still make sense to have a common helper - after all, we could also
create the mon keys via `ceph auth get-or-create` after the first one..

[pve-devel] [PATCH pve-manager 6/8] ceph: create '/etc/pve/ceph' during `pveceph init`

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/API2/Ceph.pm | 5 +++++
 1 file changed, 5 insertions(+)

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

[pve-devel] [PATCH pve-manager 7/8] debian/postinst: fix shellcheck warning

[Max Carrara]

SC3043 (warning): In POSIX sh, 'local' is undefined.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 debian/postinst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/postinst b/debian/postinst
index 6138ef6d..00d5f2cc 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -9,7 +9,7 @@ set -e
 # installed and configured.
 
 set_lvm_conf() {
-    local FORCE="$1"
+    FORCE="$1"
     LVM_CONF_MARKER="# added by pve-manager to avoid scanning"
 
     # keep user changes afterwards provided marker is still there..
-- 
2.39.2

[pve-devel] applied-partially: [PATCH pve-manager 7/8] debian/postinst: fix shellcheck warning

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> SC3043 (warning): In POSIX sh, 'local' is undefined.

while I get why you sent this here, it's not related at all to this
series, please send such changes on their own and reference them if
needed in the future.

in this case it's actually not needed at all:
https://www.debian.org/doc/debian-policy/ch-files.html#s-scripts

local is pretty much impemented by every shell under the sun, it's just
that POSIX couldn't agree how to handle them when `unset` is called, so
they are not part of POSIX.

> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  debian/postinst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 6138ef6d..00d5f2cc 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -9,7 +9,7 @@ set -e
>  # installed and configured.
>  
>  set_lvm_conf() {
> -    local FORCE="$1"
> +    FORCE="$1"
>      LVM_CONF_MARKER="# added by pve-manager to avoid scanning"
>  
>      # keep user changes afterwards provided marker is still there..
> -- 
> 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] applied-partially: [PATCH pve-manager 7/8] debian/postinst: fix shellcheck warning

[Max Carrara]

On 1/31/24 14:16, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> SC3043 (warning): In POSIX sh, 'local' is undefined.
> 
> while I get why you sent this here, it's not related at all to this
> series, please send such changes on their own and reference them if
> needed in the future.
> 
> in this case it's actually not needed at all:
> https://www.debian.org/doc/debian-policy/ch-files.html#s-scripts
> 
> local is pretty much impemented by every shell under the sun, it's just
> that POSIX couldn't agree how to handle them when `unset` is called, so
> they are not part of POSIX.

Thanks, that's helpful! Will then just drop this patch in v2 and tell
shellcheck to shut up.

> 
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  debian/postinst | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/debian/postinst b/debian/postinst
>> index 6138ef6d..00d5f2cc 100755
>> --- a/debian/postinst
>> +++ b/debian/postinst
>> @@ -9,7 +9,7 @@ set -e
>>  # installed and configured.
>>  
>>  set_lvm_conf() {
>> -    local FORCE="$1"
>> +    FORCE="$1"
>>      LVM_CONF_MARKER="# added by pve-manager to avoid scanning"
>>  
>>      # keep user changes afterwards provided marker is still there..
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

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

[Max Carrara]

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

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

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

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

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

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

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

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 debian/postinst | 109 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

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

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

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> This commit adds the `set_ceph_crash_conf` function, which dynamically
> adapts the host's Ceph configuration in order to allow the Ceph crash
> module's daemon to run without elevated privileges.
> 
> This adaptation is only performed if:
>  * Ceph is installed
>  * Ceph is configured ('/etc/pve/ceph.conf' exists)
>  * Connection to RADOS is successful
> 
> If the above conditions are met, the function will ensure that:
>  * Ceph possesses a key named 'client.crash'
>  * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>  * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>  * The 'client.crash' section has a key named 'keyring' which
>    references '/etc/pve/ceph/ceph.client.crash.keyring'
> 
> Furthermore, if a key named 'client.crash' already exists within the
> cluster, it shall be reused and not regenerated. Also, the
> configuration is not altered if the conditions above are already met.
> 
> This way the keyring file is available as read-only in
> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
> Because the `ceph` user has been made part of said `www-data` group
> [0], it may access the file without requiring any additional
> privileges.
> 
> Thus, the configuration for the Ceph crash daemon is safely adapted as
> expected by PVE tooling and also shared via pmxcfs across one's
> cluster.
> 
> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  debian/postinst | 109 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 00d5f2cc..8d2a8c4b 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -110,6 +110,114 @@ migrate_apt_auth_conf() {
>      fi
>  }
>  
> +set_ceph_crash_conf() {
> +    PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
> +    PVE_CEPH_CONFDIR='/etc/pve/ceph'
> +    PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
> +    PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
> +
> +    # ceph isn't installed -> nothing to do
> +    if ! which ceph > /dev/null 2>&1; then
> +        return 0
> +    fi
> +
> +    # ceph isn't configured -> nothing to do
> +    if test ! -f "${PVE_CEPH_CONFFILE}"; then
> +        return 0
> +    fi
> +
> +    CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
> +
> +    # ceph is installed and possibly configured, but no connection to RADOS
> +    # -> assume no monitor was created, nothing to do
> +    if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
> +        return 0
> +    fi

the stuff after this point basically duplicates a lot of things from
pveceph in shell.. wouldn't it be easier to have a pveceph reinit or
similar command (or a parameter to an existing one) and call that here?

or, for even less coupling (and thus chance of things going wrong and
interrupting the upgrade), include a check somewhere in the ceph status
code path and just add a warning if the key is not configured, with a
hint what command to run/button to click to do the setup?

> +    SECTION_RE='^\[\S+\]$'
> +    CRASH_SECTION_RE='^\[client\.crash\]$'
> +

> [..]

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

[Max Carrara]

On 1/31/24 14:15, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> This commit adds the `set_ceph_crash_conf` function, which dynamically
>> adapts the host's Ceph configuration in order to allow the Ceph crash
>> module's daemon to run without elevated privileges.
>>
>> This adaptation is only performed if:
>>  * Ceph is installed
>>  * Ceph is configured ('/etc/pve/ceph.conf' exists)
>>  * Connection to RADOS is successful
>>
>> If the above conditions are met, the function will ensure that:
>>  * Ceph possesses a key named 'client.crash'
>>  * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>>  * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>>  * The 'client.crash' section has a key named 'keyring' which
>>    references '/etc/pve/ceph/ceph.client.crash.keyring'
>>
>> Furthermore, if a key named 'client.crash' already exists within the
>> cluster, it shall be reused and not regenerated. Also, the
>> configuration is not altered if the conditions above are already met.
>>
>> This way the keyring file is available as read-only in
>> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
>> Because the `ceph` user has been made part of said `www-data` group
>> [0], it may access the file without requiring any additional
>> privileges.
>>
>> Thus, the configuration for the Ceph crash daemon is safely adapted as
>> expected by PVE tooling and also shared via pmxcfs across one's
>> cluster.
>>
>> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  debian/postinst | 109 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 109 insertions(+)
>>
>> diff --git a/debian/postinst b/debian/postinst
>> index 00d5f2cc..8d2a8c4b 100755
>> --- a/debian/postinst
>> +++ b/debian/postinst
>> @@ -110,6 +110,114 @@ migrate_apt_auth_conf() {
>>      fi
>>  }
>>  
>> +set_ceph_crash_conf() {
>> +    PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
>> +    PVE_CEPH_CONFDIR='/etc/pve/ceph'
>> +    PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
>> +    PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
>> +
>> +    # ceph isn't installed -> nothing to do
>> +    if ! which ceph > /dev/null 2>&1; then
>> +        return 0
>> +    fi
>> +
>> +    # ceph isn't configured -> nothing to do
>> +    if test ! -f "${PVE_CEPH_CONFFILE}"; then
>> +        return 0
>> +    fi
>> +
>> +    CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
>> +
>> +    # ceph is installed and possibly configured, but no connection to RADOS
>> +    # -> assume no monitor was created, nothing to do
>> +    if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
>> +        return 0
>> +    fi
> 
> the stuff after this point basically duplicates a lot of things from
> pveceph in shell.. wouldn't it be easier to have a pveceph reinit or
> similar command (or a parameter to an existing one) and call that here?

I wasn't really sure whether tinkering with ceph-crash would warrant a
separate subcommand or not, so I decided to stick with the postinst hook,
as the whole config is set up anyway for new MONs in the future.

Though, perhaps a subcommand that checks whether the entire config is in
place the way we expect it would be nice in general? E.g. whether
'/etc/pve/priv' exists and the '.keyring' files are there (and can be used
to authenticate), '/etc/pve/ceph.conf' contains what we expect, etc.

Places where the config or some files are missing / messed up could then
be spotted much easier and quicker - maybe that would also relieve some
pressure for the support staff.

Something like `pveceph verify-config`, perhaps? Eventually with a `--repair`
flag too, maybe.

Implementing this would possibly also warrant a slight overhaul / refactor
of our Ceph-related perl code, so certain strings, expected conf values, etc.
aren't scattered around as much anymore. It's not too bad IMO, mind you,
but there's no hurt in cleaning some things up along the way.

I would gladly work on that, but probably in another patch series - so I would
prefer to keep the postinst hook around for now and fix any (yet to be) known
issues it has. That would at least fix #4759 quicker and reduce the spam
`ceph-crash` causes in the systemd journal every 10 minutes.

> 
> or, for even less coupling (and thus chance of things going wrong and
> interrupting the upgrade), include a check somewhere in the ceph status
> code path and just add a warning if the key is not configured, with a
> hint what command to run/button to click to do the setup?
> 
>> +    SECTION_RE='^\[\S+\]$'
>> +    CRASH_SECTION_RE='^\[client\.crash\]$'
>> +
> 
>> [..]
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

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

[Fabian Grünbichler]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> Introduction
> ------------
> 
> This series fixes #4759 [0], an issue where Ceph's crash daemon is
> unable to post crash logs due to insufficient permissions, through an
> adaptation of our `pveceph` CLI as well as an accompanying Debian
> postinst hook.
> 
> In essence, this series ensures that the crash daemon can authenticate
> with its Ceph cluster without requiring elevated privileges. 
> 
> For this to work, the following conditions required:
>   1.  A key named 'client.crash' must be stored in the Ceph cluster
>       itself
>   2.  The key must be saved to a '.keyring' file which can be read by
>       the `ceph` user (in order to authenticate with the cluster)
>   3.  A reference to the '.keyring' file's location must be provided in
>       a 'client.crash' section within the '/etc/pve/ceph.conf' file

I like the general direction, it seems sensible. some comments on
individual patches as replies, and some general questions here:

- do we need to store the key on pmxcfs? would it also work to generate
  one on each host and store it locally?
- is there some way to get away without modifying the config? e.g., a
  fallback path for keyrings if there is no "client.XXX" section in the
  config?

  https://docs.ceph.com/en/reef/rados/configuration/auth-config-ref/#keys

  would seem to indicate that the answer to those questions is
  no/yes/yes, but I haven't tested it ;)

  IMHO that would simplify the handling a lot..

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

[Friedrich Weber]

Thanks a lot for tackling this issue!

Gave this a quick spin on a pre-existing 3-node Quincy cluster on which
I provoked a few crashes with `kill -n11 $(pidof ceph-osd)`.

ceph-base with patch 2 applied (provided by Max off-list) correctly
changed the /var/lib/ceph/crash/posted permissions to ceph:ceph for me.

Installing pve-manager (with patch 8 applied) on node 1 created a
keyring and added the section to /etc/pve/ceph.conf. However, installing
on node 2 added a second `keyring` line to the section:

[client.crash]
	 keyring = /etc/pve/ceph/$cluster.$name.keyring
	 keyring = /etc/pve/ceph/$cluster.$name.keyring

Same thing happens on each `dpkg-reconfigure pve-manager` I think.

Also, looks like every time ceph-crash posts a report, the syslog reads:

Jan 31 15:02:30 ceph1 ceph-crash[110939]: WARNING:ceph-crash:post
/var/lib/ceph/crash/2024-01-31T13:53:16.419342Z_1b5a078a-f665-4fcd-abd5-9bf602048d1f
as client.crash.ceph1 failed: 2024-01-31T15:02:30.105+0100 7f10bf7ae6c0
-1 auth: unable to find a keyring on
/etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
7f10bf7ae6c0 -1 auth: unable to find a keyring on
/etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
7f10bf7ae6c0 -1 auth: unable to find a keyring on
/etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
7f10bf7ae6c0 -1 auth: unable to find a keyring on
/etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
7f10bf7ae6c0 -1 monclient: keyring not found
Jan 31 15:02:30 ceph1 ceph-crash[110939]: [errno 13] RADOS permission
denied (error connecting to the cluster)

I remember you mentioned this before. Do I remember correctly there is
no easy way to prevent these messages? Having them appear only when a
crash is posted is certainly better than every 10 minutes, but they are
a bit misleading as they very much look like an error that needs attention.

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

[Fabian Grünbichler]

On January 31, 2024 3:22 pm, Friedrich Weber wrote:
> Also, looks like every time ceph-crash posts a report, the syslog reads:
> 
> Jan 31 15:02:30 ceph1 ceph-crash[110939]: WARNING:ceph-crash:post
> /var/lib/ceph/crash/2024-01-31T13:53:16.419342Z_1b5a078a-f665-4fcd-abd5-9bf602048d1f
> as client.crash.ceph1 failed: 2024-01-31T15:02:30.105+0100 7f10bf7ae6c0
> -1 auth: unable to find a keyring on
> /etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
> Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
> 7f10bf7ae6c0 -1 auth: unable to find a keyring on
> /etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
> Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
> 7f10bf7ae6c0 -1 auth: unable to find a keyring on
> /etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
> Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
> 7f10bf7ae6c0 -1 auth: unable to find a keyring on
> /etc/pve/priv/ceph.client.crash.ceph1.keyring: (13) Permission denied
> Jan 31 15:02:30 ceph1 ceph-crash[110939]: 2024-01-31T15:02:30.105+0100
> 7f10bf7ae6c0 -1 monclient: keyring not found
> Jan 31 15:02:30 ceph1 ceph-crash[110939]: [errno 13] RADOS permission
> denied (error connecting to the cluster)
> 
> I remember you mentioned this before. Do I remember correctly there is
> no easy way to prevent these messages? Having them appear only when a
> crash is posted is certainly better than every 10 minutes, but they are
> a bit misleading as they very much look like an error that needs attention.

so I did a few more experiments.

ceph-crash does two things

A) it executes `ceph -s` without specifying a client name, which means
that part will always try to use the `client.admin` config/keyring
B) it tries to post crashes if they exist, using the keys
`client.crash.$HOST`, `client.crash`, `client.admin`

A happens at startup to "exercise the key", irrespective of crash files
existing or not. we'd need to patch ceph-crash once we settled which
client name to use to avoid it.

B happens for every crash, once posting worked the other keyrings are
not tried again for that particular crash, but will for the next.

this means to avoid warnings altogether, we'd need to make the first
entry in auth_names work or patch the `auth_names` part of the
ceph-crash binary.

I played around a bit and it seems we could do the following:
- change the [client] section in our config to only affect
  [client.admin] (simple renaming is enough, all `ceph` invocations
  without `-n` or `-i` should continue to work as before, since
  "client.admin" is the default `-n` value)
- generate (on each node) a `client.crash.$HOSTNAME` keyring with crash
  profile and store it in /etc/ceph/ceph.client.crash.$HOSTNAME

ceph-crash will then (at least for crash posting purposes) invoke `ceph
-n client.crash.$HOSTNAME` first, which will pick up that keyring since
`/etc/ceph/$cluster.$name.keyring` is part of the default value(s) for
the client keyring. this doesn't work without modifying our ceph.conf
since the current global "client.keyring" setting overrides the built-in
defaults for *all* invocations, even for `ceph -n XXX`.

using the current approach with "client.crash" and a key on pmxcfs also
works, to silence the warnings we could then patch ceph-crash to use
that key (/client name) for `ceph -s` and remove the
`client.crash.$HOSTNAME` from auth_names. but I assume since that comes
first, that upstream actually expects people to use that keyring, the
rest are just fallbacks, so we'd need to watch for regressions when
pulling in updates.