From: "Jakub Hrozek" <jhrozek(a)redhat.com>
To: sssd-devel(a)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.