[sssd PR#5693][opened] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: opened

PR body:
"""
This is for preliminary review.

Limitations:
  - only IPA provider
  - single subid interval pair (subuid+subgid) per user
  - idviews aren't supported
  - only forward lookup (user -> subid)
    
Known TODOs:
  - delete cached subid ranges in case "not found" on a server
  - distinguish "user not found" vs "user doesn't have ranges
defined"

To test:
  - build shadow-utils from latest upstream (or get Fedora Rawhide build from
https://copr.fedorainfracloud.org/coprs/ipedrosa/subid_ranges/)
  - build FreeIPA with https://github.com/freeipa/freeipa/pull/5713 (or ping me for
details of internal test setup)
  - build SSSD with this PR (enabling `--with-subid`)
  - edit /etc/nsswitch.conf: `subid: sss`
  - use for example this
[utility](https://github.com/shadow-maint/shadow/blob/master/src/list_subi... from
shadow-utils upstream to fetch subid ranges from IPA server (or `new?idmap`)

"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5693/head:pr5693
git checkout pr5693

[sssd PR#5693][+Bugzilla] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Bugzilla

[sssd PR#5693][edited] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: edited

 Changed field: body
Original value:
"""
This is for preliminary review.

Limitations:
  - only IPA provider
  - single subid interval pair (subuid+subgid) per user
  - idviews aren't supported
  - only forward lookup (user -> subid)
    
Known TODOs:
  - delete cached subid ranges in case "not found" on a server
  - distinguish "user not found" vs "user doesn't have ranges
defined"

To test:
  - build shadow-utils from latest upstream (or get Fedora Rawhide build from
https://copr.fedorainfracloud.org/coprs/ipedrosa/subid_ranges/)
  - build FreeIPA with https://github.com/freeipa/freeipa/pull/5713 (or ping me for
details of internal test setup)
  - build SSSD with this PR (enabling `--with-subid`)
  - edit /etc/nsswitch.conf: `subid: sss`
  - use for example this
[utility](https://github.com/shadow-maint/shadow/blob/master/src/list_subi... from
shadow-utils upstream to fetch subid ranges from IPA server (or `new?idmap`)

"""

[sssd PR#5693][+Changes requested] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Changes requested

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

alexey-tikhonov commented:
"""
Hi @pbrezina,

thank you for the review.

>  * Please, provide some description to the first commit message.
>  
Do you think it makes sense to keep 2 commits? I was going to squash them after review.


>  * Why conditional build? Why is build disabled by default?
>  
This is MVP of experimental feature.
Corresponding code in shadow-utils is merged upstream, but there is no upstream release
available yet. We brought functionality to Fedora via patches. But I doubt other
distributions will do the same. I.e. out of Fedora/RHEL, this is at the moment for
enthusiasts who are willing to play with a new feature and who is capable to rebuild
shadow-utils from sources.
Moreover, I anticipate significant changes might be required once we have initial
feedback.

So... at the very least it's conditional to allow distributions to avoid installing
plugin that they can't use yet.

Once shadow-utils upstream release is done and widely used, Podman patches released
(https://github.com/containers/storage/pull/882), etc (so it is more or less easy to use),
we can change default and build code / install plugin in sssd-ipa, IMO.

>      * We don't use two blank lines between function definitions.
>  
And this (monolithic text) makes me sad :) I didn't see such restrictions in
https://sssd.io/contrib/coding-style.html


>      * Can the range be set also for trusted domain user? If yes, this
> implementation doesn't seem to support it.
>  
No, FreeIPA doesn't support it in MVP.


"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-885572708

[sssd PR#5693][synchronized] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5693/head:pr5693
git checkout pr5693

[sssd PR#5693][-Changes requested] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: -Changes requested

[sssd PR#5693][+Waiting for review] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Waiting for review

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

alexey-tikhonov commented:
"""
`make-check-valgrind` times out on Rawhide (and a couple of tests fail) but I can't
reproduce this locally.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-886909800

[sssd PR#5693][synchronized] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5693/head:pr5693
git checkout pr5693

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

pbrezina commented:
"""
>  Hi @pbrezina,
>  
>  thank you for the review.
>  
>  > * Please, provide some description to the first commit message.
>  
>  Do you think it makes sense to keep 2 commits? I was going to squash them after review.
>  
It's up to you, just make sure it has sufficient description.

>  > * Why conditional build? Why is build disabled by default?
>  
>  This is MVP of experimental feature.
>  Corresponding code in shadow-utils is merged upstream, but there is no upstream release
> available yet. We brought functionality to Fedora via patches. But I doubt other
> distributions will do the same. I.e. out of Fedora/RHEL, this is at the moment for
> enthusiasts who are willing to play with a new feature and who is capable to rebuild
> shadow-utils from sources.
>  Moreover, I anticipate significant changes might be required once we have initial
> feedback.
>  
>  So... at the very least it's conditional to allow distributions to avoid installing
> plugin that they can't use yet.
>  
Ack. This is something to document in the commit message.

>  Once shadow-utils upstream release is done and widely used, Podman
> patches released
> ([containers/storage#882](https://github.com/containers/storage/pull/882)), etc (so it is
> more or less easy to use), we can change default and build code / install plugin in
> sssd-ipa, IMO.
>  
Ok.

>  > ```
>  > * We don't use two blank lines between function definitions.
>  > ```
>  
>  And this (monolithic text) makes me sad :) I didn't see such restrictions in
> https://sssd.io/contrib/coding-style.html
>  
:-) Even though something is not explicitly banned, it is not cool to differentiate from
the rest of the project.

>  > ```
>  > * Can the range be set also for trusted domain user? If yes, this implementation
> doesn't seem to support it.
>  > ```
>  
>  No, FreeIPA doesn't support it in MVP.
>  
Code looks good to me. I'm going to test it.

"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-887410280

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

pbrezina commented:
"""
Seems to work fine. Please, do the final touch and I wlll ack it.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-888263832

[sssd PR#5693][-Waiting for review] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: -Waiting for review

[sssd PR#5693][synchronized] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5693/head:pr5693
git checkout pr5693

[sssd PR#5693][+Waiting for review] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Waiting for review

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

alexey-tikhonov commented:
"""
Squashed patches, removed excessive new lines, added release notes to the commit message.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-888636774

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

alexey-tikhonov commented:
"""
Squashed patches, removed excessive new lines, added release notes to the commit
message.

Should I reference https://github.com/shadow-maint/shadow/issues/154 explicitly in the
commit message?
"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-888636774

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

pbrezina commented:
"""
>  Squashed patches, removed excessive new lines, added release notes to
> the commit message.
>  
>  Should I reference
> [shadow-maint/shadow#154](https://github.com/shadow-maint/shadow/issues/154) explicitly in
> the commit message?
>  
It might be good for other distro maintainers.

Please separate the feature release note with blank line from the rest of description.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-888983570

[sssd PR#5693][synchronized] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5693/head:pr5693
git checkout pr5693

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

alexey-tikhonov commented:
"""
>  > Squashed patches, removed excessive new lines, added release
> notes to the commit message.
>  > Should I reference
> [shadow-maint/shadow#154](https://github.com/shadow-maint/shadow/issues/154) explicitly in
> the commit message?
>  
>  It might be good for other distro maintainers.
>  
Done.

>  Please separate the feature release note with blank line from the
> rest of description.
>  
Well, I meant all this text to be included in the notes...

"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-889007047

[sssd PR#5693][-Waiting for review] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: -Waiting for review

[sssd PR#5693][+Accepted] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Accepted

[sssd PR#5693][+Ready to push] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Ready to push

[sssd PR#5693][comment] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

pbrezina commented:
"""
Pushed PR: https://github.com/SSSD/sssd/pull/5693

* `master`
    * f546088226872f24722bdd94388816792bd5891a - Basics of 'subid ranges' support
for IPA provider.

"""

See the full comment at https://github.com/SSSD/sssd/pull/5693#issuecomment-889069780

[sssd PR#5693][+Pushed] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: +Pushed

[sssd PR#5693][-Accepted] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: -Accepted

[sssd PR#5693][-Ready to push] Basics of 'subid ranges' support for IPA provider

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider

Label: -Ready to push

[sssd PR#5693][closed] Basics of 'subid ranges' support for IPA provider

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5693
Author: alexey-tikhonov
 Title: #5693: Basics of 'subid ranges' support for IPA provider
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5693/head:pr5693
git checkout pr5693