sgnl05 / sgnl05-sssd

Puppet module for SSSD
https://forge.puppet.com/sgnl05/sssd
GNU General Public License v3.0
17 stars 77 forks source link

Ft/suse #25

Closed tobiasgiese closed 7 years ago

tobiasgiese commented 7 years ago

Added support for SuSE/SLES

sgnl05 commented 7 years ago

Looks good what you've done so far. Could you please add the Suse versions you've testet to the metadata.json file, and make spec tests for those versions?

tobiasgiese commented 7 years ago

The problem is that only Suse/SLES will not add the sss entries to the nsswitch.conf. Both debian and redhat will set the nsswitch sss entry by the sssd package. This is why i just want to add sss to passwd, group and sudoers in the nsswich.conf for osfamily == suse.

On 3 November 2016 at 21:18, Chris Edester notifications@github.com wrote:

@edestecd commented on this pull request.

In manifests/config.pp https://github.com/sgnl05/sgnl05-sssd/pull/25:

  • if $mkhomedir { +
  • exec { 'pam-config -a --mkhomedir':
  • path => '/bin:/usr/bin:/sbin:/usr/sbin',
  • unless => $pamconfig_mkhomedir_check_cmd,
  • } +
  • } +
  • exec { 'pam-config -a --sss':
  • path => '/bin:/usr/bin:/sbin:/usr/sbin',
  • unless => $pamconfig_check_cmd,
  • } +
  • if $manage_nsswitch { +

If you still really want it here, then we need some conditions to make it work on SLES differently than Debian and RedHat.

if $manage_nsswitch { include '::nsswitch::params'

class { 'nsswitch': passwd => $sssd::params::nsswitch_passwd, shadow => $sssd::params::nsswitch_shadow, group => $sssd::params::nsswitch_group, sudoers => $sssd::params::nsswitch_sudoers, } }

Then set these parameters in params to the right thing for each OS in params.pp. For SLES, you will set sudoers and shadow to undef and whatever the others should be in a default install of SLES, plus sss...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sgnl05/sgnl05-sssd/pull/25, or mute the thread https://github.com/notifications/unsubscribe-auth/AWAfC5aidBJ_5Q-Xb7u6-Q1KIFOOalHWks5q6kGegaJpZM4KhGDa .

edestecd commented 7 years ago

You mean the package install modifies the nsswich.conf in Redhat and Debian, but not in SUSE? This is fine, but its also OK to let the nsswitch module monitor that file and keep it this way... even if the package sets it initially.

What I am very opposed to is using execs to manage nsswitch.conf, when there is a supported module that supports the OSes we need.

It's also nice if you can do the same thing on all supported OSes. I suppose you could manage the nsswitch on only SUSE, but since its optional, why not offer it to all, if possible?

At any rate, I'd settle for just not using execs at the minimum.

tobiasgiese commented 7 years ago

I think you are right. Managing the nsswitch.conf outside of the sssd module is maybe the better way. But my problem with your mentioned nsswitch module is that there is no function to just add or rather keep a single value such as sss in the config file.

On 3 November 2016 at 21:47, Chris Edester notifications@github.com wrote:

You mean the package install modifies the nsswich.conf in Redhat and Debian, but not in SUSE? This is fine, but its also OK to let the nsswitch module monitor that file and keep it this way... even if the package sets it initially.

What I am very opposed to is using execs to manage nsswitch.conf, when there is a supported module that supports the OSes we need.

It's also nice if you can do the same thing on all supported OSes. I suppose you could manage the nsswitch on only SUSE, but since its optional, why not offer it to all, if possible?

At any rate, I'd settle for just not using execs at the minimum.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sgnl05/sgnl05-sssd/pull/25#issuecomment-258269111, or mute the thread https://github.com/notifications/unsubscribe-auth/AWAfC5DIYJZMzFQSUeYDgh5hZEEJzYyoks5q6khdgaJpZM4KhGDa .

edestecd commented 7 years ago

Yea the nsswitch module doesn't use anything fancy like augeaus, so you have to set exactly all the options you want for each option. However, the defaults from the module are usually in line with a fresh install of any supported OS, so you can usually just concat the default with sss, to achieve the same result.

Did you experience something different with SUSE? Was it changing what you got from a stock install?

sgnl05 commented 7 years ago

Looks very good. Could you please also do spec tests? If you're having problems with it, call out and we'll help. :)

ghoneycutt commented 7 years ago

This pattern is a bit irregular. Normally you wouldn't put packages to exclude in a module and if so, you would include a class that did that for you. You could include ghoneycutt/nscd and set the package to be absent. The other way I've seen this handled is to do it in a profile class

class profile::sssd {
  include ::sssd

  package { 'nscd':
    ensure => 'absent',
    before => Class['sssd']
  }
}

https://github.com/ghoneycutt/puppet-module-nscd

benkevan commented 7 years ago

any idea when this may get merged?

sgnl05 commented 7 years ago

@benkevan We're missing spec tests for this PR, and @ghoneycutt has made a few valid comments about the code.

sgnl05 commented 7 years ago

Closing this for now. Conflicts, spec tests and comments by @ghoneycutt needs to be resolved.