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

Pavel Brezina pbrezina at redhat.com
Mon Apr 8 21:05:11 UTC 2013



----- Original Message -----
> From: "Jakub Hrozek" <jhrozek at redhat.com>
> To: sssd-devel at lists.fedorahosted.org
> Sent: Monday, April 8, 2013 10:38:47 PM
> Subject: Re: [SSSD] [PATCH] DNS site support 1st wave - generic SRV lookup plugin
> 
> 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.
> > > 
> > > 
> > 
> > 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);
> > ?
> > 
> > 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.
> > 
> > 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?
> > 
> > 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".
> > 
> > Same question about strdup-ing input parameters as for patch #2.
> > 
> > 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 ?
> > 
> > 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.
> > 
> > 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.
> > 
> > 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 :)
> > 
> > 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?
> > 
> > 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.

What exactly looks wrong to you? The high error code? That's OK - it is custom sss error.



More information about the sssd-devel mailing list