debian/*.postinst: add adduser as a dependency and specify --home when adduser #55218

tchaikov · 2024-01-17T16:10:15Z

in this changeset, we

add adduser as the runtime dependency of ceph-common and cephadm
pass --home to adduser in the postinst script to accommodate the recent change in this tool

Fixes: https://tracker.ceph.com/issues/64069

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

mcv21 · 2024-01-17T16:23:28Z

I'm slightly confused by the whitespace changes, but the substantive changes here look right to me. Thanks for getting to this more quickly than I did! :)

tchaikov · 2024-01-18T03:11:55Z

I'm slightly confused by the whitespace changes, but the substantive changes here look right to me. Thanks for getting to this more quickly than I did! :)

@mcv21 hi Matthew, thank you for your review. regarding the "the white space changes", please check its commit message. my editor does not use 8 spaces when rendering tab by default, hence i noticed this inconsistency. and since i was editing that file, i fixed it as well.

tchaikov · 2024-01-18T03:16:43Z

note to myself, please reference the tracker ticket.

tchaikov · 2024-01-18T11:22:27Z

build-tested at https://shaman.ceph.com/builds/ceph/kefu-debian-postinst-1/6b6e81398c4958477fd92cba5671c217b70a0a08/

mcv21 · 2024-01-18T11:51:51Z

build-tested at https://shaman.ceph.com/builds/ceph/kefu-debian-postinst-1/6b6e81398c4958477fd92cba5671c217b70a0a08/

At the risk of opening a can of worms, would it be possible to build-test on Debian too?

tchaikov · 2024-01-18T12:18:18Z

build-tested at https://shaman.ceph.com/builds/ceph/kefu-debian-postinst-1/6b6e81398c4958477fd92cba5671c217b70a0a08/

At the risk of opening a can of worms, would it be possible to build-test on Debian too?

@mcv21 i am building the debian packages using a sid pbuilder env, is this good enough? or you prefer building it in a bookworm pbuilder env?

mcv21 · 2024-01-18T12:28:55Z

sid is probably OK, though I think bookworm is better given that's the release we're targetting; I'd just not seen any Debian in the shaman output you linked.

tchaikov · 2024-01-18T14:18:58Z

[ 13%] Building CXX object src/msg/CMakeFiles/common-msg-objs.dir/async/Stack.cc.o
cd /var/ssd/ceph/obj-x86_64-linux-gnu/src/msg && /usr/bin/c++ -DBOOST_ASIO_DISABLE_THREAD_KEYWORD_EXTENSION -DBOOST_ASIO_HAS_IO_URING -DBOOST_ASIO_NO_TS_EXECUTORS -DHAVE_CONFIG_H -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_REENTRANT -D_THREAD_SAFE -D__CEPH__ -D__STDC_FORMAT_MACROS -D__linux__ -I/var/ssd/ceph/obj-x86_64-linux-gnu/src/include -I/var/ssd/ceph/src -isystem /var/ssd/ceph/obj-x86_64-linux-gnu/boost/include -isystem /var/ssd/ceph/obj-x86_64-linux-gnu/include -isystem /var/ssd/ceph/src/xxHash -isystem /var/ssd/ceph/src/fmt/include -g -O2 -ffile-prefix-map=/var/ssd/ceph=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection -Wdate-time -D_FORTIFY_SOURCE=2 -std=c++20 -fPIC   -U_FORTIFY_SOURCE -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -DBOOST_PHOENIX_STL_TUPLE_H_ -Wall -fno-strict-aliasing -fsigned-char -Wtype-limits -Wignored-qualifiers -Wpointer-arith -Werror=format-security -Winit-self -Wno-unknown-pragmas -Wnon-virtual-dtor -Wno-ignored-qualifiers -ftemplate-depth-1024 -Wpessimizing-move -Wredundant-move -Wstrict-null-sentinel -Woverloaded-virtual -fstack-protector-strong -fdiagnostics-color=auto -MD -MT src/msg/CMakeFiles/common-msg-objs.dir/async/Stack.cc.o -MF CMakeFiles/common-msg-objs.dir/async/Stack.cc.o.d -o CMakeFiles/common-msg-objs.dir/async/Stack.cc.o -c /var/ssd/ceph/src/msg/async/Stack.cc
In file included from /var/ssd/ceph/src/spawn/include/spawn/detail/net.hpp:16,
                 from /var/ssd/ceph/src/spawn/include/spawn/spawn.hpp:21,
                 from /var/ssd/ceph/src/common/async/yield_context.h:23,
                 from /var/ssd/ceph/src/crypto/openssl/openssl_crypto_accel.h:19,
                 from /var/ssd/ceph/src/crypto/openssl/openssl_crypto_plugin.h:19,
                 from /var/ssd/ceph/src/crypto/openssl/openssl_crypto_plugin.cc:16:
/var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/bind_executor.hpp: In instantiation of ‘class boost::asio::detail::executor_binder_base<void (*)(), boost::asio::executor, false>’:
/var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/bind_executor.hpp:209:7:   required from ‘class boost::asio::executor_binder<void (*)(), boost::asio::executor>’
/var/ssd/ceph/src/spawn/include/spawn/spawn.hpp:114:11:   required from ‘class spawn::basic_yield_context<boost::asio::executor_binder<void (*)(), boost::asio::executor> >’
/var/ssd/ceph/src/common/async/yield_context.h:36:17:   required from here
/var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/bind_executor.hpp:185:12: error: ‘boost::asio::detail::executor_binder_base<T, Executor, false>::executor_’ has incomplete type
  185 |   Executor executor_;
      |            ^~~~~~~~~
In file included from /var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/detail/completion_handler.hpp:21,
                 from /var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/impl/io_context.hpp:18,
                 from /var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/io_context.hpp:1523,
                 from /var/ssd/ceph/src/crypto/crypto_plugin.h:23,
                 from /var/ssd/ceph/src/crypto/openssl/openssl_crypto_plugin.h:18:
/var/ssd/ceph/obj-x86_64-linux-gnu/boost/include/boost/asio/detail/handler_work.hpp:39:7: note: forward declaration of ‘class boost::asio::executor’
   39 | class executor;
      |       ^~~~~~~~
make[3]: *** [src/crypto/openssl/CMakeFiles/ceph_crypto_openssl.dir/build.make:93: src/crypto/openssl/CMakeFiles/ceph_crypto_openssl.dir/openssl_crypto_plugin.cc.o] Error 1

FWIW, the latest tree fails to build on sid. i don't think it's related to the change in this PR though.

tchaikov · 2024-01-23T16:07:35Z

jenkins test api

tchaikov · 2024-01-23T16:33:13Z

@mcv21 i am afraid that i don't have enough bandwidth chasing the FTBFS issue. do you think it's a blocker? since ubuntu focal was released back in 2020-04, while bookworm 2023-06, the dependency changes should be safe. what do you think?

tchaikov · 2024-01-23T16:34:36Z

@ljflores hi Laura, could you please help review this change? i am not sure whom i should ping for review, if you are not, would you mind pinging the reviewer(s) who can help on this?

ljflores · 2024-01-23T16:41:29Z

@tchaikov Sure, will take a look

ljflores · 2024-01-23T16:44:49Z

Since this is an update to a cephadm package, one of the orch members (or @adk3798) would probably be best to review this.

adk3798

Just reviewing from orch side. Don't know the build stuff very well, especially debian specific build stuff, but seems good to me.

ljflores

From the core side of things, I would definitely like to have this go through integration testing. Logistically it makes sense, but I'm wondering if there is there anything specific that was done (or that can be done) to verify this change?

debian/cephadm.postinst

tchaikov · 2024-01-24T05:03:38Z

@tchaikov Sure, will take a look

thank you for your review and approval.

mcv21 · 2024-01-24T09:08:12Z

@mcv21 i am afraid that i don't have enough bandwidth chasing the FTBFS issue. do you think it's a blocker? since ubuntu focal was released back in 2020-04, while bookworm 2023-06, the dependency changes should be safe. what do you think?

It'll presumably need fixing at some point, but I can't see that it's related to this change (so I don't think it should block it).

Sitiritis · 2024-01-29T03:19:34Z

debian/cephadm.postinst

+                 --system \
+                 --disabled-password \
+                 --home /home/cephadm \
+                 --gecos 'cephadm user for mgr/cephadm' \


Hey, I am new to ceph, I was trying to install cephamd on Debian bookworm and ran into the problem this PR solves.

I was going through the Debian docs and found that they are going to remove --gecos option in the next release. Instead, --comment should be used. I see that for the other postinst script --comment is already used.

Does it make sense to use the new option for this script too as a part of this PR or should it be solved in a different ticket?

@Sitiritis hi Tymur, thank you for the reminder. since this change is being tested in lab, and i hope that this fix can be backported sooner. also, --gecos has not been deprecated, so guess we can live with it a little bit longer. but once this change lands, i will create another PR to s/--gecos/--comment/ to be more future-proof. so, "in a different ticket" or pull request. in general, we file a ticket so that it can be tracked and the corresponding fixes can be backported. if the --gecos does not cause any real-world issues yet, we could dispense a ticket on tracker.

@Sitiritis since our CI is still using ubuntu:jammy, and the adduser shipped along with it does not support --comment yet, i am not using --comment and since --gecos is going to be removed after bookworm, as you point out. i am adding a commit to use usermod directly. once we have the luxury of dropping the support of release before debian/trixie or ubuntu/mantic. we can start using adduser --comment. to be more future-proof, i will add a commit in this PR to switch to usermod.

Thank you :)
I just got access to ceph's redmine to create a ticket for this change, but it seems I don't need to do this anymore.

tchaikov · 2024-01-30T08:23:27Z

@mcv21 i am afraid that i don't have enough bandwidth chasing the FTBFS issue. do you think it's a blocker? since ubuntu focal was released back in 2020-04, while bookworm 2023-06, the dependency changes should be safe. what do you think?

It'll presumably need fixing at some point, but I can't see that it's related to this change (so I don't think it should block it).

@mcv21 thank you for confirming this. could you please elaborate a little bit on what will need fixing? since this change is being tested in lab. i will create a follow-up change if we need more changes.

NitzanMordhai · 2024-02-02T07:29:51Z

@tchaikov can you please take a look at a few of the following failures? it looks related to the new added adduser
https://pulpito.ceph.com/yuriw-2024-01-31_19:19:23-rados-wip-yuri7-testing-2024-01-29-1434-distro-default-smithi/7540563/

tchaikov · 2024-02-02T08:04:06Z

@NitzanMordhai thank you Nitzan! sure. will take a look this weekend.

mcv21 · 2024-02-02T17:36:22Z

@NitzanMordhai thank you Nitzan! sure. will take a look this weekend.

The failure is because the build is being run on Ubuntu 22.04, and adduser there does not support --comment 22.04 manual. By the 24.04 manual --gecos is deprecated, but I think it'll still work; so I think we have to punt moving to --comment until we no longer need to support 22.04.

mcv21 · 2024-02-02T17:41:18Z

@mcv21 i am afraid that i don't have enough bandwidth chasing the FTBFS issue. do you think it's a blocker? since ubuntu focal was released back in 2020-04, while bookworm 2023-06, the dependency changes should be safe. what do you think?

It'll presumably need fixing at some point, but I can't see that it's related to this change (so I don't think it should block it).

@mcv21 thank you for confirming this. could you please elaborate a little bit on what will need fixing? since this change is being tested in lab. i will create a follow-up change if we need more changes.

I'm afraid I don't really know - it looks like a problem building boost, but evidently boost can be built on Debian sid, so I don't know why it's failing here. [I guess this is the side-effect of so many submodules in the build rather than pulling libraries in from the distro, you end up with what might be entirely unrelated build failures]

tchaikov · 2024-02-03T14:08:23Z

@mcv21 i am afraid that i don't have enough bandwidth chasing the FTBFS issue. do you think it's a blocker? since ubuntu focal was released back in 2020-04, while bookworm 2023-06, the dependency changes should be safe. what do you think?

It'll presumably need fixing at some point, but I can't see that it's related to this change (so I don't think it should block it).

@mcv21 thank you for confirming this. could you please elaborate a little bit on what will need fixing? since this change is being tested in lab. i will create a follow-up change if we need more changes.

I'm afraid I don't really know - it looks like a problem building boost, but evidently boost can be built on Debian sid, so I don't know why it's failing here. [I guess this is the side-effect of so many submodules in the build rather than pulling libraries in from the distro, you end up with what might be entirely unrelated build failures]

i see. you were referencing the FTBFS i reported in this PR. let's leave it for a rainy weekend maybe.

and thank you for looking into the test failure!

tchaikov · 2024-02-03T14:09:30Z

changelog

do not use adduser --comment as the test fails on ubuntu jammy.
added a commit to switch from adduser --gecos to usermod --comment

in `debian/ceph-common.postinst` and `debian/cephadm.postinst`, we use `adduser --system` to create the system user when configuring the corresponding package. before this change, the dependency is not listed in the runtime `Depends` section of ceph-common and cephadm. in this change, the dependency is added. this is also suggested by Securing Debian Manual, see https://www.debian.org/doc/manuals/securing-debian-manual/bpp-lower-privs.en.html Signed-off-by: Kefu Chai <tchaikov@gmail.com>

now that adduser allows us to set its home directory, we can do this using adduser instead of using usermod. this change also silences the warning from lintian "maintainer-script-lacks-home-in-adduser". lintian complains if `adduser --system` is called without passing `--home` option. also, take this opportunity to s/-c/--comment/ in the command line of `usermod`, for better readability. Fixes: https://tracker.ceph.com/issues/64069 Signed-off-by: Kefu Chai <tchaikov@gmail.com>

for better readability. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

quote from adduser/NEWS.Debian.gz: > System user home defaults to /nonexistent if --home is not specified. > Packages that call adduser to create system accounts should explicitly > specify a location for /home (see Lintian check > maintainer-script-lacks-home-in-adduser). so let's follow this change in adduser. otherwise "cephadm" would have a $HOME at `/nonexistent`. Fixes: https://tracker.ceph.com/issues/64069 Signed-off-by: Kefu Chai <tchaikov@gmail.com>

for better readability, and to be more consistent with the rest of this file, and other .postinst scripts of this project. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

--gecos option of adduser is deprecated in debian/bookworm, and will be removed in debian/trixie, see https://manpages.debian.org/bookworm/adduser/adduser.8.en.html. so to be future-proof, let's switch to `usermod --comment`. please note, since we still need to support ubuntu/jammy which is used in our CI, and `adduser` shipped by ubuntu/jammy does not support `--comment` yet, so we cannot use this option. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

tchaikov · 2024-02-04T08:47:42Z

test passed at https://pulpito.ceph.com/kchai-2024-02-04_08:45:31-rados-wip-kefu-pr-55218-1-distro-default-smithi/

@ljflores @adk3798 hi Laura and Adam, thank you for your reviews. could you take another look?

mcv21 · 2024-02-20T16:17:01Z

@tchaikov thanks for working on this; are you OK to backport it to reef, please? :)
[I mean if not I can probably open a MR myself, but...]

tchaikov · 2024-02-20T16:43:39Z

@tchaikov thanks for working on this; are you OK to backport it to reef, please? :) [I mean if not I can probably open a MR myself, but...]

@mcv21 Hi Mathew, would be great if you could help on this. i am tied up with my daytime job recently.

mcv21 · 2024-02-20T17:01:27Z

@tchaikov thanks for working on this; are you OK to backport it to reef, please? :) [I mean if not I can probably open a MR myself, but...]

@mcv21 Hi Mathew, would be great if you could help on this. i am tied up with my daytime job recently.

I will endeavour to, but can you set the tracker task to state: pending backport please? the ceph-backport.sh script won't run at the moment because the tracker task isn't in that state (and I don't have enough redmine permissions to do it myself).

tchaikov · 2024-02-20T17:57:23Z

@mcv21 changed. thanks in advance!

mcv21 · 2024-02-20T18:25:47Z

@tchaikov sorry, the backport tasks are assigned to you, so ceph-backport.sh won't continue. Can you assign them to me (or maybe just unassigning them to yourself is sufficient)?

jhrcz-ls · 2024-02-21T20:30:08Z

i would like to warn before using hardcoded /home/cephadm as home for cephadm user... because in debian distro package ceph (a way to get 16.2 on debian bookworm) uses /var/lib/cephadm and when upgrading from debian package to ceph-io package this causes problems. (good thing is that other issue, the different behaviour of adduser on deb bookworm is fixed with this PR). i'm not sure that clean transition from debians package to ceph-io package is something to expect.

i filled issue in tracker (https://tracker.ceph.com/issues/64488) but later found issue related to this PR (https://tracker.ceph.com/issues/64069)

mcv21 · 2024-02-22T09:45:04Z

I'd like to bring the Debian packaging in ceph-upstream and ceph-Debian (if you see what I mean) closer together; but I think fixing this bug is not the place to change where ceph-upstream sets the cephadm user's $HOME to be.

mcv21 · 2024-02-22T10:10:55Z

@tchaikov sorry, the backport tasks are assigned to you, so ceph-backport.sh won't continue. Can you assign them to me (or maybe just unassigning them to yourself is sufficient)?

NVM, I found the --force option :)

tchaikov · 2024-02-22T12:25:04Z

i would like to warn before using hardcoded /home/cephadm as home for cephadm user... because in debian distro package ceph (a way to get 16.2 on debian bookworm) uses /var/lib/cephadm and when upgrading from debian package to ceph-io package this causes problems. (good thing is that other issue, the different behaviour of adduser on deb bookworm is fixed with this PR). i'm not sure that clean transition from debians package to ceph-io package is something to expect.

i filled issue in tracker (https://tracker.ceph.com/issues/64488) but later found issue related to this PR (https://tracker.ceph.com/issues/64069)

@jhrcz-ls hi Jan, thanks for filing this issue. i think we'd need a separate PR to address it.

github-actions bot added the build/ops label Jan 17, 2024

tchaikov self-assigned this Jan 18, 2024

tchaikov force-pushed the debian/postinst branch from c7c6c2e to 7a349a5 Compare January 18, 2024 11:50

tchaikov removed their assignment Jan 18, 2024

tchaikov requested a review from a team January 23, 2024 16:33

ljflores added the core label Jan 23, 2024

ljflores requested review from adk3798 and a team January 23, 2024 16:44

ljflores added orchestrator core and removed core labels Jan 23, 2024

adk3798 approved these changes Jan 23, 2024

View reviewed changes

ljflores approved these changes Jan 23, 2024

View reviewed changes

debian/cephadm.postinst Show resolved Hide resolved

ljflores added the needs-qa label Jan 23, 2024

Sitiritis reviewed Jan 29, 2024

View reviewed changes

tchaikov self-assigned this Feb 2, 2024

tchaikov force-pushed the debian/postinst branch from 7a349a5 to f96bba6 Compare February 3, 2024 05:53

tchaikov added 6 commits February 3, 2024 22:36

debian/cephadm.postinst: wrap command line lines which is too long

c517d0b

for better readability. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

debian/cephadm.postinst: replace tab with spaces

013eebf

for better readability, and to be more consistent with the rest of this file, and other .postinst scripts of this project. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

tchaikov force-pushed the debian/postinst branch from f96bba6 to e74ec0b Compare February 3, 2024 14:37

yuriw merged commit d74e98d into ceph:main Feb 20, 2024
11 of 12 checks passed

tchaikov deleted the debian/postinst branch February 20, 2024 16:43

mcv21 mentioned this pull request Feb 22, 2024

reef: debian/*.postinst: add adduser as a dependency and specify --home when adduser #55709

Merged

adk3798 mentioned this pull request Feb 28, 2024

squid: debian/*.postinst: add adduser as a dependency and specify --home when adduser #55814

Merged

14 tasks

debian/*.postinst: add adduser as a dependency and specify --home when adduser #55218

debian/*.postinst: add adduser as a dependency and specify --home when adduser #55218

Conversation

tchaikov commented Jan 17, 2024 • edited

Contribution Guidelines

Checklist

mcv21 commented Jan 17, 2024

tchaikov commented Jan 18, 2024 • edited

tchaikov commented Jan 18, 2024 • edited

tchaikov commented Jan 18, 2024

mcv21 commented Jan 18, 2024

tchaikov commented Jan 18, 2024

mcv21 commented Jan 18, 2024

tchaikov commented Jan 18, 2024

tchaikov commented Jan 23, 2024

tchaikov commented Jan 23, 2024

tchaikov commented Jan 23, 2024

ljflores commented Jan 23, 2024

ljflores commented Jan 23, 2024

adk3798 left a comment

Choose a reason for hiding this comment

ljflores left a comment

Choose a reason for hiding this comment

tchaikov commented Jan 24, 2024

mcv21 commented Jan 24, 2024

Sitiritis Jan 29, 2024

Choose a reason for hiding this comment

tchaikov Jan 30, 2024 • edited

Choose a reason for hiding this comment

tchaikov Feb 3, 2024

Choose a reason for hiding this comment

Sitiritis Feb 4, 2024

Choose a reason for hiding this comment

tchaikov commented Jan 30, 2024

NitzanMordhai commented Feb 2, 2024

tchaikov commented Feb 2, 2024

mcv21 commented Feb 2, 2024

mcv21 commented Feb 2, 2024

tchaikov commented Feb 3, 2024

tchaikov commented Feb 3, 2024

tchaikov commented Feb 4, 2024 • edited

mcv21 commented Feb 20, 2024

tchaikov commented Feb 20, 2024 • edited

mcv21 commented Feb 20, 2024

tchaikov commented Feb 20, 2024

mcv21 commented Feb 20, 2024

jhrcz-ls commented Feb 21, 2024

mcv21 commented Feb 22, 2024

mcv21 commented Feb 22, 2024

tchaikov commented Feb 22, 2024

tchaikov commented Jan 17, 2024 •

edited

tchaikov commented Jan 18, 2024 •

edited

tchaikov commented Jan 18, 2024 •

edited

tchaikov Jan 30, 2024 •

edited

tchaikov commented Feb 4, 2024 •

edited

tchaikov commented Feb 20, 2024 •

edited