On 04/16/2013 03:03 PM, Ana Krivokapic wrote:
> On 04/16/2013 01:55 PM, Martin Kosek wrote:
>> On 04/16/2013 12:31 PM, Ana Krivokapic wrote:
>>> On 04/16/2013 09:14 AM, Martin Kosek wrote:
>>>> On 04/15/2013 11:21 PM, Rob Crittenden wrote:
>>>>> Ana Krivokapic wrote:
>>>>>> On 04/15/2013 07:06 PM, Martin Kosek wrote:
>>>>>>> On 04/15/2013 06:53 PM, Ana Krivokapic wrote:
>>>>>>>> On 04/15/2013 06:30 PM, Martin Kosek wrote:
>>>>>>>>> On 04/12/2013 08:45 PM, Ana Krivokapic wrote:
>>>>>>>>>> On 04/12/2013 01:26 PM, Ana Krivokapic wrote:
>>>>>>>>>>> On 04/12/2013 12:44 PM, Martin Kosek wrote:
>>>>>>>>>>>> On 04/12/2013 12:20 PM, Ana Krivokapic wrote:
>>>>>>>>>>>>> On 04/11/2013 03:03 PM, Alexander Bokovoy wrote:
>>>>>>>>>>>>>> On Thu, 11 Apr 2013, Ana Krivokapic wrote:
>>>>>>>>>>>>>>> On 04/11/2013 01:43 PM, Alexander Bokovoy wrote:
>>>>>>>>>>>>>>>> On Thu, 11 Apr 2013, Petr Spacek wrote:
>>>>>>>>>>>>>>>>> On 11.4.2013 13:24, Alexander Bokovoy wrote:
>>>>>>>>>>>>>>>>>> On Thu, 11 Apr 2013, Petr Spacek wrote:
>>>>>>>>>>>>>>>>>>> On 11.4.2013 13:09, Ana Krivokapic wrote:
>>>>>>>>>>>>>>>>>>>> Integrate realmdomains with IPA DNS
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Add an entry to realmdomains when a DNS zone is added to 
>>>>>>>>>>>>>>>>>>>> IPA.
>>>>>>>>>>>>>>>>>>>> Delete the
>>>>>>>>>>>>>>>>>>>> related entry from  realmdomains when the DNS zone is 
>>>>>>>>>>>>>>>>>>>> deleted from
>>>>>>>>>>>>>>>>>>>> IPA.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3544
>>>>>>>>>>>>>>>>>>> I would add a TXT record as I described in
>>>>>>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3544#comment:8
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> This integration probably should go to both commands,
>>>>>>>>>>>>>>>>>>> realmdomains-*
>>>>>>>>>>>>>>>>>>> dnszone-*.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Any objections? AB?
>>>>>>>>>>>>>>>>>> Adding TXT record is probably harmless.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I would actually add the TXT record creation only to
>>>>>>>>>>>>>>>>>> realmdomains-* and
>>>>>>>>>>>>>>>>>> trigger it only in case we manage our DNS and DNS zone is 
>>>>>>>>>>>>>>>>>> there.
>>>>>>>>>>>>>>>>>> This way a hook from dnszone-add will trigger adding TXT 
>>>>>>>>>>>>>>>>>> record back
>>>>>>>>>>>>>>>>>> (via call to
>>>>>>>>>>>>>>>>>> realmdomains-mod --add and then TXT record addition from 
>>>>>>>>>>>>>>>>>> there).
>>>>>>>>>>>>>>>>>> Also
>>>>>>>>>>>>>>>>>> the fact that admin added manually some domain to 
>>>>>>>>>>>>>>>>>> realmdomains
>>>>>>>>>>>>>>>>>> mapping
>>>>>>>>>>>>>>>>>> means that it is implied to be used in obtaining TGTs, so TXT
>>>>>>>>>>>>>>>>>> record is
>>>>>>>>>>>>>>>>>> helpful there as well.
>>>>>>>>>>>>>>>>> Okay, it makes sense. We will see how it will work in reality.
>>>>>>>>>>>>>>>> One more thing to check is that we don't do this for our own 
>>>>>>>>>>>>>>>> domain.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Our own domain is already in realmdomains by default, and it 
>>>>>>>>>>>>>>> cannot be
>>>>>>>>>>>>>>> removed from there. So I don't think any check related to our 
>>>>>>>>>>>>>>> domain is
>>>>>>>>>>>>>>> necessary.
>>>>>>>>>>>>>> We shouldn't start creating TXT records for our own domain, 
>>>>>>>>>>>>>> that's what
>>>>>>>>>>>>>> I'm asking for here.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Think about server install stage -- we start creating our own 
>>>>>>>>>>>>>> domain and
>>>>>>>>>>>>>> the hook then causes to create realmdomains entry for the domain,
>>>>>>>>>>>>>> causing realmdomains-mod code to raise ValidationError which is 
>>>>>>>>>>>>>> not
>>>>>>>>>>>>>> handled in dnszone-add code with this patch.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Same for TXT record creation starting from realmdomains-mod side 
>>>>>>>>>>>>>> -- it
>>>>>>>>>>>>>> simply should avoid calling dnsrecord-add for the case we know 
>>>>>>>>>>>>>> wouldn't
>>>>>>>>>>>>>> work.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> I just realized that this ticket was not marked as RFE although it
>>>>>>>>>>>>> obviously is
>>>>>>>>>>>>> one. I fixed the ticket summary and wrote the design page for this
>>>>>>>>>>>>> enhancement:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://www.freeipa.org/page/V3/DNS_realmdomains_integration
>>>>>>>>>>>>>
>>>>>>>>>>>> Right, that was a good thing to do. I just have comment for the UPN
>>>>>>>>>>>> enumeration
>>>>>>>>>>>> image which you linked in the RFE - can you please process it, 
>>>>>>>>>>>> upload
>>>>>>>>>>>> to the
>>>>>>>>>>>> wiki and include in the overview? This will make the RFE page more
>>>>>>>>>>>> appealing
>>>>>>>>>>>> and it will also prevent us from having a broken link when 
>>>>>>>>>>>> Alexander
>>>>>>>>>>>> removes
>>>>>>>>>>>> the file from his temporary directory.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Martin
>>>>>>>>>>> Sure, done.
>>>>>>>>>>>
>>>>>>>>>> I added the functionality to create TXT record to realmdomains-mod, 
>>>>>>>>>> and also
>>>>>>>>>> made sure that the case of our own domain is handled properly. Unit 
>>>>>>>>>> tests
>>>>>>>>>> have
>>>>>>>>>> been added to cover the new functionality. One unit test of the dns 
>>>>>>>>>> plugin
>>>>>>>>>> needed adjusting, but it still fails due to the bug in the testing
>>>>>>>>>> framework[1]. It should pass after the bug is fixed.
>>>>>>>>>>
>>>>>>>>>> Updated patch is attached.
>>>>>>>>>>
>>>>>>>>>> [1] https://fedorahosted.org/freeipa/ticket/3562
>>>>>>>>>>
>>>>>>>>> This looks nice, thanks for the new test cases.
>>>>>>>>>
>>>>>>>>> I experienced an issue with dnsrecord-find test in
>>>>>>>>> tests/test_xmlrpc/test_dns_plugin.py, but I see you already have an 
>>>>>>>>> open
>>>>>>>>> ticket
>>>>>>>>> to fix that (https://fedorahosted.org/freeipa/ticket/3562) so it is 
>>>>>>>>> not a
>>>>>>>>> show-stopper.
>>>>>>>>>
>>>>>>>>> This is a nitpick, but could you update
>>>>>>>>> tests/test_xmlrpc/test_dns_realmdomains_integration.py to use the same
>>>>>>>>> domains
>>>>>>>>> for testing as tests/test_xmlrpc/test_dns_plugin.py does?
>>>>>>>>>
>>>>>>>>> I often use example*.com zones in my testing and we also advertise 
>>>>>>>>> test
>>>>>>>>> commands with these zones in "ipa help dns" too, so I (and maybe 
>>>>>>>>> others)
>>>>>>>>> could
>>>>>>>>> get surprised that these zones are deleted after running the test 
>>>>>>>>> suite.
>>>>>>>>> I.e. I
>>>>>>>>> would prefer to have dnszone*.test used for test.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Martin
>>>>>>>> Sure. Updated patch attached.
>>>>>>>>
>>>>>>> One more nitpick (sorry for not spotting it earlier). In 
>>>>>>> realmdomains-mod, you
>>>>>>> do checks for zone/record before you do the dnsrecord-add/dnsrecord-del
>>>>>>> command.
>>>>>>>
>>>>>>> I think this will unnecessarily make the command slower. You can just 
>>>>>>> try
>>>>>>> add/delete a record and catch also a NotFound error - these commands 
>>>>>>> already
>>>>>>> check for zone/record existence, so we do not need to do the checks 
>>>>>>> twice.
>>>>>>>
>>>>>>> Relevant sections:
>>>>>>>
>>>>>>> +            try:
>>>>>>> +                api.Command['dnszone_show'](unicode(d))
>>>>>>> +            except errors.NotFound:
>>>>>>> +                pass
>>>>>>> +            else:
>>>>>>> +                try:
>>>>>>> +                    api.Command['dnsrecord_add'](
>>>>>>> +                        unicode(d),
>>>>>>> +                        u'_kerberos',
>>>>>>> +                        txtrecord=api.env.realm
>>>>>>> +                    )
>>>>>>> +                except errors.EmptyModlist:
>>>>>>> +                    pass
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>> +            try:
>>>>>>> +                api.Command['dnszone_show'](unicode(d))
>>>>>>> +            except errors.NotFound:
>>>>>>> +                pass
>>>>>>> +            else:
>>>>>>> +                try:
>>>>>>> +                    api.Command['dnsrecord_del'](
>>>>>>> +                        unicode(d),
>>>>>>> +                        u'_kerberos',
>>>>>>> +                        txtrecord=api.env.realm
>>>>>>> +                    )
>>>>>>> +                except errors.AttrValueNotFound:
>>>>>>> +                    pass
>>>>>>>
>>>>>>> Martin
>>>>>> Nice catch, those checks were totally unnecessary. Thanks.
>>>>> I tried adding an upper-case version of my primary domain and got an odd 
>>>>> error:
>>>>>
>>>>> $ ipa realmdomains-mod --add-domain EXAMPLE.COM
>>>>> ipa: ERROR: Type or value exists:
>>>>>
>>>>> Ok, I can sort of accept that, but there is no real detail.
>>>>>
>>>>> httpd logged this though:
>>>>>
>>>>> [Mon Apr 15 17:15:48.048169 2013] [:error] [pid 3515] ipa: INFO:
>>>>> ad...@example.com: realmdomains_mod(add_domain=u'EXAMPLE.COM', 
>>>>> rights=False,
>>>>> force=False, all=False, raw=False, version=u'2.57'): DatabaseError
>>>>>
>>>>> I'm really a little confused about this discrepancy. The client side looks
>>>>> right, I don't understand why we logged a DB error on the server.
>>>>>
>>>>> LDAP has the right error:
>>>>>
>>>>> [15/Apr/2013:17:15:47 -0400] conn=20 op=7 MOD dn="cn=Realm
>>>>> Domains,cn=ipa,cn=etc,dc=example,dc=com"
>>>>> [15/Apr/2013:17:15:47 -0400] conn=20 op=7 RESULT err=20 tag=103 
>>>>> nentries=0 etime=0
>>>>>
>>>>> What led me to try this was this code:
>>>>>
>>>>> +        for d in domains_added:
>>>>> +            # Skip our own domain
>>>>> +            if d == api.env.domain:
>>>>> +                continue
>>>>>
>>>>> This probably needs to be case insensitive.
>>>>>
>>>>> rob
>>>> We may also want to normalize zones to the non-fqdn DNS version before 
>>>> adding
>>>> them to realmdomains - i.e. store "example.com" instead of "example.com.".
>>>> Adding Alexander to CC list to verify this is a sound requirement.
>>>>
>>>> Otherwise, we would have duplicate zones in realmdomains object:
>>>>
>>>> # ipa dnszone-add example2.com --name-server=`hostname`.
>>>>
>>>> # ipa realmdomains-show
>>>>   Domain: idm.lab.bos.redhat.com, example2.com
>>>>
>>>> # ipa realmdomains-mod --add-domain=example2.com.
>>>>   Domain: idm.lab.bos.redhat.com, example2.com, example2.com.
>>>>
>>>> example2.com. should be normalized to example.com
>>>>
>>>>
>>>> # ipa dnszone-add example3.com. --name-server=`hostname`.
>>>>
>>>> # ipa realmdomains-show
>>>>   Domain: idm.lab.bos.redhat.com, example2.com, example2.com., 
>>>> example3.com.
>>>>
>>>> I think example3.com. should be normalized to example3.com
>>>>
>>>> Martin
>>> I added normalizers for realmdomains-mod options, which should fix
>>> issues encountered by Rob and Martin.
>>>
>>> Updated patch is attached.
>>>
>> Good! I think this addresses all concerns and issues we found. I have one 
>> last
>> minor issue and then we can push it.
>>
>> Please just add the normalizers to an own function as it is done in other
>> plugins. It is then much easier to change such normalizer in one central 
>> location.
>>
>> Thanks,
>> Martin
> 
> As we agreed on IRC, I implemented the following two changes:
> 
> 1) Create a separate normalizer function
> 2) Properly handle reverse zones (i.e. do not create realmdomains
> entries or TXT records for them)
> 
> Updated patch is attached.
> 

ACK. Pushed to master.

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to