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

Jakub Hrozek jhrozek at redhat.com
Mon Mar 25 16:18:27 UTC 2013


On Fri, Mar 22, 2013 at 02:47:31PM +0100, Pavel Březina wrote:
> Hi,
> this patchset takes the current SRV lookup code and converts it into a
> plugin. Next waves will add custom plugin for IPA and AD.
> 
> Patch 1:
> Introduce new plugin interface.
> 
> Patch 2:
> First SRV lookup plugin.
> 
> Patch 3:
> Replaces current code with a plugin code. I originally wanted to remove
> resolve_srv_* functions completely and call the plugin in
> fo_resolve_service_send() directly but that proved to be harder and
> more error sensitive than I expected.
> 
> This got me thinking, we should create a refactoring ticket for the
> failover code. It has become quite complex and hard to read and
> understand because it lacks a lot of comments. Also many things there
> seems very hackish to me.
> 

Feel free to create that ticket, I agree that the current failover code
has grown too much..but keep in mind that many of the "hacks" are in fact
special cases for all kinds of weird errors.

My idea was to decouple the caching logic in the failover to a separate
layer. So resolver would always talk to DNS or whatever back end there
was, then there would be a resolver cache and finally fail over that
would only manage states of servers and ports.

Is there anything else that bothers you with the current failover
architecture?

> Patch 4:
> Set SRV lookup plugin for all providers. This is done as a separate
> patch because it will be reverted once each provider has its own plugin.
> 
> I also would like to discuss where we can set the plugin in the future.
> At the moment we have one failover context for the whole backend. But
> the backend can be configured to run several different providers at the
> same time. The plugin itself is made per provider - you have different
> plugin for IPA and different for LDAP.
> 
> I think we should have failover context per provider. My idea is to
> create a new module constructor e.g. sssm_ipa_init(). This will be
> called only once before all other sssm_ipa_*_init() functions and it
> will initialize IPA-wise failover context.
> 

In theory there may be a case where we need this functionality, yes. I
can't think of any use-case right now, but this may be a better
architecture going forward, especially as we are adding more providers
like sudo or autofs that are only implemented with some back ends.

> We can also use it to initialize sdap_id_ctx instead of calling
> sssm_ipa_id_init from other places.

Please see comments inline.

> From 4391f7f47317f3c5664f577b833ef91d3f8e15e2 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Tue, 19 Mar 2013 15:53:44 +0100
> Subject: [PATCH 1/4] DNS sites support - SRV lookup plugin interface
> 
> https://fedorahosted.org/sssd/ticket/1032
> 
> Introduces two new error codes:
> - ERR_DNS_SRV_NOT_FOUND
> - ERR_DNS_SRV_LOOKUP_ERROR
> ---
>  src/providers/fail_over.c | 18 +++++++++++++++++
>  src/providers/fail_over.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/util_errors.c    |  2 ++
>  src/util/util_errors.h    |  2 ++
>  4 files changed, 73 insertions(+)
> 
> diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
> index e7c44174ded773a8e3bb99dc436c45d4e8ca277d..660fe8261aa279735af2b8d9d31596acbad19810 100644
> --- a/src/providers/fail_over.c
> +++ b/src/providers/fail_over.c
> @@ -55,6 +55,10 @@ struct fo_ctx {
>      struct server_common *server_common_list;
>  
>      struct fo_options *opts;
> +
> +    srv_lookup_plugin_send_t srv_send_fn;
> +    srv_lookup_plugin_recv_t srv_recv_fn;
> +    void *srv_pvt;

The design page we agreed on describes that these callbacks should have
been in the resolv context. Any reason to move them to fo_ctx ?

The failover should be providing logic for moving between servers and
ports and just call "resolve a service". The logic on how to resolve a
service should be in the resolver I think. Also see the fo_server_info
structure -- this seems exactly like code that belongs to resolver.

>  };
>  
>  struct fo_service {
> @@ -1591,3 +1595,17 @@ bool fo_svc_has_server(struct fo_service *service, struct fo_server *server)
>  
>      return false;
>  }
> +
> +void fo_set_srv_lookup_plugin(struct fo_ctx *ctx,
> +                              srv_lookup_plugin_send_t send_fn,
> +                              srv_lookup_plugin_recv_t recv_fn,
> +                              void *pvt)
> +{
> +    if (ctx == NULL) {
> +        return;
> +    }
> +
> +    ctx->srv_send_fn = send_fn;
> +    ctx->srv_recv_fn = recv_fn;
> +    ctx->srv_pvt = talloc_steal(ctx, pvt);
> +}
> diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h
> index 1ad081e78c866390a1a345feacc5c0899adf91a4..1ff06688237b5fde1d78878f45bd87f7ca1df8cf 100644
> --- a/src/providers/fail_over.h
> +++ b/src/providers/fail_over.h
> @@ -198,4 +198,55 @@ void fo_reset_services(struct fo_ctx *fo_ctx);
>  
>  bool fo_svc_has_server(struct fo_service *service, struct fo_server *server);
>  
> +/* SRV lookup plugin interface */
> +
> +struct fo_server_info {
> +    char *host;
> +    int port;
> +};
> +

OK, in the design we talked about reusing ares_srv structure..you are
right we might as well create a wrapper, but it should be in resolver,
not failover. A failover server (a bunch of services with status) is a
completely different concept than resolver server (host name an port).

> @@ -63,6 +63,8 @@ enum sssd_errors {
>      ERR_ACCOUNT_EXPIRED,
>      ERR_PASSWORD_EXPIRED,
>      ERR_ACCESS_DENIED,
> +    ERR_DNS_SRV_NOT_FOUND,
> +    ERR_DNS_SRV_LOOKUP_ERROR,
>      ERR_LAST            /* ALWAYS LAST */
>  };

Wouldn't it be better to create generic SRV lookup error codes that
could be reused also for AD site discovery (which is not DNS).

> diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h
> index 1ff06688237b5fde1d78878f45bd87f7ca1df8cf..0ab87d3230408b420a3d78e30b335ab9902c7407 100644
> --- a/src/providers/fail_over.h
> +++ b/src/providers/fail_over.h
> @@ -249,4 +249,30 @@ void fo_set_srv_lookup_plugin(struct fo_ctx *ctx,
>                                srv_lookup_plugin_recv_t recv_fn,
>                                void *pvt);
>  
> +/* Simple SRV lookup plugin */
> +
> +struct resolve_srv_simple_ctx;

The namespacing seems wrong to me. There is a resolv_ namespace in the
resolver code and fo_ namespace in failover code, this is third one and
similar to resolv_ ... very confusing.

> +
> +struct resolve_srv_simple_ctx *
> +resolve_srv_simple_context_init(TALLOC_CTX *mem_ctx,
> +                                struct resolv_ctx *resolv_ctx,
> +                                enum host_database *host_dbs,
> +                                enum restrict_family family_order,
> +                                char *sssd_domain);

I think this is a good idea to couple together the databases, resolv_ctx
and the family order because in most cases, they are simply used together,
while allowing the low level resolver calls to specify all these parameters.

But this new structure should be owned by the user of the resolver, which
is currently back end and might be provider in the future.

> +
> +struct tevent_req *resolve_srv_simple_send(TALLOC_CTX *mem_ctx,
> +                                           struct tevent_context *ev,
> +                                           const char *service,
> +                                           const char *protocol,
> +                                           const char *discovery_domain,
> +                                           void *pvt);
> +

I don't like the _simple name either, sorry. If there is a more advanced
static interface, then the simple should be called just _srv_send/recv
and the advanced _ext, I think. The reason is to have nice and readable
names for the public interface.

> +errno_t resolve_srv_simple_recv(TALLOC_CTX *mem_ctx,
> +                                struct tevent_req *req,
> +                                char **_dns_domain,
> +                                struct fo_server_info **_primary_servers,
> +                                size_t *_num_primary_servers,
> +                                struct fo_server_info **_backup_servers,
> +                                size_t *_num_backup_servers);
> +

> +struct resolve_srv_domain_state {
> +    char *fqdn;
> +    char hostname[HOST_NAME_MAX];
> +};
> +
> +static void resolve_srv_domain_done(struct tevent_req *subreq);
> +
> +static struct tevent_req *
> +resolve_srv_domain_send(TALLOC_CTX *mem_ctx,
> +                        struct tevent_context *ev,
> +                        struct resolv_ctx *resolv_ctx,
> +                        enum host_database *host_dbs,
> +                        enum restrict_family family_order)
> +{

I'm not opposed to splitting this code to a separate request and move
away from failover, but because it only calls resolv_ functions directly
it should be placed in the resolver code.

> +    struct resolve_srv_domain_state *state = NULL;
> +    struct tevent_req *req = NULL;
> +    struct tevent_req *subreq = NULL;
> +    errno_t ret;
> +
> +    req = tevent_req_create(mem_ctx, &state,
> +                            struct resolve_srv_domain_state);
> +    if (req == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("tevent_req_create() failed\n"));
> +        return NULL;
> +    }
> +
> +    state->fqdn = NULL;
> +
> +    ret = gethostname(state->hostname, HOST_NAME_MAX);
> +    if (ret) {
> +        ret = errno;
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("gethostname() failed: [%d]: %s\n",
> +                                    ret, strerror(ret)));
> +        goto immediately;
> +    }

I know that we have the same situation in the current code, but the host
name should be an input parameter. Some back ends allow you to override the
client host name (ipa_hostname parameter comes to mind).

<snip>

> +struct resolve_srv_state {
> +    struct tevent_context *ev;
> +    struct resolv_ctx *resolv_ctx;
> +    char *service;
> +    char *protocol;
> +    char *detected_domain;
> +    const char **discovery_domains;
> +    int domain_index;
> +
> +    struct fo_server_info *servers;
> +    size_t num_servers;
> +};
> +

Please move this whole request to resolver. Feel free to split the
resolver code into more modules and introduce a structure analogous to
the fo_server_info one.

> +static void resolve_srv_step1_done(struct tevent_req *subreq);

Ugh, step1 is not the greatest name :)

I think we should resume review of the other patches when we settle
which is the best place to implement the plugin architecture.



More information about the sssd-devel mailing list