[SSSD] [PATCH] DNS site support 1st wave - generic SRV lookup plugin

Pavel Březina pbrezina at redhat.com
Tue Apr 9 12:35:24 UTC 2013


On 04/08/2013 10:55 PM, Jakub Hrozek wrote:
> On Mon, Apr 08, 2013 at 10:38:47PM +0200, Jakub Hrozek wrote:
>> On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
>>> On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
>>>> On 04/05/2013 01:43 PM, Pavel Březina wrote:
>>>>> On 04/05/2013 12:41 PM, Pavel Březina wrote:
>>>>>> On 04/03/2013 11:48 AM, Pavel Březina wrote:
>>>>>>> On 04/02/2013 03:55 PM, Pavel Březina wrote:
>>>>>>>> On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
>>>>>>>>> On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
>>>>>>>>>> Later, we can place the plugin architecture inside a module that
>>>>>>>>>> knows about SSSD and is placed above resolver. You said you want to
>>>>>>>>>> decouple fail over to fail over -> cache -> resolver. This is nice
>>>>>>>>>> idea.
>>>>>>>>>>
>>>>>>>>>> The plugin architecture can be put inside the cache, because cache
>>>>>>>>>> is responsible for resolving expired uris and _srv_ and it does not
>>>>>>>>>> really say that is has to use resolver directly.
>>>>>>>>>
>>>>>>>>> OK, this would be the best approach. After more discussion on #sssd
>>>>>>>>> with Pavel and Simo we agreed that Pavel will carry on with this
>>>>>>>>> approach for the time being.
>>>>>>>>>
>>>>>>>>> The only two changes in these patches would be better namespacing of
>>>>>>>>> the functions used and unifying the resolver initialization with what
>>>>>>>>> I'm working on wrt dynamic DNS updates.
>>>>>>>>>
>>>>>>>>> In the next release, we will create the additional layer and move
>>>>>>>>> the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
>>>>>>>>> which is tracking that effort to NEEDS_TRIAGE so we can put it into
>>>>>>>>> an appropriate milestone tomorrow.
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>> I'm sending new patch set.
>>>>>>>>
>>>>>>>> I created async_resolv_utils.c and put there functions for domain
>>>>>>>> detection and for resolving SRV with fallback discovery domains.
>>>>>>>> This is
>>>>>>>> similar to resolve_srv_send and resolve_get_domain_send from the first
>>>>>>>> patchset.
>>>>>>>>
>>>>>>>> resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
>>>>>>>> resolv_discover_srv_send() so that it returns fo_server_info instead of
>>>>>>>> ares.
>>>>>>>>
>>>>>>>> The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
>>>>>>>> more complicated since it performs domain detection (originally in
>>>>>>>> resolve_srv_send()) if the domain is not provided from fail over.
>>>>>>>>
>>>>>>>> Hostname is passed via plugin context. It is currently retrieved via
>>>>>>>> gethostname() so the current behaviour does not change. Later, when the
>>>>>>>> plugin initialization will be moved to provider code, the provider will
>>>>>>>> decide what hostname will be used (ipa_hostname for example).
>>>>>>>
>>>>>>> Rebased on top of latest be_res patches.
>>>>>>
>>>>>> When working on IPA plugin I've done some changes to this code. Since it
>>>>>> is still review pending, I have squashed the changes to this patch set.
>>>>>>
>>>>>> 1. all SRV plugin related things are in a separate header file named
>>>>>> fail_over_srv.h. This way we don't have to #include the whole fail over
>>>>>> when developing new plugin. The only thing which remained in fail_over.h
>>>>>> is fo_set_srv_lookup_plugin(), because it should remain hidden for
>>>>>> providers, see #2
>>>>>>
>>>>>> 2. be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
>>>>>> for providers.
>>>>>>
>>>>>> 3. the original sixth patch (set plugin for all providers) was meant to
>>>>>> be reverted, once we agreed on further steps. Now it is completely
>>>>>> rewritten to final solution on which we agreed with Jakub.
>>>>>>
>>>>>> I have the IPA plugin already written (not tested yet), so I believe
>>>>>> this is the final version if there won't be any review comments.
>>>>> //lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>>
>>>>> OK. One more time :-)
>>>>> I have slightly enhanced debug messages.
>>>>
>>>> I forgot to check if a server is already present in the list. The fifth
>>>> patch was split in two.

Segfault fixed.

>>> Patch 0001: resolv: add resolv_get_domain request to resolv utils
>>> Mostly OK, can you just explain this part of the code?
>>>
>>> 152     dns_domain = talloc_strdup(state, dns_domain);
>>> 153     if (dns_domain == NULL) {
>>> 154         return ENOMEM;
>>> 155     }
>>> 156
>>> 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
>>> 158
>>> 159     return EOK;
>>>
>>> Isn't it better to just:
>>>      *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
>>> ?

I think it is better to not touch output parameters at all in case of 
failure. But I don't feel so strong about it. Done.

>>> Patch 0002: resolv: add resolv_discover_srv request to resolv utils
>>> Is it necessary to strdup the input variables? It might be safer in
>>> case this request outlives the request that created it but in most cases
>>> it's not really needed. The discovery_domain, on the other hand is
>>> neither checked not duped.

I remember fixing similar issue in resolver some time ago. So that
probably made me write more robust code here.
58b335985e75672e4de699351ab1182cbd7aa990

But you are right that it is unlikely to happen here.
Removed strdup() and added check for discovery_domain.

>>> Patch 0003: DNS sites support - SRV lookup plugin interface
>>> Code-wise looks good, but I have two questions related to the behaviour.
>>>
>>> Why can't the setter overwrite the plugin interface when it's already
>>> set? Isn't is nice for cases like LDAP where LDAP might set its generic
>>> plugin first and then IPA which uses LDAP underneath would reset them?

We agreed that id_provider is authoritative for plugin choise. And 
id_provider is initialized at first. Therefore it is not desirable to 
override it. It is described in #0007 commit message, but maybe I should 
put a comment here as well.

>>> Patch 0004: DNS sites support - SRV DNS lookup plugin
>>> Please do not use constructs like this unless EOK is expected:
>>>
>>> 66 immediately:
>>> 67     if (ret == EOK) {
>>> 68         tevent_req_done(req);
>>> 69     } else {
>>>
>>> At the very least it's confusing Coverity and showing up as "dead code".

Done.

>>> Same question about strdup-ing input parameters as for patch #2.

Done.

>>>
>>> I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
>>> -- standard by what means? What about using the same naming standard as
>>> in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?

Done.

>>> Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
>>> failover would not really be usable, it should return errno and users
>>> should check it.

Done.

>>> Patch 0005: fail over - add function to insert multiple servers to the
>>> list
>>> Ack
>>>
>>> Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
>>> call
>>> Did you check if the code can be compiled with this patch? If it can,
>>> then it's fine that the lookups don't work but I would prefer the
>>> patches to compile on their own so that git-bisect is usable.

Yes, it compiles fine.

>>>
>>> Otherwise ack.
>>>
>>> Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
>>> Mostly ack, but when I looked at how the host name is selected in LDAP
>>> initialization, it seemed to be that this could be moved down the stack
>>> either to be_fo_set_standard_srv_lookup_plugin or even lower to
>>> resolv_utils. Anyway, looks ugly in the generic LDAP code :)

Moved to be_fo_set_dns_srv_lookup_plugin().

>>> Patch 0008: DNS sites support - make fo_discover_srv request visible
>>> I don't get the point of this patch, why not make the functions pubilc
>>> from the start?

Darn, this belonged to the IPA plugin patch. But yes, it can be also 
squashed in the forth patch.

>>> So far I haven't tested the patches, just read the code to give some
>>> feedback. I'll reply again when I've tested it.
>>
>> My testing consisted of positive tests with the IPA (including the one
>> patch in the other thread) and LDAP backends, those went fine. Negative
>> tests worked OK, too but revealed one probably uninitialized variable:
>>
>> [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
>> [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
>>                                   ^^^^^^
>>                                   seems wrong
>> [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
>> [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
>> [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!
>>
>> This occured when I queried a server that didn't have the query stored
>> at all.
>>
>> The query refresh seemed to worked OK also (I tested by manually
>> modifiying the timeout). Except for the uninitialized variable, I think
>> the patches are working good and we just need to resolve the minor code
>> concerns above.
>
> You also need to put the new header files (src/providers/fail_over_srv.h
> and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
> Makefile.am

Done.

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-resolv-add-resolv_get_domain-request-to-resolv-utils.patch
Type: text/x-patch
Size: 7066 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-resolv-add-resolv_discover_srv-request-to-resolv-uti.patch
Type: text/x-patch
Size: 6926 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-DNS-sites-support-SRV-lookup-plugin-interface.patch
Type: text/x-patch
Size: 9768 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-DNS-sites-support-SRV-DNS-lookup-plugin.patch
Type: text/x-patch
Size: 19334 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-fail-over-add-function-to-insert-multiple-servers-to.patch
Type: text/x-patch
Size: 4604 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-DNS-sites-support-replace-SRV-lookup-code-with-a-plu.patch
Type: text/x-patch
Size: 15190 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-DNS-sites-support-use-SRV-DNS-lookup-plugin-in-all-p.patch
Type: text/x-patch
Size: 3757 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130409/0e89c8a9/attachment-0006.bin>


More information about the sssd-devel mailing list