[PATCHES] Fixes and improvements for ipa_server_mode

[Sumit Bose]

Hi,

I wrote the following patches while testig the ipa_server_mode. While
the first three are needed fixes the fourth patch is an improvement
which might help to avoid an additional request to the LDAP server.

bye,
Sumit

[Jakub Hrozek]

On Thu, Aug 15, 2013 at 01:07:47PM +0200, Sumit Bose wrote:
> Hi,
>
> I wrote the following patches while testig the ipa_server_mode. While
> the first three are needed fixes the fourth patch is an improvement
> which might help to avoid an additional request to the LDAP server.
>
> bye,
> Sumit
> From 416e41b55514b34519fb3b8df91cf26e3e4fca73 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Tue, 6 Aug 2013 11:10:42 +0200
> Subject: [PATCH 1/4] IPA_SERVER_MODE: do not follow AD referrals
>
> As in the plain AD provider we do not want to follow referrals send by
> AD in the ipa_server_mode.
>
> src/providers/ipa/ipa_subdomains.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
> index 9ded995..abec644 100644
> --- a/src/providers/ipa/ipa_subdomains.c
> +++ b/src/providers/ipa/ipa_subdomains.c
> @@ -127,6 +127,13 @@ ipa_ad_ctx_new(struct be_ctx *be_ctx,
>          return ret;
>      }
> ret = dp_opt_set_bool(ad_options->id->basic, SDAP_REFERRALS, false);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, ("Cannot set ldap_referrals to false.\n"));
>    talloc_free(ad_options);
>    return ret;
> }
> gc_service_name = talloc_asprintf(ad_options, "%s%s", "gc_", subdom->name);
>    if (gc_service_name == NULL) {
>        talloc_free(ad_options);
>
> -- 
> 1.7.7.6

The code looks good to me but I'm suprised this patch was needed, did you
see referrals being followed?

The ad_id context's sdap_options is copied from
ad_def_ldap_opts, where referrals are set to false. See
src/providers/ad/ad_common.c:47 for example.

Actually, I saw referrals being followed when developing the server mode
initially and came up with 76602b9fbe1b292c5446a44604f49f3122666f9c when
I found out the bool default was not copied properly.
> From: Sumit Bose 
> Date: Tue, 6 Aug 2013 12:17:39 +0200
> Subject: [PATCH 2/4] sdap_add_incomplete_groups: use fully qualified name if
>  needed

ACK
> From: Sumit Bose 
> Date: Wed, 7 Aug 2013 12:12:48 +0200
> Subject: [PATCH 3/4] save_rfc2307bis_user_memberships: use fq names for
>  subdomains

ACK
> From: Sumit Bose 
> Date: Thu, 8 Aug 2013 12:04:11 +0200
> Subject: [PATCH 4/4] sysdb_add_incomplete_group: store SID string is
>  available

ACK

[Sumit Bose]

On Mon, Aug 19, 2013 at 04:07:07PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 15, 2013 at 01:07:47PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > I wrote the following patches while testig the ipa_server_mode. While
> > the first three are needed fixes the fourth patch is an improvement
> > which might help to avoid an additional request to the LDAP server.
> >
> > bye,
> > Sumit
> > From 416e41b55514b34519fb3b8df91cf26e3e4fca73 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Tue, 6 Aug 2013 11:10:42 +0200
> > Subject: [PATCH 1/4] IPA_SERVER_MODE: do not follow AD referrals
> >
> > As in the plain AD provider we do not want to follow referrals send by
> > AD in the ipa_server_mode.
> >
> > src/providers/ipa/ipa_subdomains.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
> > index 9ded995..abec644 100644
> > --- a/src/providers/ipa/ipa_subdomains.c
> > +++ b/src/providers/ipa/ipa_subdomains.c
> > @@ -127,6 +127,13 @@ ipa_ad_ctx_new(struct be_ctx *be_ctx,
> >          return ret;
> >      }
> > ret = dp_opt_set_bool(ad_options->id->basic, SDAP_REFERRALS, false);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_OP_FAILURE, ("Cannot set ldap_referrals to false.\n"));
> >    talloc_free(ad_options);
> >    return ret;
> > }
> > gc_service_name = talloc_asprintf(ad_options, "%s%s", "gc_", subdom->name);
> >    if (gc_service_name == NULL) {
> >        talloc_free(ad_options);
> >
> > -- 
> > 1.7.7.6
>
> The code looks good to me but I'm suprised this patch was needed, did you
> see referrals being followed?
>
> The ad_id context's sdap_options is copied from
> ad_def_ldap_opts, where referrals are set to false. See
> src/providers/ad/ad_common.c:47 for example.
>
> Actually, I saw referrals being followed when developing the server mode
> initially and came up with 76602b9fbe1b292c5446a44604f49f3122666f9c when
> I found out the bool default was not copied properly.

hmm, I think you are right. I re-tested without the patch and do not see
any referrals followed. Maybe I had a broken configuration in my
original testing. Please drop this patch.

Thank you for the review.

bye,
Sumit

[Jakub Hrozek]

On Mon, Aug 19, 2013 at 09:40:01PM +0200, Sumit Bose wrote:
> hmm, I think you are right. I re-tested without the patch and do not see
> any referrals followed. Maybe I had a broken configuration in my
> original testing. Please drop this patch.
>
> Thank you for the review.
>
> bye,
> Sumit

OK, pushed patches 2-4 to master.