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

[Jan Cholasta]

On 20.9.2013 12:55, Alexander Bokovoy wrote:
>>>> 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.

This doesn't sound like a big obstacle to me. Right now the trust_type 
lookup is done in trust_show.execute() for some reason, which is not the 
best place to do it IMHO. Doing it in trust.get_dn() instead should 
simplify things enough to make parent_object work.

>
>>> 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.

Sorry, I should have been more specific - trust-domain-fetch is OK and 
the rest of the commands are not visible in the CLI, so naming them 
trustdomain-* will not confuse anyone.

>
> 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.

I agree with this. As I said above, I am not suggesting making the trust 
domain objects visible to users.

>
>
>>>> 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.

Honza

-- 
Jan Cholasta