On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote: > > > On 06/22/2016 06:25 AM, Petr Cech wrote: > > On 06/14/2016 05:21 PM, Petr Cech wrote: > > > Hello list, > > > > > > there is patch set attached resolving: > > > https://fedorahosted.org/sssd/ticket/2828 > > > > > > You can also see: > > > https://github.com/celestian/sssd/commits/subdomains_v2 > > > > > > > > > This patch set is without tests yet. > > > I will send tests after Pavel B. refactor of AD provider. > > > > > > > > > I would like to remark couple of things: > > > > > > 1) Option ad_enabled_domains disabled by default. > > > > > > 2) We use labels master domain and root domain in code. Master domain > > > belongs to one that SSSD is connected to. Root domain means AD forest > > > root. > > > > > > 3) There is a lot of different environment setup. So this option has > > > slightly different meaning in each of them. > > > > > > 4) There is secret feature if you forget fill in master domain to the > > > option it will be added automatically. > > > > > > > > > What is this option good for? > > > Let's imagine environment like: > > > > > > A (forest root) > > > | > > > |-- S1 (SSSD client) > > > | > > > |-- B (AD client) > > > | | > > > | |-- S2 (SSSD client) > > > | > > > |-- C (AD client) > > > | > > > |-- D (AD client) > > > > > > A ... forest root > > > B, C, D ... AD clients > > > S1 ... SSSD client connected to A > > > S2 ... SSSD client connected to B > > > > > > We could write: > > > > > > * on S1: ad_enabled_domain = A, B, C > > > and then S1 will not attempt to connect D. > > > > > > * on S1: ad_enabled_domain = B, C > > > the same as above because A is automagically > > > filled in. > > > > > > * on S2: ad_enabled_domain = A, C > > > and then B is filled in automatically > > > and S2 will not attempt to connect D. > > > > > > * on S2: ad_enabled_domain = C > > > and then B is filled in automatically > > > and S2 will not attempt to connect D and A. > > > (I am wondering that it is possible but it > > > happened during manual testing.) > > > > Hello list, > > > > I rebased this patch set due to refactoring of the data provider. New > > patch set is attached. > > > > I will add tests, I hope soon :-) > > Hello list, > > there is final patch. > I added tests and I fix some char * to const char * too. > > Tests cover only option handling. More complex tests should be done as > integrating tests, not as unit tests.
Hi, see some inline comments. So far, I didn't test the patches, just read the code. > From 63610f3be85821c6daa98bc6b2b3be438520c82d Mon Sep 17 00:00:00 2001 > From: Petr Cech <[email protected]> > Date: Fri, 13 May 2016 05:21:07 -0400 > Subject: [PATCH 1/5] AD_PROVIDER: Add ad_enabled_domains option > > Resolves: > https://fedorahosted.org/sssd/ticket/2828 We need to add this option to the new config schema (it wasn't in master when you started writing the patches). > From e6b6c461ec7d4e91299b1e8290e1998e21aa9576 Mon Sep 17 00:00:00 2001 > From: Petr Cech <[email protected]> > Date: Tue, 21 Jun 2016 08:34:15 +0200 > Subject: [PATCH 2/5] AD_PROVIDER: Initializing of ad_enabled_domains > > We add ad_enabled_domains into ad_subdomains_ctx. > > Resolves: > https://fedorahosted.org/sssd/ticket/2828 > --- > src/providers/ad/ad_subdomains.c | 94 > ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 94 insertions(+) > > diff --git a/src/providers/ad/ad_subdomains.c > b/src/providers/ad/ad_subdomains.c > index > 5b0bee86665a46f45fde5ec1034a9ccf8700d13a..af438078deb8b103f8a24dbef7cf818fab9df538 > 100644 > --- a/src/providers/ad/ad_subdomains.c > +++ b/src/providers/ad/ad_subdomains.c > @@ -57,6 +57,91 @@ > /* do not refresh more often than every 5 seconds for now */ > #define AD_SUBDOMAIN_REFRESH_LIMIT 5 > > +static errno_t ad_get_enabled_domains(TALLOC_CTX *mem_ctx, > + struct ad_id_ctx *ad_id_ctx, > + const char *ad_domain, > + const char ***_ad_enabled_domains) > +{ > + int ret; > + const char *str; > + const char *option_name; > + char **domains = NULL; > + const char **list = NULL; > + int count; > + bool is_ad_in_domains; > + TALLOC_CTX *tmp_ctx = NULL; > + > + tmp_ctx = talloc_new(NULL); > + if (tmp_ctx == NULL) { > + return ENOMEM; > + } > + > + str = dp_opt_get_cstring(ad_id_ctx->ad_options->basic, > AD_ENABLED_DOMAINS); > + if (str == NULL) { > + _ad_enabled_domains = NULL; > + ret = EOK; > + goto done; > + } > + > + count = 0; > + ret = split_on_separator(tmp_ctx, str, ',', true, true, &domains, > &count); > + if (ret != EOK) { > + option_name = > ad_id_ctx->ad_options->basic[AD_ENABLED_DOMAINS].opt_name; > + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to parse option [%s], [%i] > [%s]!\n", > + option_name, ret, sss_strerror(ret)); > + ret = EINVAL; > + goto done; > + } > + > + list = talloc_array_size(tmp_ctx, sizeof(char*), count); > + if (list == NULL) { > + ret = ENOMEM; > + goto done; > + } > + > + is_ad_in_domains = false; > + for (int i = 0; i < count; i++) { > + list[i] = talloc_strdup(list, domains[i]); Do we need to duplicate the string here, wouldn't it be enough to steal it? > + if (list[i] == NULL) { > + ret = ENOMEM; > + goto done; > + } > + > + is_ad_in_domains += strcmp(ad_domain, domains[i]) == 0 ? true : > false; > + } > + > + if (is_ad_in_domains == false) { > + list = talloc_realloc(tmp_ctx, list, const char*, count + 1); > + if (list == NULL) { > + ret = ENOMEM; > + goto done; > + } > + > + list[count] = talloc_strdup(list, ad_domain); > + if (list[count] == NULL) { > + ret = ENOMEM; > + goto done; > + } > + > + count++; > + } > + > + list = talloc_realloc(tmp_ctx, list, const char*, count + 1); Why don't you allocate count+1 from the start but realloc here instead? > + if (list == NULL) { > + ret = ENOMEM; > + goto done; > + } > + list[count] = NULL; > + count++; > + > + *_ad_enabled_domains = talloc_steal(mem_ctx, list); > + ret = EOK; > + > +done: > + talloc_free(tmp_ctx); > + return ret; > +} > From 6f5f06a892ff49c256e2476b5c365709cbf73164 Mon Sep 17 00:00:00 2001 > From: Petr Cech <[email protected]> > Date: Tue, 21 Jun 2016 09:48:52 +0200 > Subject: [PATCH 3/5] AD_PROVIDER: ad_enabled_domains - only master > > We can skip looking up other domains if option ad_enabled_domains > contains only master domain. > > Resolves: > https://fedorahosted.org/sssd/ticket/2828 > --- > src/providers/ad/ad_subdomains.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/src/providers/ad/ad_subdomains.c > b/src/providers/ad/ad_subdomains.c > index > af438078deb8b103f8a24dbef7cf818fab9df538..6fa134eed69aa8f0a672bc53c9b0d7dd0d7f37b1 > 100644 > --- a/src/providers/ad/ad_subdomains.c > +++ b/src/providers/ad/ad_subdomains.c > @@ -1170,6 +1170,7 @@ static void ad_subdomains_refresh_connect_done(struct > tevent_req *subreq) > return; > } > > + /* connect to the DC we are a member of */ > subreq = ad_master_domain_send(state, state->ev, state->id_ctx->conn, > state->sdap_op, > state->sd_ctx->domain_name); > if (subreq == NULL) { > @@ -1219,6 +1220,21 @@ static void ad_subdomains_refresh_master_done(struct > tevent_req *subreq) > goto done; > } > > + /* > + * If ad_enabled_domains contains only master domain > + * we shouldn't lookup other domains. > + */ > + if (state->sd_ctx->ad_enabled_domains != NULL) { > + if (talloc_array_length(state->sd_ctx->ad_enabled_domains) == 2) { > + if (strcmp(state->sd_ctx->ad_enabled_domains[0], > + state->be_ctx->domain->name) == 0) { > + DEBUG(SSSDBG_TRACE_FUNC, > + "No other enabled domain than master.\n"); > + goto done; > + } > + } > + } > + > subreq = ad_get_root_domain_send(state, state->ev, forest, > sdap_id_op_handle(state->sdap_op), > state->sd_ctx); > -- > 2.5.5 > > From 3dcc8efe27567bfa5300db1dd5e971571b671706 Mon Sep 17 00:00:00 2001 > From: Petr Cech <[email protected]> > Date: Mon, 27 Jun 2016 11:51:30 +0200 > Subject: [PATCH 4/5] AD_PROVIDER: ad_enabled_domains - other then master > > We can skip looking up other domains if > option ad_enabled_domains doesn't contain them. > > Resolves: > https://fedorahosted.org/sssd/ticket/2828 > --- > src/providers/ad/ad_subdomains.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/src/providers/ad/ad_subdomains.c > b/src/providers/ad/ad_subdomains.c > index > 6fa134eed69aa8f0a672bc53c9b0d7dd0d7f37b1..f9e4ac57904514c89bcc3a8160194fd0be246a21 > 100644 > --- a/src/providers/ad/ad_subdomains.c > +++ b/src/providers/ad/ad_subdomains.c > @@ -497,6 +497,7 @@ done: > > static errno_t ad_subdomains_process(TALLOC_CTX *mem_ctx, > struct sss_domain_info *domain, > + const char **enabled_domains_list, > size_t nsd, struct sysdb_attrs **sd, > struct sysdb_attrs *root, > size_t *_nsd_out, > @@ -505,9 +506,11 @@ static errno_t ad_subdomains_process(TALLOC_CTX *mem_ctx, > size_t i, sdi; > struct sysdb_attrs **sd_out; > const char *sd_name; > + const bool is_sd_filtered = (enabled_domains_list == NULL) ? false : > true; > + bool is_sd_enabled; > errno_t ret; > > - if (root == NULL) { > + if (root == NULL && is_sd_filtered == false) { > /* We are connected directly to the root domain. The 'sd' > * list is complete and we can just use it > */ > @@ -534,6 +537,17 @@ static errno_t ad_subdomains_process(TALLOC_CTX *mem_ctx, > goto fail; > } > > + is_sd_enabled = true; > + if (is_sd_filtered == true) { > + is_sd_enabled = false; > + for (size_t j = 0; enabled_domains_list[j] != NULL; j++) { > + if (strcmp(sd_name, enabled_domains_list[j]) == 0) { > + is_sd_enabled = true; > + break; > + } > + } > + } > + Wouldn't it be easier to just call if (string_in_list(sd_name, enabled_domains_list) == false) { continue; } and always append the domain if neither the filtering condition nor the subdomain-is-root matches? > From b8ce2c188f081ac3073c68c4b57e0371984faa58 Mon Sep 17 00:00:00 2001 > From: Petr Cech <[email protected]> > Date: Mon, 27 Jun 2016 11:53:19 +0200 > Subject: [PATCH 5/5] TESTS: Adding tests for ad_enabled_domains option > > There is special logic around ad_enabled_domains option: > * option is disabled by default > * master domain is always added to enabled domains > > Resolves: > https://fedorahosted.org/sssd/ticket/2828 LGTM. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
