URL: https://github.com/SSSD/sssd/pull/186 Author: mzidek-rh Title: #186: Subdomain config Action: opened
PR body: """ This is the part of subdomain configuration feature needed to allow specify the search bases for the subdomain (one of the two use cases that were specified in the desin doc).
The second part with the server was more complicated then I expected, so I will send it separately (hopefully today or tomorrow), but I can not delay this first part anymore).
In order to test create a ipa-ad trust and redefine for example the ldap_search_base or ldap_user_search_base on the server side SSSD. BOth client and server side SSSDs will be able to use the alternative search base (no config changes needed on the client side SSSD). """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/186/head:pr186 git checkout pr186
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
jhrozek commented: """ Thank you, setting the search bases work in my testing. As far as this first patchset, I only have the questions I asked in the review. But more importantly, what are all the options that will be settable for a subdomain?
It looks like I was able to at least override the user maps, at least I tested `ldap_user_name=xxx`. I think it's fine, but the manpage doesn't mention that, so I'm not sure if that was actually intended.
Should the next version allow setting ad_server ad_site per subdomain?
When creating a new subdomain with `new_subdomain`, shouldn't the code also look into "its own" subdomain section so that also timeouts are settable? (I'm not sure there is a good use-case for this, but in new_subdomain we at least set the ignore_group_members which would be nice to set per subdomain). """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-285615666
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
mzidek-rh commented: """ Hi, I respond to your comments in the review in this comment, so that you get notification :)
About the condition with cdb and conf_path being NULL. You are right that this can not happen in the SSSD code and is (half) dead code and I do plan to delete it, but not immediately. Some of our cmocka tests rely on this function (they use it to set up the defaults and then run 1way/2way trust init unit tests). I had trouble when rewriting the tests, so I decided to keep it there for now.
About server and site. Yes I plan to add that too, but it will be separate patch, not new version of the patches I sent. It is still not testable so I wanted to get at least this part out.
About what options will be supported. Yes it is possible to set more options with this patch. But I was not sure if we want to support all possible cases, so I decided to only document those. My concern is that we would need to support those forever, so I would prefer to have a solid use case behind every such option.
I will update the patches. """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-285637849
URL: https://github.com/SSSD/sssd/pull/186 Author: mzidek-rh Title: #186: Subdomain config Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/186/head:pr186 git checkout pr186
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
jhrozek commented: """
On 10 Mar 2017, at 11:47, mzidek-rh notifications@github.com wrote:
Hi, I respond to your comments in the review in this comment, so that you get notification :)
About the condition with cdb and conf_path being NULL. You are right that this can not happen in the SSSD code and is (half) dead code and I do plan to delete it, but not immediately. Some of our cmocka tests rely on this function (they use it to set up the defaults and then run 1way/2way trust init unit tests). I had trouble when rewriting the tests, so I decided to keep it there for now.
About server and site. Yes I plan to add that too, but it will be separate patch, not new version of the patches I sent. It is still not testable so I wanted to get at least this part out.
About what options will be supported. Yes it is possible to set more options with this patch. But I was not sure if we want to support all possible cases, so I decided to only document those. My concern is that we would need to support those forever, so I would prefer to have a solid use case behind every such option.
Yeah, all options don’t make sense in all cases. And I think it’s better to only document, test and support a subset, as long as the code is built generically so it’s easy to extend. """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-285642341
URL: https://github.com/SSSD/sssd/pull/186 Author: mzidek-rh Title: #186: Subdomain config Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/186/head:pr186 git checkout pr186
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
mzidek-rh commented: """ Ok, I added the ad_server, ad_site and ad_backup_server to this patchset (it is only 14 lines and the testing is almost the same, so i guess it makes sense).
I also updated the MAN page patch to include these options. """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-286395594
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
jhrozek commented: """ The patches now look good to me, but I have some questions: 1) Could we (perhaps in a future patch) extend the dp_options structure with a boolean flag to indicate which options can be inherited? Right now we inherit the whole map which is dangerous 2) Can you also add (again, in another PR) a patch that would allow to set also all the options that are currently settable with subdomain_inherit directly? """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-286758617
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
mzidek-rh commented: """ I agree with the comments. I also want to add a sssd specific configuration validator that will check if options used in subdomain are allowed and print message if they are not. But I need to keep this PR as small as possible, because it is tied to some deadly deadlines :)
I will create pagure issues to address the above once this is in master. """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-286762014
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
jhrozek commented: """ CI: http://sssd-ci.duckdns.org/logs/job/64/93/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-286777717
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
jhrozek commented: """ * master: 61854f17c8de6548fb56bb7ad959708109d76fc3 62a1570f01053ec61e894ee3e58fc759ee809c6e 231bd1b34023daa3080cf461085e6e4aa7f4d733 ebe05e32b5af9b1ee404ebe492e52096d45fb675
"""
See the full comment at https://github.com/SSSD/sssd/pull/186#issuecomment-286786637
URL: https://github.com/SSSD/sssd/pull/186 Author: mzidek-rh Title: #186: Subdomain config Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/186/head:pr186 git checkout pr186
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/186 Title: #186: Subdomain config
Label: -Accepted
sssd-devel@lists.fedorahosted.org
-
jhrozek -
mzidek-rh