[Freeipa-devel] [PATCH] 0118 add support for subdomains

[Alexander Bokovoy]

On Fri, 20 Sep 2013, Jan Cholasta wrote:
>>>>part of https://fedorahosted.org/freeipa/ticket/3909
>>>>
>>>>The patch implements some dark magic to get around IPA framework
>>>>limitations:
>>>>
>>>> -- CLI commands belong to 'trust' family but operate on 'subdomain'
>>>>object
>>>> -- 'subdomain' objects are stored under trust container, thus making
>>>>    container_dn dependent on a particular trust:
>>>>     cn=<subdomain>,cn=<trust>,cn=ad,cn=trusts,$SUFFIX
>>>>
>>>>The latter is a design decision since our KDC driver loads all objects
>>>>with objectclass=ipaNTTrustedDomain from cn=ad,cn=trusts,$SUFFIX using
>>>>subtree scope. With this design no changes were needed in ipa-kdb at all
>>>>to support subdomains.
>>>>
>>>
>>>NACK, this patch breaks several conventions we use in the framework:
>>Have you read the reasoning first?
>
>Yes.
>
>>
>>>1) The object is named "subdomain", but the commands are named
>>>"trust_domain_*". Please use the object name as the base for command
>>>names. I would suggest renaming the object to "trustdomain", as the
>>>framework does not allow underscores in object names, and "subdomain"
>>>sounds a little bit too generic.
>>subdomains as objects are not visible to users. Users cannot operate on
>>subdomains themselves. I do not want to give you impression we are
>>having separate trust and domains. Instead, we deal with trusts and
>>subdomains are simply internal components of it.
>
>They are part of the API, whether they are visible to users or not, 
>and they should follow the same convention as the rest of the API.
>
>From the framework perspective, subdomains are child objects of 
>trusts, no different to how DNS records are child objects of DNS 
>zones, except they are internal and not visible to users. You even 
>implemented them this way in the patch, I just don't like how hackily 
>it is done.
The difference is that with DNS records and zones you have explicit
mapping -- record is under zone and zone is under dns container. The DN
is explicitly defined by input under static DNS container DN.

With trusts there are few possible containers and input does not define
the DN fully. We have trust type which means whenever trust is created,
type is either default or explicitly set, yet for any other operation on
the trust its type is not specified and has to be looked up.

With subdomains under trusts we get another level of indirection. Had
subdomains and trusts be based on a static DN, it could have worked.
However, we don't have trust type available so it needs to discovered
every time. This doesn't play well with the framework, it is simply not
expecting dynamic containers.

>>The fact that you currently see these commands in 'ipa trust-*' family
>>reflects the situation. -mod command is intended to be internal as well,
>>it will not be visible in CLI in the end. The only command available to
>>users will be 'ipa trust-domain-fetch'.
>
>All the more reason for following the usual convention.
I don't follow you here. With a single visible command added to the
trust family why it should be 'ipa trustdomain-fetch' instead of 'ipa
trust-domain-fetch'? What is the reason for inconsistency.

Current way framework lays down object-verb system is due to
implementation convenience, not because this is a really required from
CLI point of view. I believe I have reasonable arguments to not fall
into trap of having multiple objects visible to CLI. CLI should be
simple to use, it should not expose too much of internal complexity
where not required.


>>>2) There is already support for objects inside objects in the
>>>framework, there's no need to reinvent this. See the parent_object
>>>attribute of LDAPObject and the dns plugin for practical example.
>>It does not work in this case properly. Feel free to try to implement
>>it, really. I have spent good deal of last two weeks trying all options
>>possible.
>
>Why does it not work? The functionality of parent_object is pretty 
>straightforward, so I can't imagine what might go wrong.
See above. It is not designed for the use case of dynamic container DNs
which involves components not derived from the input.


-- 
/ Alexander Bokovoy