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

Jakub Hrozek jhrozek at redhat.com
Mon Apr 8 14:51:47 UTC 2013


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.

There is quite some new functionality in this patchset that would be
great to have covered by unit-tests. I realize we need to finish the
code first and foremost, so maybe we could file tickets (trivial or
minor) and fix them in 1.10 post-beta? But I would like to start
building the coverage, if we don't even file the ticket, we'll forget
completely.



More information about the sssd-devel mailing list