Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use systemd-sysusers in RPMs #6807

Closed
wants to merge 1 commit into from

Conversation

jonathanspw
Copy link

No description provided.

@jonathanspw
Copy link
Author

I'm not sure how to properly add the new source file (sssd.sysusers) into all of the CI/automated builds.

@ikerexxe
Copy link
Contributor

Can you provide some type of background (i.e. description, links) to what you are trying to achieve?

@justin-stephenson
Copy link
Contributor

Can you provide some type of background (i.e. description, links) to what you are trying to achieve?

This is about adopting to the new preferred implementation for adding users/groups to replace 'useradd/groupadd' commands.

https://fedoraproject.org/wiki/Changes/Adopting_sysusers.d_format
https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_allocation_strategies

@justin-stephenson
Copy link
Contributor

I'm not sure how to properly add the new source file (sssd.sysusers) into all of the CI/automated builds.

IIUC we need to add an additional input variable to https://github.com/SSSD/action-build-srpm which accepts a file, that can be used for any additional SourceX files, in this case sssd.sysusers. So that it can be called with:

  • name: Build source rpm
    id: srpm
    uses: next-actions/build-srpm@master
    with:
    tarball: ${{ inputs.working-directory }}/sssd-${{ steps.sanitize.outputs.version }}.tar.gz
    specfile: ${{ inputs.working-directory }}/sssd.spec
    sourcefile: ${{ inputs.working-directory }}/sssd.sysusers

Do you agree @pbrezina ?

contrib/sssd.sysusers Outdated Show resolved Hide resolved
@jonathanspw jonathanspw force-pushed the systemd-sysusers branch 2 times, most recently from 2b91dcd to 578ce13 Compare July 18, 2023 19:24
Signed-off-by: Jonathan <[email protected]>
@pbrezina
Copy link
Member

pbrezina commented Aug 1, 2023

I'm not sure how to properly add the new source file (sssd.sysusers) into all of the CI/automated builds.

IIUC we need to add an additional input variable to https://github.com/SSSD/action-build-srpm which accepts a file, that can be used for any additional SourceX files, in this case sssd.sysusers. So that it can be called with:

* name: Build source rpm
  id: srpm
  uses: next-actions/build-srpm@master
  with:
  tarball: ${{ inputs.working-directory }}/sssd-${{ steps.sanitize.outputs.version }}.tar.gz
  specfile: ${{ inputs.working-directory }}/sssd.spec
  sourcefile: ${{ inputs.working-directory }}/sssd.sysusers

Do you agree @pbrezina ?

Yes, but it must be able to take more files not just one.

@justin-stephenson
Copy link
Contributor

Do you agree @pbrezina ?

Yes, but it must be able to take more files not just one.

PR Created next-actions/build-srpm#1

@justin-stephenson
Copy link
Contributor

I added the changes justin-stephenson@8308be0 to have this pass in CI. @ikerexxe does it look good from your side? If so then @jonathanspw can cherry-pick this commit into this PR.

I am not sure if we want to make this change for RHEL8 and RHEL9, if yes then we need to change this condition in the spec file. I will defer to @alexey-tikhonov on this.

%if 0%{?rhel} >= 9 || 0%{?fedora}

@@ -188,7 +189,8 @@ Requires: (sssd-nfs-idmap = %{version}-%{release} if libnfsidmap)
Requires: libsss_idmap = %{version}-%{release}
Requires: libsss_certmap = %{version}-%{release}
%if 0%{?rhel}
Requires(pre): shadow-utils
BuildRequires: systemd-rpm-macros
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it BuildRequires (and not Requires)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guide says "add a BuildRequires" but doesn't actually explain.

@@ -701,6 +703,8 @@ do
cat $subpackage.lang
done

install -D -p -m 0644 %{SOURCE1} %{buildroot}%{_sysusersdir}/sssd.conf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this name - 'sssd.conf' - mandated anywhere?
It's confusing because it matches name of SSSD config file.
Would it be possible to use different name for a file with sysuser definition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked _sysusersdir location (/usr/lib/sysusers.d/) and I only saw one exception, which I guess makes it valid to use another extension

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per man sysusers.d

CONFIGURATION DIRECTORIES AND PRECEDENCE
       Each configuration file shall be named in the style of package.conf or package-part.conf. The second variant should be used when it is desirable to make it easy to override just this part of configuration.

@@ -54,6 +54,7 @@ Summary: System Security Services Daemon
License: GPLv3+
URL: https://github.com/SSSD/sssd/
Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz
Source1: sssd.sysusers
Copy link
Member

@alexey-tikhonov alexey-tikhonov Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file has to be maintained in downstream dist-git like spec-file?

Copy link
Contributor

@justin-stephenson justin-stephenson Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -804,6 +808,10 @@ done
%{_datadir}/systemtap/tapset/sssd.stp
%{_datadir}/systemtap/tapset/sssd_functions.stp
%{_mandir}/man5/sssd-systemtap.5*
%if 0%{?rhel} >= 9 || 0%{?fedora}
Copy link
Member

@alexey-tikhonov alexey-tikhonov Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either I misunderstand something, or this condition is inconsistent:

  • install above is unconditional.
  • config is packaged for rhel >= 9 and fedora
  • below users are created for rhel only (any version)

Frankly I wouldn't touch RHEL8 and RHEL9 and only bring this to F40+/RHEL10

@@ -0,0 +1 @@
u sssd - "User for sssd" / /sbin/nologin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the beginning of the spec-file we have:

# define SSSD user
%if 0%{?rhel}
%global sssd_user sssd
...

Would it be possible to avoid copy-paste of 'sssd' user name?

@ikerexxe
Copy link
Contributor

I added the changes justin-stephenson@8308be0 to have this pass in CI. @ikerexxe does it look good from your side? If so then @jonathanspw can cherry-pick this commit into this PR.

It's failing for centos stream 8, and if we follow Alexey's proposal it will also fail in centos stream 9. For those distributions we should remove /usr/lib/sysusers.d/sssd.conf file.

@pbrezina
Copy link
Member

pbrezina commented Sep 7, 2023

Any update on this one?

@justin-stephenson
Copy link
Contributor

if @jonathanspw is okay with it, I can open a new PR and continue to move this forward (I already have a systemd sysusers ticket assigned to me)

@jonathanspw
Copy link
Author

if @jonathanspw is okay with it, I can open a new PR and continue to move this forward (I already have a systemd sysusers ticket assigned to me)

Sure, whatever keeps things moving forward.

@justin-stephenson
Copy link
Contributor

#6925 created to continue this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes requested no-backport This should go to target branch only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants