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

[Martin Kosek]

On 09/20/2013 01:59 PM, Jan Cholasta wrote:
> 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.

Yup, get_dn() is the method where object DN lookup should be done. See for
example host.py plugin get_dn method, we also do a dynamic lookup for correct
host name.

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

+1 for being consistent with the rest of the system, even if the commands are
internal (for now), we should keep the naming conventions that are present in
the rest of the system.

Since we are operating on trust domain objects, the API should be:
trustdomain-add
trustdomain-del
trustdomain-find
trustdomain-mod

+ the visible commands on Trust. I did not dig into code, but as Honza said,
the best way to implement dynamic DN gathering is the get_dn() method. That
way, it could be implemented in one place and all commands could take advantage
of it instead of re-implementing it several times in pre_callback - this is
just hackish.

Alexander, can you please work with Honza in case the get_dn function does not
work for you? He may prepare a patch for you if he has an idea how to do it better.

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

As above, even if the commands are internal, let's make them consistent with
rest of the framework. It would be very hard to change API later if we for
example find we want to expose the API.

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

I think it would have been very useful to have a design page before sending a
patch. It is then easier to make design decisions without having to dig into
the patch.

Thank you,
Martin