[Freeipa-devel] [PATCH] [WIP] 172+173+175 Create per-type DNS API

Fri Dec 2 14:33:43 UTC 2011

Martin Kosek wrote:
> On Thu, 2011-12-01 at 17:18 -0500, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On Mon, 2011-11-28 at 17:35 +0100, Martin Kosek wrote:
>>>> I have prepared a working prototype of the new structured DNS API. It
>>>> may still have rough edges (and unit tests are not ready), but it will
>>>> provide a base for discussion and for WebUI folks - so that they can
>>>> start development of the new DNS WebUI API.
>>>>
>>>> The patch takes advantage of the DNS refactor I did in 172. For all
>>>> supported non-DNSSEC RR types, the following commands are available:
>>>>
>>>> dnsrecord<RRTYPE>-show ZONE NAME
>>>> dnsrecord<RRTYPE>-add ZONE NAME
>>>> dnsrecord<RRTYPE>-mod ZONE NAME VALUE
>>>>
>>>> This is an example of the new API in action:
>>>>
>>>> # ipa dnsrecord-show example.com foo
>>>>     Record name: foo
>>>>     A record: 10.0.0.1
>>>>
>>>> # ipa dnsrecordmx-add example.com foo --exchanger="foo.example.com."
>>>>     MX record: 0 foo.example.com.
>>>>     Preference: 0
>>>>     Exchanger: foo.example.com.
>>>> ----------------------------
>>>> Number of entries returned 1
>>>> ----------------------------
>>>>
>>>> # ipa dnsrecordmx-add example.com foo --preference=1 --exchanger="foo.example.com."
>>>>     MX record: 0 foo.example.com.
>>>>     Preference: 0
>>>>     Exchanger: foo.example.com.
>>>>
>>>>     MX record: 1 foo.example.com.
>>>>     Preference: 1
>>>>     Exchanger: foo.example.com.
>>>> ----------------------------
>>>> Number of entries returned 2
>>>> ----------------------------
>>>>
>>>> # ipa dnsrecordmx-show example.com foo
>>>>     MX record: 0 foo.example.com.
>>>>     Preference: 0
>>>>     Exchanger: foo.example.com.
>>>>
>>>>     MX record: 1 foo.example.com.
>>>>     Preference: 1
>>>>     Exchanger: foo.example.com.
>>>> ----------------------------
>>>> Number of entries returned 2
>>>> ----------------------------
>>>>
>>>>
>>>> There is an interactive wizard to help user modify a record without
>>>> specifying an updated value first. If there is just one (MX) record, no
>>>> wizard would be run.
>>>>
>>>> # ipa dnsrecordmx-mod example.com foo --preference=2
>>>> Which MX record would you like to modify?
>>>>
>>>> [1]: 0 foo.example.com.
>>>> [2]: 1 foo.example.com.
>>>>
>>>> DNS record number: 2
>>>>     MX record: 0 foo.example.com.
>>>>     Preference: 0
>>>>     Exchanger: foo.example.com.
>>>>
>>>>     MX record: 2 foo.example.com.
>>>>     Preference: 2
>>>>     Exchanger: foo.example.com.
>>>> ----------------------------
>>>> Number of entries returned 2
>>>> ----------------------------
>>>>
>>>> # ipa dnsrecordmx-mod example.com foo "2 foo.example.com." --preference=3
>>>>     MX record: 0 foo.example.com.
>>>>     Preference: 0
>>>>     Exchanger: foo.example.com.
>>>>
>>>>     MX record: 3 foo.example.com.
>>>>     Preference: 3
>>>>     Exchanger: foo.example.com.
>>>> ----------------------------
>>>> Number of entries returned 2
>>>> ----------------------------
>>>>
>>>>
>>>> There are few open questions I am still thinking about:
>>>>
>>>> 1) The commands return a list of structured records (just like *-find
>>>> commands) instead of returning just one record. I thought that it may be
>>>> more usable this way and consistent with dnsrecord-add/mod/show commands
>>>> behavior which returns all records too. Otherwise, we would have to
>>>> change the show command API and add VALUE argument, which would specify
>>>> a value to be displayed:
>>>> dnsrecord<RRTYPE>-show ZONE NAME VALUE
>>>>
>>>> 2) Raw DNS record value is in the output too. I though it would be
>>>> useful to see the raw DNS record value + its parts at one place.
>>>>
>>>> 3) The commands are in format dnsrecord<RRTYPE>-cmd, for example
>>>> dnsrecordmx-add. I think dnsrecord-mx-add may be more readable. If we
>>>> want to go this way, I would have to bend the server framework a little
>>>> which parses an LDAP object from the command name (LDAP object name is
>>>> dnsrecordmx in this case). This is doable, although I am not sure if
>>>> this does not have some implications in WebUI side.
>>>>
>>>> Martin
>>>
>>> I rebased both patches to the most recent master. Adding CSVs now works
>>> ok again (with the fix in 175):
>>>
>>> # ipa dnsrecord-mod example.com foo --a-rec=10.0.0.1,10.0.0.2
>>>     Record name: foo
>>>     A record: 10.0.0.1, 10.0.0.2
>>>
>>> Martin
>>
>
> Rob, thank you for the review! What do you think about the 3 open
> questions I posted above?

I haven't applied the patches to see what the output looks like yet so 
can't really comment on the first two.

I think the extra dash would make the command line easier to grok, or at 
least read, but it isn't a show stopper for me. I'd be interested in 
feedback from the UI guys but they may have to start poking at it to 
really know for sure how much of an issue it would be.

>
>> I think some abbreviations can be eliminated:
>>
>> siz ->  size
>> alt ->  altitude
>
> Sure, this can be fixed.
>
>>
>> For MX record (and probably KX) you can make preference required. It
>> should fill in without prompting since it has a default. This way it
>> will show as required in the UI.
>
> Right, we don't want to run into issues we had with user fields.
>
>>
>> PTRRecord doc I think would read better as "The hostname this reverse
>> record points to"
>
> Ok. Btw do you think it would be good to document this way all these new
> DNSRecord part parameters? As I checked with Petr, these would be shown
> in the UI - so the UI user would benefit from it. But it will require a
> lot of writing and RFC study :-)

I was wondering that myself. The labels can be rather terse, I wasn't 
sure how much more a _doc() would add. I was also wondering if we should 
include some of the limits within the doc, esp the 0-64k ones since 
those are smaller. It would make it somewhat inconsistent which is why I 
didn't raise it.

>>
>> I'm not sure I follow the makeapi change. I see the new entry types in
>> API.txt but this makeapi seems to suggest the DNS api is not checked.
>
> This fix is in validate_doc(), which tries to check that all our
> commands have proper __doc__ string. It fails with the new DNS API
> commands (dnsrecordmx-add etc.) because it cannot find class definitions
> in dns.py. This is expected as I generate all these LDAP command classes
> on the fly from new DNSRecord parameters.

Ok, that's fine then.

>
> Martin
>
>>
>> Otherwise generally looks good.
>>
>> rob
>
>