[SSSD] Support for subdomains - stage 1
Stephen Gallagher
sgallagh at redhat.com
Tue Apr 3 12:32:13 UTC 2012
This is not (yet) a review. Just replying to a few questions in the
email, inline.
On Mon, 2012-04-02 at 10:07 +0200, Jan Zeleny wrote:
...
> > sysdb_update_subdomains() desperately needs comments. It took me ten
> > minutes to figure out the logic there. I also question why you didn'd
> > include sysdb_[add|remove]_subdomain(). An explanation of this lack
> > should be in the source code, if there's a good reason.
>
> Hm, I didn't write the routine and I could read it quite well. Nevertheless I
> added some comments. Those routines functions you are missing would be rather
> simple (almost aliases), that's why they are missing. Do you think there is a
> need for them?
>
When dealing with Active Directory directly, it may be possible that we
will only discover a new domain by requesting it the first time (as
opposed to acquiring a complete list ahead of time). If that's the case,
it would be handy to have a routine to simply add one to the list. I
agree that removal is probably unnecessary in most situations.
> > The creation of new_sd_list should be its own function.
>
> I disagree. The funcion would be called on this one place anyway so there is
> probably only a little gain in doing that.
>
The gain is mostly in readability. The creation of the new_sd_list is
moderately complex and slightly distracts from the main purpose of the
function. It's not worth nacking on, just my preference. Keep it if you
wish.
...
> > You need to check for whether becli->bectx->bet_info[BET_SUBDOMAINS] is
> > NULL, or else you'll crash on domains that don't support subdomain
> > lookups.
>
> I'm not sure what do you mean here, how could it be null when it's not a
> pointer?
>
As Jakub noted in his review, I meant to say
becli->bectx->bet_info[BET_SUBDOMAINS].bet_ops here.
> > One additional point about this patch. I don't like that you're forcing
> > the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer
> > that we add a subdomain_provider option, which defaults to being the
> > same as the ID provider. I prefer to maintain the distinction in case we
> > ever decide to have alternate mechanisms for determining the subdomains
> > (in IPA or AD).
>
> I discussed this with Sumit to be sure and we agreed that subdomains are
> supposed to be tightly bound to the ID provider. One of the reasons is that ID
> provider has to sort out all following requests so it will also needs to keep
> track of all subdomains.
Sure, I'm fine with that, then.
...
> > I'd prefer if you added an ipa_subdomain_map and used that for
> > determining the attribute names to use in
> > ipa_subdomains_parse_results(). Mostly for consistency.
>
> I don't think there would be much gain, the code almost wouldn't change, only
> names of attributes would be different.
>
Again, code-readibility is a good thing. I'd like to have a simple map
that we can glance at.
> > > #0011
> > > New ID-related config options for subdomains, these have to be present
> > > because IPA provider doesn't provide these values and defaults need to
> > > be implemented. Having defaults on the responder level didn't seem right
> > > since the policy might differ for each domain.
> >
> > Nack.
> >
> > I don't think this really makes sense at all. In most cases, users will
> > prefer to use the value on the LDAP server. If they choose to override
> > it, they'll do so through the existing override options (in the case of
> > override_homedir, it already has %d available anyway.
> >
> > We definitely don't need separate handling for shells. I can *kind of*
> > see a value if you wanted to have only subdomains have a non-default
> > location. I'm not sure I like that though. I feel like it's probably
> > more complexity than we need.
>
> I think you possibly missed the point. The point is that this information is
> NOT on the server, therefore we need a value that will fill it in. Otherwise
> only a blank field will be stored in sysdb and returned to the client
>
Ok, I think I understand now, but the manpages need to be MUCH more
clear. It sounds like you're adding this option to always override
subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles
this internally by translating a NULL value for the shell into "the
system default shell" (usually /bin/sh). This is handled by glibc and
isn't our concern.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120403/a9125c04/attachment.sig>
More information about the sssd-devel
mailing list