* [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:32 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
` (9 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* use `find` instead of for-loop
...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
patches/series | 1 +
2 files changed, 51 insertions(+)
create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
new file mode 100644
index 000000000..7445f3945
--- /dev/null
+++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
@@ -0,0 +1,50 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 1 Feb 2024 18:43:36 +0100
+Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by also updating the permissions of
+/var/lib/ceph/*/* - the subdirectories and files of the directories in
+/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 16 +++++++++-------
+ 1 file changed, 9 insertions(+), 7 deletions(-)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..70d07977f82 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -33,13 +33,15 @@ case "$1" in
+ rm -f /etc/init/ceph.conf
+ [ -x /sbin/start ] && start ceph-all || :
+
+- # adjust file and directory permissions
+- for DIR in /var/lib/ceph/* ; do
+- if ! dpkg-statoverride --list $DIR >/dev/null
+- then
+- chown $SERVER_USER:$SERVER_GROUP $DIR
+- fi
+- done
++ PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
++
++ # adjust directory permissions
++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++ # adjust file permissions
++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
+ ;;
+ abort-upgrade|abort-remove|abort-deconfigure)
+ :
+--
+2.39.2
+
diff --git a/patches/series b/patches/series
index 865caf23d..cf8f1ea31 100644
--- a/patches/series
+++ b/patches/series
@@ -12,3 +12,4 @@
0012-backport-mgr-dashboard-simplify-authentication-proto.patch
0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
0014-rocksb-inherit-parent-cmake-cxx-flags.patch
+0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
@ 2024-02-12 13:32 ` Fabian Grünbichler
2024-02-13 8:25 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:32 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
>
> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
> * use `find` instead of for-loop
>
> ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
> patches/series | 1 +
> 2 files changed, 51 insertions(+)
> create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>
> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> new file mode 100644
> index 000000000..7445f3945
> --- /dev/null
> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> @@ -0,0 +1,50 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Thu, 1 Feb 2024 18:43:36 +0100
> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
> +
> +A rather recent PR made ceph-crash run as "ceph" user instead of
> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
> +ceph-crash cannot actually post any crash logs now.
> +
> +This commit fixes this by also updating the permissions of
> +/var/lib/ceph/*/* - the subdirectories and files of the directories in
> +/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
> +
> +[0]: https://github.com/ceph/ceph/pull/48713
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + debian/ceph-base.postinst | 16 +++++++++-------
> + 1 file changed, 9 insertions(+), 7 deletions(-)
> +
> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
> +index 75eeb59c624..70d07977f82 100644
> +--- a/debian/ceph-base.postinst
> ++++ b/debian/ceph-base.postinst
> +@@ -33,13 +33,15 @@ case "$1" in
> + rm -f /etc/init/ceph.conf
> + [ -x /sbin/start ] && start ceph-all || :
> +
> +- # adjust file and directory permissions
> +- for DIR in /var/lib/ceph/* ; do
> +- if ! dpkg-statoverride --list $DIR >/dev/null
> +- then
> +- chown $SERVER_USER:$SERVER_GROUP $DIR
> +- fi
> +- done
> ++ PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
this doesn't quote {} properly, files with spaces or other things in
them can cause issues including shell command injection as root when
this is executed by dpkg..
> ++
> ++ # adjust directory permissions
> ++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
> ++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
> ++
> ++ # adjust file permissions
> ++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
> ++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
> + ;;
do we need the depth stuff or the split by type? we want to make
everything under /var/lib/ceph owned by ceph:ceph, unless the admin has
specifically overriden a particular path.
a simple
find /var/lib/ceph -print0 | ...
should work and be much simpler (or if we want to limit to dirs and
files, that can also be simply done in one go by ORing the two checks)
> + abort-upgrade|abort-remove|abort-deconfigure)
> + :
> +--
> +2.39.2
> +
> diff --git a/patches/series b/patches/series
> index 865caf23d..cf8f1ea31 100644
> --- a/patches/series
> +++ b/patches/series
> @@ -12,3 +12,4 @@
> 0012-backport-mgr-dashboard-simplify-authentication-proto.patch
> 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
> 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
> +0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook
2024-02-12 13:32 ` Fabian Grünbichler
@ 2024-02-13 8:25 ` Max Carrara
0 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-13 8:25 UTC (permalink / raw)
To: pve-devel
On 2/12/24 14:32, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
>> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
>>
>> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>> * use `find` instead of for-loop
>>
>> ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
>> patches/series | 1 +
>> 2 files changed, 51 insertions(+)
>> create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>>
>> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> new file mode 100644
>> index 000000000..7445f3945
>> --- /dev/null
>> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> @@ -0,0 +1,50 @@
>> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
>> +From: Max Carrara <m.carrara@proxmox.com>
>> +Date: Thu, 1 Feb 2024 18:43:36 +0100
>> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
>> +
>> +A rather recent PR made ceph-crash run as "ceph" user instead of
>> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
>> +ceph-crash cannot actually post any crash logs now.
>> +
>> +This commit fixes this by also updating the permissions of
>> +/var/lib/ceph/*/* - the subdirectories and files of the directories in
>> +/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
>> +
>> +[0]: https://github.com/ceph/ceph/pull/48713
>> +
>> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> +---
>> + debian/ceph-base.postinst | 16 +++++++++-------
>> + 1 file changed, 9 insertions(+), 7 deletions(-)
>> +
>> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
>> +index 75eeb59c624..70d07977f82 100644
>> +--- a/debian/ceph-base.postinst
>> ++++ b/debian/ceph-base.postinst
>> +@@ -33,13 +33,15 @@ case "$1" in
>> + rm -f /etc/init/ceph.conf
>> + [ -x /sbin/start ] && start ceph-all || :
>> +
>> +- # adjust file and directory permissions
>> +- for DIR in /var/lib/ceph/* ; do
>> +- if ! dpkg-statoverride --list $DIR >/dev/null
>> +- then
>> +- chown $SERVER_USER:$SERVER_GROUP $DIR
>> +- fi
>> +- done
>> ++ PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
>
> this doesn't quote {} properly, files with spaces or other things in
> them can cause issues including shell command injection as root when
> this is executed by dpkg..
Noted, will be corrected in v3!
>
>> ++
>> ++ # adjust directory permissions
>> ++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
>> ++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> ++
>> ++ # adjust file permissions
>> ++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
>> ++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> + ;;
>
> do we need the depth stuff or the split by type? we want to make
> everything under /var/lib/ceph owned by ceph:ceph, unless the admin has
> specifically overriden a particular path.
I added `-mindepth 1` because the '/var/lib/ceph' dir would be included in
`find`'s results otherwise, which isn't the case in the original code.
`-maxdepth 2` is to limit it to the depth that I had originally intended in
v1 - files and subdirectories, as well as sub-subdirectories and files in
those.
I agree that everything (in those two levels?) could just be `chown`ed
instead though - would make the above a little less verbose and only
need one invocation.
>
> a simple
>
> find /var/lib/ceph -print0 | ...
>
> should work and be much simpler (or if we want to limit to dirs and
> files, that can also be simply done in one go by ORing the two checks)
ORing the two checks didn't work in my case - turns out I just held `find`
wrongly.
Will correct all the above in v3 - thanks for your feedback!
>
>> + abort-upgrade|abort-remove|abort-deconfigure)
>> + :
>> +--
>> +2.39.2
>> +
>> diff --git a/patches/series b/patches/series
>> index 865caf23d..cf8f1ea31 100644
>> --- a/patches/series
>> +++ b/patches/series
>> @@ -12,3 +12,4 @@
>> 0012-backport-mgr-dashboard-simplify-authentication-proto.patch
>> 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>> 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
>> +0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> --
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:33 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 03/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
` (8 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
This patch makes it so that `ceph-crash` attempts to use the
non-host-specific keyring before anything else, which avoids
unnecessary error messages landing in the systemd-journal in our case.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* new
...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
patches/series | 1 +
2 files changed, 31 insertions(+)
create mode 100644 patches/0016-ceph-crash-change-order-of-client-names.patch
diff --git a/patches/0016-ceph-crash-change-order-of-client-names.patch b/patches/0016-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..cc0bdccdb
--- /dev/null
+++ b/patches/0016-ceph-crash-change-order-of-client-names.patch
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Mon, 5 Feb 2024 11:44:14 +0100
+Subject: [PATCH] ceph-crash: change order of client names
+
+This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ src/ceph-crash.in | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/ceph-crash.in b/src/ceph-crash.in
+index 010d6ea2e82..d1d9f4123e6 100755
+--- a/src/ceph-crash.in
++++ b/src/ceph-crash.in
+@@ -16,8 +16,8 @@ import time
+ logging.basicConfig(level=logging.INFO)
+ log = logging.getLogger('ceph-crash')
+
+-auth_names = ['client.crash.%s' % socket.gethostname(),
+- 'client.crash',
++auth_names = ['client.crash',
++ 'client.crash.%s' % socket.gethostname(),
+ 'client.admin']
+
+
+--
+2.39.2
+
diff --git a/patches/series b/patches/series
index cf8f1ea31..1c78e62ba 100644
--- a/patches/series
+++ b/patches/series
@@ -13,3 +13,4 @@
0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
0014-rocksb-inherit-parent-cmake-cxx-flags.patch
0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
+0016-ceph-crash-change-order-of-client-names.patch
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
@ 2024-02-12 13:33 ` Fabian Grünbichler
0 siblings, 0 replies; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:33 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> This patch makes it so that `ceph-crash` attempts to use the
> non-host-specific keyring before anything else, which avoids
> unnecessary error messages landing in the systemd-journal in our case.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
worst case, a user that has already manually setup client.crash.$host
will now get spurios warnings, but crashes woulds till get posted
anyhow..
> ---
> Changes v1 --> v2:
> * new
>
> ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
> patches/series | 1 +
> 2 files changed, 31 insertions(+)
> create mode 100644 patches/0016-ceph-crash-change-order-of-client-names.patch
>
> diff --git a/patches/0016-ceph-crash-change-order-of-client-names.patch b/patches/0016-ceph-crash-change-order-of-client-names.patch
> new file mode 100644
> index 000000000..cc0bdccdb
> --- /dev/null
> +++ b/patches/0016-ceph-crash-change-order-of-client-names.patch
> @@ -0,0 +1,30 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Mon, 5 Feb 2024 11:44:14 +0100
> +Subject: [PATCH] ceph-crash: change order of client names
> +
> +This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + src/ceph-crash.in | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/src/ceph-crash.in b/src/ceph-crash.in
> +index 010d6ea2e82..d1d9f4123e6 100755
> +--- a/src/ceph-crash.in
> ++++ b/src/ceph-crash.in
> +@@ -16,8 +16,8 @@ import time
> + logging.basicConfig(level=logging.INFO)
> + log = logging.getLogger('ceph-crash')
> +
> +-auth_names = ['client.crash.%s' % socket.gethostname(),
> +- 'client.crash',
> ++auth_names = ['client.crash',
> ++ 'client.crash.%s' % socket.gethostname(),
> + 'client.admin']
> +
> +
> +--
> +2.39.2
> +
> diff --git a/patches/series b/patches/series
> index cf8f1ea31..1c78e62ba 100644
> --- a/patches/series
> +++ b/patches/series
> @@ -13,3 +13,4 @@
> 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
> 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
> 0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> +0016-ceph-crash-change-order-of-client-names.patch
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 quincy-stable-8 ceph 03/11] debian: add patch to fix ceph crash dir permissions in postinst hook
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:32 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
` (7 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* use `find` instead of for-loop
...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
patches/series | 1 +
2 files changed, 51 insertions(+)
create mode 100644 patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
diff --git a/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
new file mode 100644
index 000000000..7445f3945
--- /dev/null
+++ b/patches/0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
@@ -0,0 +1,50 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 1 Feb 2024 18:43:36 +0100
+Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by also updating the permissions of
+/var/lib/ceph/*/* - the subdirectories and files of the directories in
+/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 16 +++++++++-------
+ 1 file changed, 9 insertions(+), 7 deletions(-)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..70d07977f82 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -33,13 +33,15 @@ case "$1" in
+ rm -f /etc/init/ceph.conf
+ [ -x /sbin/start ] && start ceph-all || :
+
+- # adjust file and directory permissions
+- for DIR in /var/lib/ceph/* ; do
+- if ! dpkg-statoverride --list $DIR >/dev/null
+- then
+- chown $SERVER_USER:$SERVER_GROUP $DIR
+- fi
+- done
++ PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
++
++ # adjust directory permissions
++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++ # adjust file permissions
++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
+ ;;
+ abort-upgrade|abort-remove|abort-deconfigure)
+ :
+--
+2.39.2
+
diff --git a/patches/series b/patches/series
index ee897a78a..34185c7da 100644
--- a/patches/series
+++ b/patches/series
@@ -16,3 +16,4 @@
0021-backport-mgr-dashboard-simplify-authentication-proto.patch
0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
0023-rocksb-inherit-parent-cmake-cxx-flags.patch
+0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (2 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 03/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:33 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser Max Carrara
` (6 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
This patch makes it so that `ceph-crash` attempts to use the
non-host-specific keyring before anything else, which avoids
unnecessary error messages landing in the systemd-journal in our case.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* new
...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
patches/series | 1 +
2 files changed, 31 insertions(+)
create mode 100644 patches/0025-ceph-crash-change-order-of-client-names.patch
diff --git a/patches/0025-ceph-crash-change-order-of-client-names.patch b/patches/0025-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..cc0bdccdb
--- /dev/null
+++ b/patches/0025-ceph-crash-change-order-of-client-names.patch
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Mon, 5 Feb 2024 11:44:14 +0100
+Subject: [PATCH] ceph-crash: change order of client names
+
+This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ src/ceph-crash.in | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/ceph-crash.in b/src/ceph-crash.in
+index 010d6ea2e82..d1d9f4123e6 100755
+--- a/src/ceph-crash.in
++++ b/src/ceph-crash.in
+@@ -16,8 +16,8 @@ import time
+ logging.basicConfig(level=logging.INFO)
+ log = logging.getLogger('ceph-crash')
+
+-auth_names = ['client.crash.%s' % socket.gethostname(),
+- 'client.crash',
++auth_names = ['client.crash',
++ 'client.crash.%s' % socket.gethostname(),
+ 'client.admin']
+
+
+--
+2.39.2
+
diff --git a/patches/series b/patches/series
index 34185c7da..8160de645 100644
--- a/patches/series
+++ b/patches/series
@@ -17,3 +17,4 @@
0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
0023-rocksb-inherit-parent-cmake-cxx-flags.patch
0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
+0025-ceph-crash-change-order-of-client-names.patch
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
@ 2024-02-12 13:33 ` Fabian Grünbichler
0 siblings, 0 replies; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:33 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> This patch makes it so that `ceph-crash` attempts to use the
> non-host-specific keyring before anything else, which avoids
> unnecessary error messages landing in the systemd-journal in our case.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> Changes v1 --> v2:
> * new
>
> ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
> patches/series | 1 +
> 2 files changed, 31 insertions(+)
> create mode 100644 patches/0025-ceph-crash-change-order-of-client-names.patch
>
> diff --git a/patches/0025-ceph-crash-change-order-of-client-names.patch b/patches/0025-ceph-crash-change-order-of-client-names.patch
> new file mode 100644
> index 000000000..cc0bdccdb
> --- /dev/null
> +++ b/patches/0025-ceph-crash-change-order-of-client-names.patch
> @@ -0,0 +1,30 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Max Carrara <m.carrara@proxmox.com>
> +Date: Mon, 5 Feb 2024 11:44:14 +0100
> +Subject: [PATCH] ceph-crash: change order of client names
> +
> +This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
> +
> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> +---
> + src/ceph-crash.in | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/src/ceph-crash.in b/src/ceph-crash.in
> +index 010d6ea2e82..d1d9f4123e6 100755
> +--- a/src/ceph-crash.in
> ++++ b/src/ceph-crash.in
> +@@ -16,8 +16,8 @@ import time
> + logging.basicConfig(level=logging.INFO)
> + log = logging.getLogger('ceph-crash')
> +
> +-auth_names = ['client.crash.%s' % socket.gethostname(),
> +- 'client.crash',
> ++auth_names = ['client.crash',
> ++ 'client.crash.%s' % socket.gethostname(),
> + 'client.admin']
> +
> +
> +--
> +2.39.2
> +
> diff --git a/patches/series b/patches/series
> index 34185c7da..8160de645 100644
> --- a/patches/series
> +++ b/patches/series
> @@ -17,3 +17,4 @@
> 0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
> 0023-rocksb-inherit-parent-cmake-cxx-flags.patch
> 0024-debian-adjust-permissions-of-subdirectories-of-var-l.patch
> +0025-ceph-crash-change-order-of-client-names.patch
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (3 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:33 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections Max Carrara
` (5 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
1. Comments, irrespective of whether they start with '#' or ';' are
now treated the same. Otherwise, sections and key-value pairs with
a trailing comment starting with ';' are still parsed. Consider
this example:
[some.section] # inline comment after section
foo = bar ; inline comment after value
The '[some.section]' section in the example above would otherwise
not be parsed at all, while in the key-value definition 'foo'
parses as the key, which is correct, but 'bar ; inline comment
after value' parses as value, which is incorrect according to
Ceph's grammar [0][1].
2. Sections may now contain any character, including whitespace, but
not '\n' or a comment literal '#' or ';'. The case for comment
literals is handled in 1. above.
3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
are treated the same, like in Ceph's parser [2].
[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
[1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
[2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* new
src/PVE/CephConfig.pm | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b10d46..77b745f 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
\&parse_ceph_config,
\&write_ceph_config);
+# For more details on how Ceph's config parser works, see:
+# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
sub parse_ceph_config {
my ($filename, $raw) = @_;
@@ -20,14 +22,13 @@ sub parse_ceph_config {
my $section;
- foreach my $line (@lines) {
- $line =~ s/#.*$//;
+ for my $line (@lines) {
+ $line =~ s/(#|;).*$//;
$line =~ s/^\s+//;
- $line =~ s/^;.*$//;
$line =~ s/\s+$//;
next if !$line;
- $section = $1 if $line =~ m/^\[(\S+)\]$/;
+ $section = $1 if $line =~ m/^\[(.+)\]$/;
if (!$section) {
warn "no section - skip: $line\n";
next;
@@ -35,11 +36,10 @@ sub parse_ceph_config {
if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
my ($key, $val) = ($1, $2);
- # ceph treats ' ', '_' and '-' in keys the same, so lets do too
- $key =~ s/[-\ ]/_/g;
+ # ceph treats ' ' and '_' in keys the same, so lets do too
+ $key =~ s/ /_/g;
$cfg->{$section}->{$key} = $val;
}
-
}
return $cfg;
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser Max Carrara
@ 2024-02-12 13:33 ` Fabian Grünbichler
2024-02-13 8:34 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:33 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> 1. Comments, irrespective of whether they start with '#' or ';' are
> now treated the same. Otherwise, sections and key-value pairs with
> a trailing comment starting with ';' are still parsed. Consider
> this example:
>
> [some.section] # inline comment after section
> foo = bar ; inline comment after value
>
> The '[some.section]' section in the example above would otherwise
> not be parsed at all, while in the key-value definition 'foo'
> parses as the key, which is correct, but 'bar ; inline comment
> after value' parses as value, which is incorrect according to
> Ceph's grammar [0][1].
>
> 2. Sections may now contain any character, including whitespace, but
> not '\n' or a comment literal '#' or ';'. The case for comment
> literals is handled in 1. above.
these seem sensible - what about line continuations? ;)
>
> 3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
> are treated the same, like in Ceph's parser [2].
the ceph docs state something else - which is wrong? ;)
https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/ says:
> Each of the Ceph configuration options has a unique name that consists
> of words formed with lowercase characters and connected with
> underscore characters (_).
this would seem to agree
> When option names are specified on the command line, underscore (_)
> and dash (-) characters can be used interchangeably (for example,
> --mon-host is equivalent to --mon_host).
okay, this is CLI which might just have its own mapping
> When option names appear in configuration files, spaces can also be
> used in place of underscores or dashes. However, for the sake of
> clarity and convenience, we suggest that you consistently use
> underscores, as we do throughout this documentation.
but this now says that dash in config files is OK?
> [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
> [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
> [2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
> * new
>
> src/PVE/CephConfig.pm | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 6b10d46..77b745f 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
> \&parse_ceph_config,
> \&write_ceph_config);
>
> +# For more details on how Ceph's config parser works, see:
> +# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
> sub parse_ceph_config {
> my ($filename, $raw) = @_;
>
> @@ -20,14 +22,13 @@ sub parse_ceph_config {
>
> my $section;
>
> - foreach my $line (@lines) {
> - $line =~ s/#.*$//;
> + for my $line (@lines) {
> + $line =~ s/(#|;).*$//;
> $line =~ s/^\s+//;
> - $line =~ s/^;.*$//;
> $line =~ s/\s+$//;
> next if !$line;
>
> - $section = $1 if $line =~ m/^\[(\S+)\]$/;
> + $section = $1 if $line =~ m/^\[(.+)\]$/;
> if (!$section) {
> warn "no section - skip: $line\n";
> next;
> @@ -35,11 +36,10 @@ sub parse_ceph_config {
>
> if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
> my ($key, $val) = ($1, $2);
> - # ceph treats ' ', '_' and '-' in keys the same, so lets do too
> - $key =~ s/[-\ ]/_/g;
> + # ceph treats ' ' and '_' in keys the same, so lets do too
> + $key =~ s/ /_/g;
> $cfg->{$section}->{$key} = $val;
> }
> -
> }
>
> return $cfg;
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser
2024-02-12 13:33 ` Fabian Grünbichler
@ 2024-02-13 8:34 ` Max Carrara
0 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-13 8:34 UTC (permalink / raw)
To: pve-devel
On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> 1. Comments, irrespective of whether they start with '#' or ';' are
>> now treated the same. Otherwise, sections and key-value pairs with
>> a trailing comment starting with ';' are still parsed. Consider
>> this example:
>>
>> [some.section] # inline comment after section
>> foo = bar ; inline comment after value
>>
>> The '[some.section]' section in the example above would otherwise
>> not be parsed at all, while in the key-value definition 'foo'
>> parses as the key, which is correct, but 'bar ; inline comment
>> after value' parses as value, which is incorrect according to
>> Ceph's grammar [0][1].
>>
>> 2. Sections may now contain any character, including whitespace, but
>> not '\n' or a comment literal '#' or ';'. The case for comment
>> literals is handled in 1. above.
>
> these seem sensible - what about line continuations? ;)
Yeah, those get swallowed. Will be corrected in v3, thanks for pointing
this out!
>
>>
>> 3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
>> are treated the same, like in Ceph's parser [2].
>
> the ceph docs state something else - which is wrong? ;)
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/ says:
>
>> Each of the Ceph configuration options has a unique name that consists
>> of words formed with lowercase characters and connected with
>> underscore characters (_).
>
> this would seem to agree
>
>> When option names are specified on the command line, underscore (_)
>> and dash (-) characters can be used interchangeably (for example,
>> --mon-host is equivalent to --mon_host).
>
> okay, this is CLI which might just have its own mapping
>
>> When option names appear in configuration files, spaces can also be
>> used in place of underscores or dashes. However, for the sake of
>> clarity and convenience, we suggest that you consistently use
>> underscores, as we do throughout this documentation.
>
> but this now says that dash in config files is OK?
The docs and the code don't seem to agree in that regard; at least the
INI parser in C++ does *not* treat `-` the same [0]:
> /* Normalize a key name.
> *
> * Normalized key names have no leading or trailing whitespace, and all
> * whitespace is stored as underscores. The main reason for selecting this
> * normal form is so that in common/config.cc, we can use a macro to stringify
> * the field names of md_config_t and get a key in normal form.
> */
(The code also actually does what it says on the tin. ;) )
So, I think it would be sensible to stick to how the Code does it, in this
case. Or was the original treatment of dashes in our code included in order
to guard against user errors, perhaps?
[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
>
>> [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
>> [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
>> [2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>> * new
>>
>> src/PVE/CephConfig.pm | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..77b745f 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
>> \&parse_ceph_config,
>> \&write_ceph_config);
>>
>> +# For more details on how Ceph's config parser works, see:
>> +# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
>> sub parse_ceph_config {
>> my ($filename, $raw) = @_;
>>
>> @@ -20,14 +22,13 @@ sub parse_ceph_config {
>>
>> my $section;
>>
>> - foreach my $line (@lines) {
>> - $line =~ s/#.*$//;
>> + for my $line (@lines) {
>> + $line =~ s/(#|;).*$//;
>> $line =~ s/^\s+//;
>> - $line =~ s/^;.*$//;
>> $line =~ s/\s+$//;
>> next if !$line;
>>
>> - $section = $1 if $line =~ m/^\[(\S+)\]$/;
>> + $section = $1 if $line =~ m/^\[(.+)\]$/;
>> if (!$section) {
>> warn "no section - skip: $line\n";
>> next;
>> @@ -35,11 +36,10 @@ sub parse_ceph_config {
>>
>> if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
>> my ($key, $val) = ($1, $2);
>> - # ceph treats ' ', '_' and '-' in keys the same, so lets do too
>> - $key =~ s/[-\ ]/_/g;
>> + # ceph treats ' ' and '_' in keys the same, so lets do too
>> + $key =~ s/ /_/g;
>> $cfg->{$section}->{$key} = $val;
>> }
>> -
>> }
>>
>> return $cfg;
>> --
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (4 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:33 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 07/11] amend! " Max Carrara
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
This adds support for writing arbitrary sections to 'ceph.conf' while
ensuring that already written sections are not duplicated.
Sections that are associated with the client, for example
'[client.foo]', are written directly after the '[client]' section.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* Instead of just adding 'client.crash' as a separate section,
also allow writing arbitrary sections
src/PVE/CephConfig.pm | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 77b745f..86d3079 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
sub write_ceph_config {
my ($filename, $cfg) = @_;
+ my $written_sections = {};
my $out = '';
my $cond_write_sec = sub {
my $re = shift;
- foreach my $section (sort keys %$cfg) {
- next if $section !~ m/^$re$/;
+ for my $section (sort keys %$cfg) {
+ if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
+ next;
+ }
+
$out .= "[$section]\n";
- foreach my $key (sort keys %{$cfg->{$section}}) {
- $out .= "\t $key = $cfg->{$section}->{$key}\n";
+ for my $key (sort keys %{$cfg->{$section}}) {
+ $out .= "\t$key = $cfg->{$section}->{$key}\n";
}
$out .= "\n";
+
+ $written_sections->{$section} = 1;
}
};
&$cond_write_sec('global');
&$cond_write_sec('client');
+ &$cond_write_sec('client\..*');
&$cond_write_sec('mds');
&$cond_write_sec('mon');
@@ -88,6 +95,8 @@ sub write_ceph_config {
&$cond_write_sec('osd\..*');
&$cond_write_sec('mgr\..*');
+ &$cond_write_sec('.*');
+
return $out;
}
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections Max Carrara
@ 2024-02-12 13:33 ` Fabian Grünbichler
2024-02-13 8:46 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:33 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> This adds support for writing arbitrary sections to 'ceph.conf' while
> ensuring that already written sections are not duplicated.
>
> Sections that are associated with the client, for example
> '[client.foo]', are written directly after the '[client]' section.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
would have been easier to parse if the style cleanup and actual
behaviour change would have been split..
style change:
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 6b10d46..34c3107 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -65,10 +65,10 @@ sub write_ceph_config {
> my $cond_write_sec = sub {
> my $re = shift;
>
> - foreach my $section (sort keys %$cfg) {
> + for my $section (sort keys %$cfg) {
> next if $section !~ m/^$re$/;
> $out .= "[$section]\n";
> - foreach my $key (sort keys %{$cfg->{$section}}) {
> + for my $key (sort keys %{$cfg->{$section}}) {
> $out .= "\t $key = $cfg->{$section}->{$key}\n";
> }
> $out .= "\n";
actual change (small nits inline!):
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 34c3107..24bc78c 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
> sub write_ceph_config {
> my ($filename, $cfg) = @_;
>
> + my $written_sections = {};
> my $out = '';
>
> my $cond_write_sec = sub {
> my $re = shift;
>
> for my $section (sort keys %$cfg) {
> - next if $section !~ m/^$re$/;
> + if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
> + next;
> + }
these two could be more clearly written as
next if $written_sections->{section};
next if $section !~ m/$re$/;
since the two checks are not related in any way, other than both having
the same effect if true (skipping that section). the second line is then
unchanged, and both the diff and the resulting code is easier to parse
IMHO.
> +
> $out .= "[$section]\n";
> for my $key (sort keys %{$cfg->{$section}}) {
> - $out .= "\t $key = $cfg->{$section}->{$key}\n";
> + $out .= "\t$key = $cfg->{$section}->{$key}\n";
this part here is not mentioned at all, and I might have missed it if I
hadn't split the diffs ;)
> }
> $out .= "\n";
> +
> + $written_sections->{$section} = 1;
> }
> };
>
> &$cond_write_sec('global');
> &$cond_write_sec('client');
> + &$cond_write_sec('client\..*');
>
> &$cond_write_sec('mds');
> &$cond_write_sec('mon');
> @@ -88,6 +95,8 @@ sub write_ceph_config {
> &$cond_write_sec('osd\..*');
> &$cond_write_sec('mgr\..*');
>
> + &$cond_write_sec('.*');
> +
> return $out;
> }
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections
2024-02-12 13:33 ` Fabian Grünbichler
@ 2024-02-13 8:46 ` Max Carrara
0 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-13 8:46 UTC (permalink / raw)
To: pve-devel
On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>
> would have been easier to parse if the style cleanup and actual
> behaviour change would have been split..
Fair point, acknowledged!
>
> style change:
>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..34c3107 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -65,10 +65,10 @@ sub write_ceph_config {
>> my $cond_write_sec = sub {
>> my $re = shift;
>>
>> - foreach my $section (sort keys %$cfg) {
>> + for my $section (sort keys %$cfg) {
>> next if $section !~ m/^$re$/;
>> $out .= "[$section]\n";
>> - foreach my $key (sort keys %{$cfg->{$section}}) {
>> + for my $key (sort keys %{$cfg->{$section}}) {
>> $out .= "\t $key = $cfg->{$section}->{$key}\n";
>> }
>> $out .= "\n";
>
> actual change (small nits inline!):
>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 34c3107..24bc78c 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
>> sub write_ceph_config {
>> my ($filename, $cfg) = @_;
>>
>> + my $written_sections = {};
>> my $out = '';
>>
>> my $cond_write_sec = sub {
>> my $re = shift;
>>
>> for my $section (sort keys %$cfg) {
>> - next if $section !~ m/^$re$/;
>> + if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
>> + next;
>> + }
>
> these two could be more clearly written as
>
> next if $written_sections->{section};
> next if $section !~ m/$re$/;
>
> since the two checks are not related in any way, other than both having
> the same effect if true (skipping that section). the second line is then
> unchanged, and both the diff and the resulting code is easier to parse
> IMHO.
>
>> +
>> $out .= "[$section]\n";
>> for my $key (sort keys %{$cfg->{$section}}) {
>> - $out .= "\t $key = $cfg->{$section}->{$key}\n";
>> + $out .= "\t$key = $cfg->{$section}->{$key}\n";
>
> this part here is not mentioned at all, and I might have missed it if I
> hadn't split the diffs ;)
Mea culpa!
Will clean this up in v3 and split the changes accordingly.
>
>> }
>> $out .= "\n";
>> +
>> + $written_sections->{$section} = 1;
>> }
>> };
>>
>> &$cond_write_sec('global');
>> &$cond_write_sec('client');
>> + &$cond_write_sec('client\..*');
>>
>> &$cond_write_sec('mds');
>> &$cond_write_sec('mon');
>> @@ -88,6 +95,8 @@ sub write_ceph_config {
>> &$cond_write_sec('osd\..*');
>> &$cond_write_sec('mgr\..*');
>>
>> + &$cond_write_sec('.*');
>> +
>> return $out;
>> }
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (5 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:33 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge Max Carrara
` (3 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
cephconfig: allow writing arbitrary sections
This adds support for writing arbitrary sections to 'ceph.conf' while
ensuring that already written sections are not duplicated.
Sections that are associated with the client, for example
'[client.foo]', are written directly after the '[client]' section.
Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
written directly after their associated section.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
NOTE: This amend really just changes the order of the sections below and
may be dropped if not desired.
Changes v1 --> v2:
* new
src/PVE/CephConfig.pm | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 86d3079..00b1f35 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -82,17 +82,20 @@ sub write_ceph_config {
};
&$cond_write_sec('global');
+
&$cond_write_sec('client');
&$cond_write_sec('client\..*');
&$cond_write_sec('mds');
- &$cond_write_sec('mon');
- &$cond_write_sec('osd');
- &$cond_write_sec('mgr');
-
&$cond_write_sec('mds\..*');
+
+ &$cond_write_sec('mon');
&$cond_write_sec('mon\..*');
+
+ &$cond_write_sec('osd');
&$cond_write_sec('osd\..*');
+
+ &$cond_write_sec('mgr');
&$cond_write_sec('mgr\..*');
&$cond_write_sec('.*');
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 07/11] amend! " Max Carrara
@ 2024-02-12 13:33 ` Fabian Grünbichler
2024-02-13 8:50 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:33 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> cephconfig: allow writing arbitrary sections
>
> This adds support for writing arbitrary sections to 'ceph.conf' while
> ensuring that already written sections are not duplicated.
>
> Sections that are associated with the client, for example
> '[client.foo]', are written directly after the '[client]' section.
>
> Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
> written directly after their associated section.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> NOTE: This amend really just changes the order of the sections below and
> may be dropped if not desired.
does the order matter at all? the docs would imply that it doesn't,
since there is a clear precedence between [global], [$type], and
[$type.$id] (e.g., global -> client -> client.crash)..
https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#configuration-sections
>
> Changes v1 --> v2:
> * new
>
> src/PVE/CephConfig.pm | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> index 86d3079..00b1f35 100644
> --- a/src/PVE/CephConfig.pm
> +++ b/src/PVE/CephConfig.pm
> @@ -82,17 +82,20 @@ sub write_ceph_config {
> };
>
> &$cond_write_sec('global');
> +
> &$cond_write_sec('client');
> &$cond_write_sec('client\..*');
>
> &$cond_write_sec('mds');
> - &$cond_write_sec('mon');
> - &$cond_write_sec('osd');
> - &$cond_write_sec('mgr');
> -
> &$cond_write_sec('mds\..*');
> +
> + &$cond_write_sec('mon');
> &$cond_write_sec('mon\..*');
> +
> + &$cond_write_sec('osd');
> &$cond_write_sec('osd\..*');
> +
> + &$cond_write_sec('mgr');
> &$cond_write_sec('mgr\..*');
>
> &$cond_write_sec('.*');
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections
2024-02-12 13:33 ` Fabian Grünbichler
@ 2024-02-13 8:50 ` Max Carrara
0 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-13 8:50 UTC (permalink / raw)
To: pve-devel
On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> cephconfig: allow writing arbitrary sections
>>
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
>> written directly after their associated section.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> NOTE: This amend really just changes the order of the sections below and
>> may be dropped if not desired.
>
> does the order matter at all? the docs would imply that it doesn't,
> since there is a clear precedence between [global], [$type], and
> [$type.$id] (e.g., global -> client -> client.crash)..
No, it doesn't - I perhaps should've worded this a little more clearly;
this change is purely cosmetic and just makes it so each '[$type]' is followed
by its respective '[$type.$id]'.
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#configuration-sections
>
>>
>> Changes v1 --> v2:
>> * new
>>
>> src/PVE/CephConfig.pm | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 86d3079..00b1f35 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -82,17 +82,20 @@ sub write_ceph_config {
>> };
>>
>> &$cond_write_sec('global');
>> +
>> &$cond_write_sec('client');
>> &$cond_write_sec('client\..*');
>>
>> &$cond_write_sec('mds');
>> - &$cond_write_sec('mon');
>> - &$cond_write_sec('osd');
>> - &$cond_write_sec('mgr');
>> -
>> &$cond_write_sec('mds\..*');
>> +
>> + &$cond_write_sec('mon');
>> &$cond_write_sec('mon\..*');
>> +
>> + &$cond_write_sec('osd');
>> &$cond_write_sec('osd\..*');
>> +
>> + &$cond_write_sec('mgr');
>> &$cond_write_sec('mgr\..*');
>>
>> &$cond_write_sec('.*');
>> --
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (6 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 07/11] amend! " Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:33 ` [pve-devel] applied: " Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service Max Carrara
` (2 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
Having a file named e.g. "60" in your current directory will cause it
to be deleted when executing `pveceph purge`. This commit fixes that
by making the config hash differ between which values represent file
paths and which don't.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:as
* use two hashes instead of one large nested hash to differ between
config values that correspond to files and adapt surrounding logic
correspondingly
PVE/Ceph/Tools.pm | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index a1458b40..273a3eb6 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -35,15 +35,18 @@ my $ceph_service = {
ceph_volume => '/usr/sbin/ceph-volume',
};
-my $config_hash = {
+my $config_values = {
ccname => $ccname,
+ ceph_mds_data_dir => $ceph_mds_data_dir,
+ long_rados_timeout => 60,
+};
+
+my $config_files = {
pve_ceph_cfgpath => $pve_ceph_cfgpath,
pve_mon_key_path => $pve_mon_key_path,
pve_ckeyring_path => $pve_ckeyring_path,
ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
- ceph_mds_data_dir => $ceph_mds_data_dir,
- long_rados_timeout => 60,
ceph_cfgpath => $ceph_cfgpath,
};
@@ -84,9 +87,12 @@ sub get_cluster_versions {
sub get_config {
my $key = shift;
- my $value = $config_hash->{$key};
+ my $value = $config_values->{$key};
+ if (! defined($value)) {
+ $value = $config_files->{$key};
+ }
- die "no such ceph config '$key'" if !$value;
+ die "no such ceph config '$key'" if ! defined($value);
return $value;
}
@@ -123,7 +129,7 @@ sub purge_all_ceph_files {
warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
} else {
print "Removing config & keyring files\n";
- foreach my $file (%$config_hash) {
+ for my $file (%$config_files) {
unlink $file if (-e $file);
}
}
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* [pve-devel] applied: [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge Max Carrara
@ 2024-02-12 13:33 ` Fabian Grünbichler
0 siblings, 0 replies; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:33 UTC (permalink / raw)
To: Proxmox VE development discussion
applied this one with a small style follow-up:
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 273a3eb63..ee6c515cb 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -87,10 +87,7 @@ sub get_cluster_versions {
sub get_config {
my $key = shift;
- my $value = $config_values->{$key};
- if (! defined($value)) {
- $value = $config_files->{$key};
- }
+ my $value = $config_values->{$key} // $config_files->{$key};
die "no such ceph config '$key'" if ! defined($value);
On February 5, 2024 6:54 pm, Max Carrara wrote:
> Having a file named e.g. "60" in your current directory will cause it
> to be deleted when executing `pveceph purge`. This commit fixes that
> by making the config hash differ between which values represent file
> paths and which don't.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:as
> * use two hashes instead of one large nested hash to differ between
> config values that correspond to files and adapt surrounding logic
> correspondingly
>
> PVE/Ceph/Tools.pm | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index a1458b40..273a3eb6 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -35,15 +35,18 @@ my $ceph_service = {
> ceph_volume => '/usr/sbin/ceph-volume',
> };
>
> -my $config_hash = {
> +my $config_values = {
> ccname => $ccname,
> + ceph_mds_data_dir => $ceph_mds_data_dir,
> + long_rados_timeout => 60,
> +};
> +
> +my $config_files = {
> pve_ceph_cfgpath => $pve_ceph_cfgpath,
> pve_mon_key_path => $pve_mon_key_path,
> pve_ckeyring_path => $pve_ckeyring_path,
> ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
> ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
> - ceph_mds_data_dir => $ceph_mds_data_dir,
> - long_rados_timeout => 60,
> ceph_cfgpath => $ceph_cfgpath,
> };
>
> @@ -84,9 +87,12 @@ sub get_cluster_versions {
> sub get_config {
> my $key = shift;
>
> - my $value = $config_hash->{$key};
> + my $value = $config_values->{$key};
> + if (! defined($value)) {
> + $value = $config_files->{$key};
> + }
>
> - die "no such ceph config '$key'" if !$value;
> + die "no such ceph config '$key'" if ! defined($value);
>
> return $value;
> }
> @@ -123,7 +129,7 @@ sub purge_all_ceph_files {
> warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
> } else {
> print "Removing config & keyring files\n";
> - foreach my $file (%$config_hash) {
> + for my $file (%$config_files) {
> unlink $file if (-e $file);
> }
> }
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (7 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:34 ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 10/11] ceph: create '/etc/pve/ceph' during `pveceph init` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key Max Carrara
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
when creating the cluster's first monitor.
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* do not enable/restart `ceph-crash` anymore when creating first mon
* drop changes to function `ceph_service_cmd` as they are no longer
needed
* create keyring for `ceph-crash` before modifying 'ceph.conf'
* always set keyring for 'client.crash' section instead of only
if section doesn't exist already
* only modify the keyring file in `get_or_create_crash_keyring()`
if the content differs from the output of `ceph auth get-or-create`
PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
PVE/Ceph/Tools.pm | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..ae12a2d3 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
});
die $@ if $@;
# automatically create manager after the first monitor is created
+ # and set up keyring and config for ceph-crash.service
if ($is_first_monitor) {
PVE::API2::Ceph::MGR->createmgr({
node => $param->{node},
id => $param->{node}
- })
+ });
+
+ eval {
+ PVE::Ceph::Tools::get_or_create_crash_keyring();
+ };
+ warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
+
+ PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
+ my $cfg = cfs_read_file('ceph.conf');
+
+ $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
+
+ cfs_write_file('ceph.conf', $cfg);
+ });
+ die $@ if $@;
}
};
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 273a3eb6..02a932e3 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
my $ceph_cfgdir = "/etc/ceph";
my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
+my $pve_ceph_cfgdir = "/etc/pve/ceph";
+my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
@@ -37,12 +39,14 @@ my $ceph_service = {
my $config_values = {
ccname => $ccname,
+ pve_ceph_cfgdir => $pve_ceph_cfgdir,
ceph_mds_data_dir => $ceph_mds_data_dir,
long_rados_timeout => 60,
};
my $config_files = {
pve_ceph_cfgpath => $pve_ceph_cfgpath,
+ pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
pve_mon_key_path => $pve_mon_key_path,
pve_ckeyring_path => $pve_ckeyring_path,
ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
@@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
return $pve_ckeyring_path;
}
+# requires connection to existing monitor
+sub get_or_create_crash_keyring {
+ my ($rados) = @_;
+
+ if (!defined($rados)) {
+ $rados = PVE::RADOS->new();
+ }
+
+ my $output = $rados->mon_command({
+ prefix => 'auth get-or-create',
+ entity => 'client.crash',
+ caps => [
+ mon => 'profile crash',
+ mgr => 'profile crash',
+ ],
+ format => 'plain',
+ });
+
+ if (! -d $pve_ceph_cfgdir) {
+ File::Path::make_path($pve_ceph_cfgdir);
+ }
+
+ if (-f $pve_ceph_crash_key_path) {
+ my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
+
+ if ($contents ne $output) {
+ PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+ }
+ } else {
+ PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+ }
+
+ return $pve_ceph_crash_key_path;
+}
+
# get ceph-volume managed osds
sub ceph_volume_list {
my $result = {};
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service Max Carrara
@ 2024-02-12 13:34 ` Fabian Grünbichler
2024-02-13 9:09 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:34 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> when creating the cluster's first monitor.
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
> * do not enable/restart `ceph-crash` anymore when creating first mon
> * drop changes to function `ceph_service_cmd` as they are no longer
> needed
> * create keyring for `ceph-crash` before modifying 'ceph.conf'
> * always set keyring for 'client.crash' section instead of only
> if section doesn't exist already
> * only modify the keyring file in `get_or_create_crash_keyring()`
> if the content differs from the output of `ceph auth get-or-create`
you kinda ignored my comment about this adding yet another create keyring helper ;)
> PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
> PVE/Ceph/Tools.pm | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..ae12a2d3 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
> });
> die $@ if $@;
> # automatically create manager after the first monitor is created
> + # and set up keyring and config for ceph-crash.service
> if ($is_first_monitor) {
> PVE::API2::Ceph::MGR->createmgr({
> node => $param->{node},
> id => $param->{node}
> - })
> + });
> +
> + eval {
> + PVE::Ceph::Tools::get_or_create_crash_keyring();
this is called get_or_create, but it actually returns the path to, not
the key(ring).. nothing uses the return value anyway, so it could also
be called differently I guess and not return anything, but just from the
name, I'd expect the key to be returned.
> + };
> + warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
> +
> + PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
> + my $cfg = cfs_read_file('ceph.conf');
> +
> + $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
I guess this doesn't make anything worse (since we starting from
"ceph-crash is totally broken"), but if querying or creating the keyring
failed, does it make sense to reference it in the config?
> +
> + cfs_write_file('ceph.conf', $cfg);
> + });
> + die $@ if $@;
we could move this whole part up to where we do the monitor changes, and
only lock and read and write the config once..
> }
> };
>
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 273a3eb6..02a932e3 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
> my $ceph_cfgdir = "/etc/ceph";
> my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
> my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>
> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
> my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
> my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
> my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
> @@ -37,12 +39,14 @@ my $ceph_service = {
>
> my $config_values = {
> ccname => $ccname,
> + pve_ceph_cfgdir => $pve_ceph_cfgdir,
> ceph_mds_data_dir => $ceph_mds_data_dir,
> long_rados_timeout => 60,
> };
>
> my $config_files = {
> pve_ceph_cfgpath => $pve_ceph_cfgpath,
> + pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
> pve_mon_key_path => $pve_mon_key_path,
> pve_ckeyring_path => $pve_ckeyring_path,
> ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
> return $pve_ckeyring_path;
> }
>
> +# requires connection to existing monitor
> +sub get_or_create_crash_keyring {
> + my ($rados) = @_;
> +
> + if (!defined($rados)) {
> + $rados = PVE::RADOS->new();
> + }
> +
> + my $output = $rados->mon_command({
> + prefix => 'auth get-or-create',
> + entity => 'client.crash',
> + caps => [
> + mon => 'profile crash',
> + mgr => 'profile crash',
> + ],
> + format => 'plain',
> + });
> +
> + if (! -d $pve_ceph_cfgdir) {
> + File::Path::make_path($pve_ceph_cfgdir);
> + }
> +
> + if (-f $pve_ceph_crash_key_path) {
> + my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
> +
> + if ($contents ne $output) {
> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> + }
> + } else {
> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> + }
> +
> + return $pve_ceph_crash_key_path;
> +}
> +
> # get ceph-volume managed osds
> sub ceph_volume_list {
> my $result = {};
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service
2024-02-12 13:34 ` Fabian Grünbichler
@ 2024-02-13 9:09 ` Max Carrara
2024-02-14 12:43 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-13 9:09 UTC (permalink / raw)
To: pve-devel
On 2/12/24 14:34, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> when creating the cluster's first monitor.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>> * do not enable/restart `ceph-crash` anymore when creating first mon
>> * drop changes to function `ceph_service_cmd` as they are no longer
>> needed
>> * create keyring for `ceph-crash` before modifying 'ceph.conf'
>> * always set keyring for 'client.crash' section instead of only
>> if section doesn't exist already
>> * only modify the keyring file in `get_or_create_crash_keyring()`
>> if the content differs from the output of `ceph auth get-or-create`
>
> you kinda ignored my comment about this adding yet another create keyring helper ;)
Saw your other reply - will disregard this as noted ;)
>
>> PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
>> PVE/Ceph/Tools.pm | 39 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>> index 1e959ef3..ae12a2d3 100644
>> --- a/PVE/API2/Ceph/MON.pm
>> +++ b/PVE/API2/Ceph/MON.pm
>> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>> });
>> die $@ if $@;
>> # automatically create manager after the first monitor is created
>> + # and set up keyring and config for ceph-crash.service
>> if ($is_first_monitor) {
>> PVE::API2::Ceph::MGR->createmgr({
>> node => $param->{node},
>> id => $param->{node}
>> - })
>> + });
>> +
>> + eval {
>> + PVE::Ceph::Tools::get_or_create_crash_keyring();
>
> this is called get_or_create, but it actually returns the path to, not
> the key(ring).. nothing uses the return value anyway, so it could also
> be called differently I guess and not return anything, but just from the
> name, I'd expect the key to be returned.
I agree; I initially wanted this helper to be similar to `get_or_create_ceph_admin_keyring`,
but in this case I'll just unify the helper(s) once and for all in v3.
No need to beat around the bush if I'm gonna unify them anyway, so might
as well do it sooner than later ;)
>
>> + };
>> + warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
>> +
>> + PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
>> + my $cfg = cfs_read_file('ceph.conf');
>> +
>> + $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
>
> I guess this doesn't make anything worse (since we starting from
> "ceph-crash is totally broken"), but if querying or creating the keyring
> failed, does it make sense to reference it in the config?
I guess in this case it doesn't, but we would need some kind of way to
re-init this part of the configuration in case the 'keyring' key doesn't
get set here.
Originally I planned to do this in a separate series to keep the scope of
this series focused on bug #4759, so perhaps it's best (at least for the
time being) to just not set the 'keyring' at all at the moment if querying
or creating the crash key fails.
So, I would change it to that in v3 and then later on supply a separate
series for the 're-init' part, if that's alright.
>
>> +
>> + cfs_write_file('ceph.conf', $cfg);
>> + });
>> + die $@ if $@;
>
> we could move this whole part up to where we do the monitor changes, and
> only lock and read and write the config once..
Good point, will be done in v3.
Thanks again for the thorough reviews! :)
>
>> }
>> };
>>
>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>> index 273a3eb6..02a932e3 100644
>> --- a/PVE/Ceph/Tools.pm
>> +++ b/PVE/Ceph/Tools.pm
>> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
>> my $ceph_cfgdir = "/etc/ceph";
>> my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
>> my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
>> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>>
>> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
>> my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>> my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>> my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
>> @@ -37,12 +39,14 @@ my $ceph_service = {
>>
>> my $config_values = {
>> ccname => $ccname,
>> + pve_ceph_cfgdir => $pve_ceph_cfgdir,
>> ceph_mds_data_dir => $ceph_mds_data_dir,
>> long_rados_timeout => 60,
>> };
>>
>> my $config_files = {
>> pve_ceph_cfgpath => $pve_ceph_cfgpath,
>> + pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
>> pve_mon_key_path => $pve_mon_key_path,
>> pve_ckeyring_path => $pve_ckeyring_path,
>> ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
>> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>> return $pve_ckeyring_path;
>> }
>>
>> +# requires connection to existing monitor
>> +sub get_or_create_crash_keyring {
>> + my ($rados) = @_;
>> +
>> + if (!defined($rados)) {
>> + $rados = PVE::RADOS->new();
>> + }
>> +
>> + my $output = $rados->mon_command({
>> + prefix => 'auth get-or-create',
>> + entity => 'client.crash',
>> + caps => [
>> + mon => 'profile crash',
>> + mgr => 'profile crash',
>> + ],
>> + format => 'plain',
>> + });
>> +
>> + if (! -d $pve_ceph_cfgdir) {
>> + File::Path::make_path($pve_ceph_cfgdir);
>> + }
>> +
>> + if (-f $pve_ceph_crash_key_path) {
>> + my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
>> +
>> + if ($contents ne $output) {
>> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>> + }
>> + } else {
>> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>> + }
>> +
>> + return $pve_ceph_crash_key_path;
>> +}
>> +
>> # get ceph-volume managed osds
>> sub ceph_volume_list {
>> my $result = {};
>> --
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service
2024-02-13 9:09 ` Max Carrara
@ 2024-02-14 12:43 ` Max Carrara
0 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-14 12:43 UTC (permalink / raw)
To: pve-devel
On 2/13/24 10:09, Max Carrara wrote:
> On 2/12/24 14:34, Fabian Grünbichler wrote:
>> On February 5, 2024 6:54 pm, Max Carrara wrote:
>>> when creating the cluster's first monitor.
>>>
>>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>>> ---
>>> Changes v1 --> v2:
>>> * do not enable/restart `ceph-crash` anymore when creating first mon
>>> * drop changes to function `ceph_service_cmd` as they are no longer
>>> needed
>>> * create keyring for `ceph-crash` before modifying 'ceph.conf'
>>> * always set keyring for 'client.crash' section instead of only
>>> if section doesn't exist already
>>> * only modify the keyring file in `get_or_create_crash_keyring()`
>>> if the content differs from the output of `ceph auth get-or-create`
>>
>> you kinda ignored my comment about this adding yet another create keyring helper ;)
>
> Saw your other reply - will disregard this as noted ;)
>
>>
>>> PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
>>> PVE/Ceph/Tools.pm | 39 +++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>>> index 1e959ef3..ae12a2d3 100644
>>> --- a/PVE/API2/Ceph/MON.pm
>>> +++ b/PVE/API2/Ceph/MON.pm
>>> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>>> });
>>> die $@ if $@;
>>> # automatically create manager after the first monitor is created
>>> + # and set up keyring and config for ceph-crash.service
>>> if ($is_first_monitor) {
>>> PVE::API2::Ceph::MGR->createmgr({
>>> node => $param->{node},
>>> id => $param->{node}
>>> - })
>>> + });
>>> +
>>> + eval {
>>> + PVE::Ceph::Tools::get_or_create_crash_keyring();
>>
>> this is called get_or_create, but it actually returns the path to, not
>> the key(ring).. nothing uses the return value anyway, so it could also
>> be called differently I guess and not return anything, but just from the
>> name, I'd expect the key to be returned.
>
> I agree; I initially wanted this helper to be similar to `get_or_create_ceph_admin_keyring`,
> but in this case I'll just unify the helper(s) once and for all in v3.
>
> No need to beat around the bush if I'm gonna unify them anyway, so might
> as well do it sooner than later ;)
Now that I've taken a more detailed look at all the existing code, in particular
usages of `ceph auth get-or-create` (via RADOS) and `ceph-authtool` (as command),
it's become clear to me that there's no actual benefit in creating an auth-helper
in that regard *at all*.
`ceph auth get-or-create` is only used via RADOS (`$rados->mon_command(...)`)
and `ceph-authtool` is only invoked in three places, each with separate use cases.
So, creating the keyring for 'client.crash' is (yet) an(other) exception rather
than something that benefits from a helper, so in this case, I'll think of a
different solution.
>
>>
>>> + };
>>> + warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
>>> +
>>> + PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
>>> + my $cfg = cfs_read_file('ceph.conf');
>>> +
>>> + $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
>>
>> I guess this doesn't make anything worse (since we starting from
>> "ceph-crash is totally broken"), but if querying or creating the keyring
>> failed, does it make sense to reference it in the config?
>
> I guess in this case it doesn't, but we would need some kind of way to
> re-init this part of the configuration in case the 'keyring' key doesn't
> get set here.
>
> Originally I planned to do this in a separate series to keep the scope of
> this series focused on bug #4759, so perhaps it's best (at least for the
> time being) to just not set the 'keyring' at all at the moment if querying
> or creating the crash key fails.
>
> So, I would change it to that in v3 and then later on supply a separate
> series for the 're-init' part, if that's alright.
>
>>
>>> +
>>> + cfs_write_file('ceph.conf', $cfg);
>>> + });
>>> + die $@ if $@;
>>
>> we could move this whole part up to where we do the monitor changes, and
>> only lock and read and write the config once..
>
> Good point, will be done in v3.
>
> Thanks again for the thorough reviews! :)
>
>>
>>> }
>>> };
>>>
>>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>>> index 273a3eb6..02a932e3 100644
>>> --- a/PVE/Ceph/Tools.pm
>>> +++ b/PVE/Ceph/Tools.pm
>>> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
>>> my $ceph_cfgdir = "/etc/ceph";
>>> my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
>>> my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
>>> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>>>
>>> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
>>> my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>>> my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>>> my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
>>> @@ -37,12 +39,14 @@ my $ceph_service = {
>>>
>>> my $config_values = {
>>> ccname => $ccname,
>>> + pve_ceph_cfgdir => $pve_ceph_cfgdir,
>>> ceph_mds_data_dir => $ceph_mds_data_dir,
>>> long_rados_timeout => 60,
>>> };
>>>
>>> my $config_files = {
>>> pve_ceph_cfgpath => $pve_ceph_cfgpath,
>>> + pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
>>> pve_mon_key_path => $pve_mon_key_path,
>>> pve_ckeyring_path => $pve_ckeyring_path,
>>> ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
>>> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>>> return $pve_ckeyring_path;
>>> }
>>>
>>> +# requires connection to existing monitor
>>> +sub get_or_create_crash_keyring {
>>> + my ($rados) = @_;
>>> +
>>> + if (!defined($rados)) {
>>> + $rados = PVE::RADOS->new();
>>> + }
>>> +
>>> + my $output = $rados->mon_command({
>>> + prefix => 'auth get-or-create',
>>> + entity => 'client.crash',
>>> + caps => [
>>> + mon => 'profile crash',
>>> + mgr => 'profile crash',
>>> + ],
>>> + format => 'plain',
>>> + });
>>> +
>>> + if (! -d $pve_ceph_cfgdir) {
>>> + File::Path::make_path($pve_ceph_cfgdir);
>>> + }
>>> +
>>> + if (-f $pve_ceph_crash_key_path) {
>>> + my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
>>> +
>>> + if ($contents ne $output) {
>>> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>>> + }
>>> + } else {
>>> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
>>> + }
>>> +
>>> + return $pve_ceph_crash_key_path;
>>> +}
>>> +
>>> # get ceph-volume managed osds
>>> sub ceph_volume_list {
>>> my $result = {};
>>> --
>>> 2.39.2
>>>
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [pve-devel] [PATCH v2 pve-manager 10/11] ceph: create '/etc/pve/ceph' during `pveceph init`
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (8 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key Max Carrara
10 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* none
PVE/API2/Ceph.pm | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index 81c17d6e..7fedb87a 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -192,6 +192,11 @@ __PACKAGE__->register_method ({
PVE::Ceph::Tools::check_ceph_installed('ceph_bin');
}
+ my $pve_ceph_cfgdir = PVE::Ceph::Tools::get_config('pve_ceph_cfgdir');
+ if (! -d $pve_ceph_cfgdir) {
+ File::Path::make_path($pve_ceph_cfgdir);
+ }
+
my $auth = $param->{disable_cephx} ? 'none' : 'cephx';
# simply load old config if it already exists
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (9 preceding siblings ...)
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 10/11] ceph: create '/etc/pve/ceph' during `pveceph init` Max Carrara
@ 2024-02-05 17:54 ` Max Carrara
2024-02-12 13:34 ` Fabian Grünbichler
10 siblings, 1 reply; 29+ messages in thread
From: Max Carrara @ 2024-02-05 17:54 UTC (permalink / raw)
To: pve-devel
This commit adds the `set_ceph_crash_conf` function, which dynamically
adapts the host's Ceph configuration in order to allow the Ceph crash
module's daemon to run without elevated privileges.
This adaptation is only performed if:
* Ceph is installed
* Ceph is configured ('/etc/pve/ceph.conf' exists)
* Connection to RADOS is successful
If the above conditions are met, the function will ensure that:
* Ceph possesses a key named 'client.crash'
* The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
* A section for 'client.crash' exists in '/etc/pve/ceph.conf'
* The 'client.crash' section has a key named 'keyring' which
references '/etc/pve/ceph/ceph.client.crash.keyring'
Furthermore, if a key named 'client.crash' already exists within the
cluster, it shall be reused and not regenerated. Also, the
configuration is not altered if the conditions above are already met.
This way the keyring file is available as read-only in
'/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
Because the `ceph` user has been made part of said `www-data` group
[0], it may access the file without requiring any additional
privileges.
Thus, the configuration for the Ceph crash daemon is safely adapted as
expected by PVE tooling and also shared via pmxcfs across one's
cluster.
[0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
* fix 'keyring' key being appended to 'client.crash' section even
if it already exists and configured correctly
debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
diff --git a/debian/postinst b/debian/postinst
index 6138ef6d..267a62ae 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
fi
}
+set_ceph_crash_conf() {
+ PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
+ PVE_CEPH_CONFDIR='/etc/pve/ceph'
+ PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
+ PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
+
+ # ceph isn't installed -> nothing to do
+ if ! which ceph > /dev/null 2>&1; then
+ return 0
+ fi
+
+ # ceph isn't configured -> nothing to do
+ if test ! -f "${PVE_CEPH_CONFFILE}"; then
+ return 0
+ fi
+
+ CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
+
+ # ceph is installed and possibly configured, but no connection to RADOS
+ # -> assume no monitor was created, nothing to do
+ if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
+ return 0
+ fi
+
+ SECTION_RE='^\[\S+\]$'
+ CRASH_SECTION_RE='^\[client\.crash\]$'
+
+ if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
+ DO_RESTART_UNIT=0
+ CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
+
+ if test ! -d "${PVE_CEPH_CONFDIR}"; then
+ mkdir -p "${PVE_CEPH_CONFDIR}"
+ fi
+
+ # keyring file doesn't exist or contains wrong key
+ if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
+ echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
+ echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
+ DO_RESTART_UNIT=1
+ fi
+
+ # 'client.crash' section is in conf file
+ if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
+ IFS=''
+ NEW_PVE_CEPH_CONFFILE=''
+ IN_CRASH_SECTION=0
+ HAS_KEYRING=0
+ REPLACED_KEYRING=0
+
+ # look for 'keyring' key in 'client.crash' section
+ # -> replace it if it points to the wrong location
+ while read -r LINE; do
+ if test "${IN_CRASH_SECTION}" = "1"; then
+ if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
+ IN_CRASH_SECTION=0
+ elif echo "${LINE}" | grep -q -E '\s+keyring'; then
+ HAS_KEYRING=1
+
+ if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
+ echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
+ LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
+ REPLACED_KEYRING=1
+ fi
+ fi
+ elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
+ IN_CRASH_SECTION=1
+ fi
+
+ NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
+ done < "${PVE_CEPH_CONFFILE}"
+
+ unset IFS
+
+ if test "${HAS_KEYRING}" = "1"; then
+ # 'keyring' key was replaced -> write to file
+ if test "${REPLACED_KEYRING}" = "1"; then
+ echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
+ DO_RESTART_UNIT=1
+ fi
+
+ # client.crash section exists, but contained no 'keyring' key
+ # -> put 'keyring' key into 'client.crash' section
+ else
+ sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
+ "${PVE_CEPH_CONFFILE}"
+ DO_RESTART_UNIT=1
+ fi
+
+ # 'client.crash' section doesn't exist -> add it
+ else
+ echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
+ printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
+ >> "${PVE_CEPH_CONFFILE}"
+ DO_RESTART_UNIT=1
+ fi
+
+ if test "${DO_RESTART_UNIT}" = "1"; then
+ UNIT='ceph-crash.service'
+
+ if systemctl -q is-enabled "${UNIT}"; then
+ echo "Restarting ceph-crash.service"
+ deb-systemd-invoke restart "${UNIT}"
+ fi
+ fi
+
+ else
+ echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
+ printf '%s\n\n' "${CEPH_AUTH_RES}"
+ fi
+}
+
case "$1" in
triggered)
# We don't print a status message here, as dpkg already said
@@ -189,6 +301,7 @@ case "$1" in
fi
set_lvm_conf
+ set_ceph_crash_conf
if test ! -e /proxmox_install_mode; then
# modeled after code generated by dh_start
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key Max Carrara
@ 2024-02-12 13:34 ` Fabian Grünbichler
2024-02-13 9:25 ` Max Carrara
0 siblings, 1 reply; 29+ messages in thread
From: Fabian Grünbichler @ 2024-02-12 13:34 UTC (permalink / raw)
To: Proxmox VE development discussion
On February 5, 2024 6:54 pm, Max Carrara wrote:
> This commit adds the `set_ceph_crash_conf` function, which dynamically
> adapts the host's Ceph configuration in order to allow the Ceph crash
> module's daemon to run without elevated privileges.
>
> This adaptation is only performed if:
> * Ceph is installed
> * Ceph is configured ('/etc/pve/ceph.conf' exists)
> * Connection to RADOS is successful
>
> If the above conditions are met, the function will ensure that:
> * Ceph possesses a key named 'client.crash'
> * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
> * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
> * The 'client.crash' section has a key named 'keyring' which
> references '/etc/pve/ceph/ceph.client.crash.keyring'
>
> Furthermore, if a key named 'client.crash' already exists within the
> cluster, it shall be reused and not regenerated. Also, the
> configuration is not altered if the conditions above are already met.
>
> This way the keyring file is available as read-only in
> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
> Because the `ceph` user has been made part of said `www-data` group
> [0], it may access the file without requiring any additional
> privileges.
>
> Thus, the configuration for the Ceph crash daemon is safely adapted as
> expected by PVE tooling and also shared via pmxcfs across one's
> cluster.
I still don't think this is a good idea, even a simple perl -e '..'
invocation or two (or a small helper script that doesn't live in $PATH)
for doing the two steps we want (initialize key if missing, lock+modify
config if key was missing) would be better (although compared to the
"hidden" or regular command approach, it has the downside that somebody
might miss the calls here when refactoring),
among other things the code below
- doesn't lock /etc/pve/ceph.conf but modifies it
- implements yet another broken parser for ceph.conf (e.g., it doesn't
handle the stuff you fix in the perl variant in this series!)
- duplicates constants from the perl code that risk running out of sync,
like paths or the key profile
- still has issues that you fixed in the perl code between v1 and v2
(restarting services)
I haven't reviewed the bash code in detail for that reason!
another issue - IMHO this should be version-guarded, since any new setup
would already gain it when setting up a monitor, and we avoid access to
pmxcfs in the upgrade hot path which can cause problems (cluster
non-quorate, ..).
>
> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
> * fix 'keyring' key being appended to 'client.crash' section even
> if it already exists and configured correctly
>
> debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 113 insertions(+)
>
> diff --git a/debian/postinst b/debian/postinst
> index 6138ef6d..267a62ae 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
> fi
> }
>
> +set_ceph_crash_conf() {
> + PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
> + PVE_CEPH_CONFDIR='/etc/pve/ceph'
> + PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
> + PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
> +
> + # ceph isn't installed -> nothing to do
> + if ! which ceph > /dev/null 2>&1; then
> + return 0
> + fi
> +
> + # ceph isn't configured -> nothing to do
> + if test ! -f "${PVE_CEPH_CONFFILE}"; then
> + return 0
> + fi
> +
> + CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
> +
> + # ceph is installed and possibly configured, but no connection to RADOS
> + # -> assume no monitor was created, nothing to do
> + if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
> + return 0
> + fi
> +
> + SECTION_RE='^\[\S+\]$'
> + CRASH_SECTION_RE='^\[client\.crash\]$'
> +
> + if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
> + DO_RESTART_UNIT=0
> + CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
> +
> + if test ! -d "${PVE_CEPH_CONFDIR}"; then
> + mkdir -p "${PVE_CEPH_CONFDIR}"
> + fi
> +
> + # keyring file doesn't exist or contains wrong key
> + if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
> + echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
> + echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
> + DO_RESTART_UNIT=1
> + fi
> +
> + # 'client.crash' section is in conf file
> + if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
> + IFS=''
> + NEW_PVE_CEPH_CONFFILE=''
> + IN_CRASH_SECTION=0
> + HAS_KEYRING=0
> + REPLACED_KEYRING=0
> +
> + # look for 'keyring' key in 'client.crash' section
> + # -> replace it if it points to the wrong location
> + while read -r LINE; do
> + if test "${IN_CRASH_SECTION}" = "1"; then
> + if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
> + IN_CRASH_SECTION=0
> + elif echo "${LINE}" | grep -q -E '\s+keyring'; then
> + HAS_KEYRING=1
> +
> + if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
> + echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
> + LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
> + REPLACED_KEYRING=1
> + fi
> + fi
> + elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
> + IN_CRASH_SECTION=1
> + fi
> +
> + NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
> + done < "${PVE_CEPH_CONFFILE}"
> +
> + unset IFS
> +
> + if test "${HAS_KEYRING}" = "1"; then
> + # 'keyring' key was replaced -> write to file
> + if test "${REPLACED_KEYRING}" = "1"; then
> + echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
> + DO_RESTART_UNIT=1
> + fi
> +
> + # client.crash section exists, but contained no 'keyring' key
> + # -> put 'keyring' key into 'client.crash' section
> + else
> + sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
> + "${PVE_CEPH_CONFFILE}"
> + DO_RESTART_UNIT=1
> + fi
> +
> + # 'client.crash' section doesn't exist -> add it
> + else
> + echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
> + printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
> + >> "${PVE_CEPH_CONFFILE}"
> + DO_RESTART_UNIT=1
> + fi
> +
> + if test "${DO_RESTART_UNIT}" = "1"; then
> + UNIT='ceph-crash.service'
> +
> + if systemctl -q is-enabled "${UNIT}"; then
> + echo "Restarting ceph-crash.service"
> + deb-systemd-invoke restart "${UNIT}"
> + fi
> + fi
> +
> + else
> + echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
> + printf '%s\n\n' "${CEPH_AUTH_RES}"
> + fi
> +}
> +
> case "$1" in
> triggered)
> # We don't print a status message here, as dpkg already said
> @@ -189,6 +301,7 @@ case "$1" in
> fi
>
> set_lvm_conf
> + set_ceph_crash_conf
>
> if test ! -e /proxmox_install_mode; then
> # modeled after code generated by dh_start
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key
2024-02-12 13:34 ` Fabian Grünbichler
@ 2024-02-13 9:25 ` Max Carrara
0 siblings, 0 replies; 29+ messages in thread
From: Max Carrara @ 2024-02-13 9:25 UTC (permalink / raw)
To: pve-devel
On 2/12/24 14:34, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> This commit adds the `set_ceph_crash_conf` function, which dynamically
>> adapts the host's Ceph configuration in order to allow the Ceph crash
>> module's daemon to run without elevated privileges.
>>
>> This adaptation is only performed if:
>> * Ceph is installed
>> * Ceph is configured ('/etc/pve/ceph.conf' exists)
>> * Connection to RADOS is successful
>>
>> If the above conditions are met, the function will ensure that:
>> * Ceph possesses a key named 'client.crash'
>> * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>> * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>> * The 'client.crash' section has a key named 'keyring' which
>> references '/etc/pve/ceph/ceph.client.crash.keyring'
>>
>> Furthermore, if a key named 'client.crash' already exists within the
>> cluster, it shall be reused and not regenerated. Also, the
>> configuration is not altered if the conditions above are already met.
>>
>> This way the keyring file is available as read-only in
>> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
>> Because the `ceph` user has been made part of said `www-data` group
>> [0], it may access the file without requiring any additional
>> privileges.
>>
>> Thus, the configuration for the Ceph crash daemon is safely adapted as
>> expected by PVE tooling and also shared via pmxcfs across one's
>> cluster.
>
> I still don't think this is a good idea, even a simple perl -e '..'
> invocation or two (or a small helper script that doesn't live in $PATH)
> for doing the two steps we want (initialize key if missing, lock+modify
> config if key was missing) would be better (although compared to the
> "hidden" or regular command approach, it has the downside that somebody
> might miss the calls here when refactoring),
>
> among other things the code below
> - doesn't lock /etc/pve/ceph.conf but modifies it
> - implements yet another broken parser for ceph.conf (e.g., it doesn't
> handle the stuff you fix in the perl variant in this series!)
> - duplicates constants from the perl code that risk running out of sync,
> like paths or the key profile
> - still has issues that you fixed in the perl code between v1 and v2
> (restarting services)
>
> I haven't reviewed the bash code in detail for that reason!
>
> another issue - IMHO this should be version-guarded, since any new setup
> would already gain it when setting up a monitor, and we avoid access to
> pmxcfs in the upgrade hot path which can cause problems (cluster
> non-quorate, ..).
I hadn't considered the points you mentioned above - I agree with all of them,
actually. I'll see if I can rewrite all this in Perl (would probably be easier
than BASH anyway).
As discussed off-list, instead of writing an inline Perl script, a separate
helper script (as an executable) would probably be better - a sensible place
for this script would be in '/usr/share/pve-manager/helpers'. That way we
can also point users to this script if something goes wrong during the update.
>
>>
>> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>> * fix 'keyring' key being appended to 'client.crash' section even
>> if it already exists and configured correctly
>>
>> debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 113 insertions(+)
>>
>> diff --git a/debian/postinst b/debian/postinst
>> index 6138ef6d..267a62ae 100755
>> --- a/debian/postinst
>> +++ b/debian/postinst
>> @@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
>> fi
>> }
>>
>> +set_ceph_crash_conf() {
>> + PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
>> + PVE_CEPH_CONFDIR='/etc/pve/ceph'
>> + PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
>> + PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
>> +
>> + # ceph isn't installed -> nothing to do
>> + if ! which ceph > /dev/null 2>&1; then
>> + return 0
>> + fi
>> +
>> + # ceph isn't configured -> nothing to do
>> + if test ! -f "${PVE_CEPH_CONFFILE}"; then
>> + return 0
>> + fi
>> +
>> + CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
>> +
>> + # ceph is installed and possibly configured, but no connection to RADOS
>> + # -> assume no monitor was created, nothing to do
>> + if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
>> + return 0
>> + fi
>> +
>> + SECTION_RE='^\[\S+\]$'
>> + CRASH_SECTION_RE='^\[client\.crash\]$'
>> +
>> + if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
>> + DO_RESTART_UNIT=0
>> + CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
>> +
>> + if test ! -d "${PVE_CEPH_CONFDIR}"; then
>> + mkdir -p "${PVE_CEPH_CONFDIR}"
>> + fi
>> +
>> + # keyring file doesn't exist or contains wrong key
>> + if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
>> + echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
>> + echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
>> + DO_RESTART_UNIT=1
>> + fi
>> +
>> + # 'client.crash' section is in conf file
>> + if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
>> + IFS=''
>> + NEW_PVE_CEPH_CONFFILE=''
>> + IN_CRASH_SECTION=0
>> + HAS_KEYRING=0
>> + REPLACED_KEYRING=0
>> +
>> + # look for 'keyring' key in 'client.crash' section
>> + # -> replace it if it points to the wrong location
>> + while read -r LINE; do
>> + if test "${IN_CRASH_SECTION}" = "1"; then
>> + if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
>> + IN_CRASH_SECTION=0
>> + elif echo "${LINE}" | grep -q -E '\s+keyring'; then
>> + HAS_KEYRING=1
>> +
>> + if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
>> + echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
>> + LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
>> + REPLACED_KEYRING=1
>> + fi
>> + fi
>> + elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
>> + IN_CRASH_SECTION=1
>> + fi
>> +
>> + NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
>> + done < "${PVE_CEPH_CONFFILE}"
>> +
>> + unset IFS
>> +
>> + if test "${HAS_KEYRING}" = "1"; then
>> + # 'keyring' key was replaced -> write to file
>> + if test "${REPLACED_KEYRING}" = "1"; then
>> + echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
>> + DO_RESTART_UNIT=1
>> + fi
>> +
>> + # client.crash section exists, but contained no 'keyring' key
>> + # -> put 'keyring' key into 'client.crash' section
>> + else
>> + sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
>> + "${PVE_CEPH_CONFFILE}"
>> + DO_RESTART_UNIT=1
>> + fi
>> +
>> + # 'client.crash' section doesn't exist -> add it
>> + else
>> + echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
>> + printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
>> + >> "${PVE_CEPH_CONFFILE}"
>> + DO_RESTART_UNIT=1
>> + fi
>> +
>> + if test "${DO_RESTART_UNIT}" = "1"; then
>> + UNIT='ceph-crash.service'
>> +
>> + if systemctl -q is-enabled "${UNIT}"; then
>> + echo "Restarting ceph-crash.service"
>> + deb-systemd-invoke restart "${UNIT}"
>> + fi
>> + fi
>> +
>> + else
>> + echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
>> + printf '%s\n\n' "${CEPH_AUTH_RES}"
>> + fi
>> +}
>> +
>> case "$1" in
>> triggered)
>> # We don't print a status message here, as dpkg already said
>> @@ -189,6 +301,7 @@ case "$1" in
>> fi
>>
>> set_lvm_conf
>> + set_ceph_crash_conf
>>
>> if test ! -e /proxmox_install_mode; then
>> # modeled after code generated by dh_start
>> --
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread