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

Jakub Hrozek jhrozek at redhat.com
Mon Apr 8 21:40:35 UTC 2013


On Mon, Apr 08, 2013 at 05:05:11PM -0400, Pavel Brezina wrote:
> 
> 
> ----- 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.

Yes, but I didn't realize the new error code base was set so high
(0x555D0000) Sorry, carry on :)



More information about the sssd-devel mailing list