[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