[PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
All other ldap-querying methods in idmap_ad make a single retry attempt if they get
TLDAP_SERVER_DOWN. This patch brings idmap_ad_query_user in line with that design.

This fixes the symptom described in 12720 at the cost of an additional reconnect per
failed lookup.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12720

Signed-off-by: Dustin L. Howett <[hidden email]>
---
 source3/winbindd/idmap_ad.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/source3/winbindd/idmap_ad.c b/source3/winbindd/idmap_ad.c
index 5039e9bfe56..2bee08a5577 100644
--- a/source3/winbindd/idmap_ad.c
+++ b/source3/winbindd/idmap_ad.c
@@ -511,9 +511,26 @@ static NTSTATUS idmap_ad_query_user(struct idmap_domain *domain,
  return NT_STATUS_OK;
 }
 
+static NTSTATUS idmap_ad_query_user_retry(struct idmap_domain *domain,
+          struct wbint_userinfo *info)
+{
+ const NTSTATUS status_server_down =
+ NT_STATUS_LDAP(TLDAP_RC_V(TLDAP_SERVER_DOWN));
+ NTSTATUS status;
+
+ status = idmap_ad_query_user(domain, info);
+
+ if (NT_STATUS_EQUAL(status, status_server_down)) {
+ TALLOC_FREE(domain->private_data);
+ status = idmap_ad_query_user(domain, info);
+ }
+
+ return status;
+}
+
 static NTSTATUS idmap_ad_initialize(struct idmap_domain *dom)
 {
- dom->query_user = idmap_ad_query_user;
+ dom->query_user = idmap_ad_query_user_retry;
  dom->private_data = NULL;
  return NT_STATUS_OK;
 }
--
2.13.2


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Fri, Jun 30, 2017 at 04:10:01PM -0700, Dustin L. Howett via samba-technical wrote:
> All other ldap-querying methods in idmap_ad make a single retry attempt if they get
> TLDAP_SERVER_DOWN. This patch brings idmap_ad_query_user in line with that design.
>
> This fixes the symptom described in 12720 at the cost of an additional reconnect per
> failed lookup.

lgtm. Can I get a second reviewer?

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Mon, 2017-07-10 at 13:02 +0200, Ralph Böhme via samba-technical
wrote:
> On Fri, Jun 30, 2017 at 04:10:01PM -0700, Dustin L. Howett via samba-technical wrote:
> > All other ldap-querying methods in idmap_ad make a single retry attempt if they get
> > TLDAP_SERVER_DOWN. This patch brings idmap_ad_query_user in line with that design.
> >
> > This fixes the symptom described in 12720 at the cost of an additional reconnect per
> > failed lookup.
>
> lgtm. Can I get a second reviewer?

Can we get a selftest for idmap_ad, like but not re-using the totally
different idmap_rfc2307 tests, perhaps as simple as running
nsswitch/tests/test_rfc2307_mapping.sh against an appropriate member
(rather than DC) environment?

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Mon, Jul 10, 2017 at 11:07:16PM +1200, Andrew Bartlett wrote:

> On Mon, 2017-07-10 at 13:02 +0200, Ralph Böhme via samba-technical
> wrote:
> > On Fri, Jun 30, 2017 at 04:10:01PM -0700, Dustin L. Howett via samba-technical wrote:
> > > All other ldap-querying methods in idmap_ad make a single retry attempt if they get
> > > TLDAP_SERVER_DOWN. This patch brings idmap_ad_query_user in line with that design.
> > >
> > > This fixes the symptom described in 12720 at the cost of an additional reconnect per
> > > failed lookup.
> >
> > lgtm. Can I get a second reviewer?
>
> Can we get a selftest for idmap_ad, like but not re-using the totally
> different idmap_rfc2307 tests, perhaps as simple as running
> nsswitch/tests/test_rfc2307_mapping.sh against an appropriate member
> (rather than DC) environment?
like this?

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Mon, 2017-07-10 at 16:28 +0200, Ralph Böhme wrote:

> On Mon, Jul 10, 2017 at 11:07:16PM +1200, Andrew Bartlett wrote:
> > On Mon, 2017-07-10 at 13:02 +0200, Ralph Böhme via samba-technical
> > wrote:
> > > On Fri, Jun 30, 2017 at 04:10:01PM -0700, Dustin L. Howett via samba-technical wrote:
> > > > All other ldap-querying methods in idmap_ad make a single retry attempt if they get
> > > > TLDAP_SERVER_DOWN. This patch brings idmap_ad_query_user in line with that design.
> > > >
> > > > This fixes the symptom described in 12720 at the cost of an additional reconnect per
> > > > failed lookup.
> > >
> > > lgtm. Can I get a second reviewer?
> >
> > Can we get a selftest for idmap_ad, like but not re-using the totally
> > different idmap_rfc2307 tests, perhaps as simple as running
> > nsswitch/tests/test_rfc2307_mapping.sh against an appropriate member
> > (rather than DC) environment?
>
> like this?

Yes!  I would prefer we didn't re-use administrator but modified a
different user, but that isn't an objection but rather a suggested
improvement that I'm also trying to fix for the idmap_rfc2307 tests.

The only change I want to see is adding:
password server = $DC_SERVER
to the smb.conf, so we don't race replication if the environment
startup order changes.

With that second proviso:

Reviewed-by: Andrew Bartlett <[hidden email]>
(including for Dustin's patch)

Thanks!
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Mon, Jul 10, 2017 at 1:04 PM, Andrew Bartlett <[hidden email]> wrote:
>
> With that second proviso:
>
> Reviewed-by: Andrew Bartlett <[hidden email]>
> (including for Dustin's patch)
>
> Thanks!

Andrew, Ralph,

Thanks for the review.

I've got a couple notes:

1. It looks like I missed a space.

+static NTSTATUS idmap_ad_query_user_retry(struct idmap_domain *domain,
+          struct wbint_userinfo *info)

(on the struct wbint_userinfo line.)

I can further revise Ralph's patch if you'd rather not fix it inline.

2. While this brings idmap_ad_query_user in line with the other idmap_ad
functions, it doesn't solve the core issue.

It looks like the winbindd hosting idmap eventually operates on a closed ldap
connection. I haven't been able to determine why it's being closed, but it's
on the member server.

The winbind cache covered the rfc2307 NSS info until 4.6. The ldap connection
loss may have happened in 4.5 and prior as well, but the cache covered for it
until the connection was reestablished.

I may fork a thread over to samba@ to discuss this further.

-d

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Mon, 2017-07-10 at 21:13 -0700, Dustin Howett wrote:

> On Mon, Jul 10, 2017 at 1:04 PM, Andrew Bartlett <[hidden email]> wrote:
> >
> > With that second proviso:
> >
> > Reviewed-by: Andrew Bartlett <[hidden email]>
> > (including for Dustin's patch)
> >
> > Thanks!
>
> Andrew, Ralph,
>
> Thanks for the review.
>
> I've got a couple notes:
>
> 1. It looks like I missed a space.
>
> +static NTSTATUS idmap_ad_query_user_retry(struct idmap_domain *domain,
> +          struct wbint_userinfo *info)
>
> (on the struct wbint_userinfo line.)
>
> I can further revise Ralph's patch if you'd rather not fix it inline.
>
> 2. While this brings idmap_ad_query_user in line with the other idmap_ad
> functions, it doesn't solve the core issue.
>
> It looks like the winbindd hosting idmap eventually operates on a closed ldap
> connection. I haven't been able to determine why it's being closed, but it's
> on the member server.
>
> The winbind cache covered the rfc2307 NSS info until 4.6. The ldap connection
> loss may have happened in 4.5 and prior as well, but the cache covered for it
> until the connection was reestablished.
>
> I may fork a thread over to samba@ to discuss this further.

This kind of discussion belongs here, on samba-technical.

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Jul 10, 2017 at 09:13:16PM -0700, Dustin Howett wrote:
> I've got a couple notes:
>
> 1. It looks like I missed a space.
>
> +static NTSTATUS idmap_ad_query_user_retry(struct idmap_domain *domain,
> +          struct wbint_userinfo *info)
>
> (on the struct wbint_userinfo line.)

no prob, shit happens. ;)

> I can further revise Ralph's patch if you'd rather not fix it inline.

I'll fix it when I push it.

> 2. While this brings idmap_ad_query_user in line with the other idmap_ad
> functions, it doesn't solve the core issue.
>
> It looks like the winbindd hosting idmap eventually operates on a closed ldap
> connection. I haven't been able to determine why it's being closed, but it's
> on the member server.
>
> The winbind cache covered the rfc2307 NSS info until 4.6. The ldap connection
> loss may have happened in 4.5 and prior as well, but the cache covered for it
> until the connection was reestablished.

Does this imply your patch is void? I think it is correct, essentially
triggering a reconect of an idle connection that got disconnected by the server
(at least that's my understanding of the retry logic).

> I may fork a thread over to samba@ to discuss this further.

As mentioned by Andrew, this discussion rightly belongs here, but feel free to
create a new thread as appropriate.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] idmap_ad: Retry query_user exactly once if we get TLDAP_SERVER_DOWN

Samba - samba-technical mailing list
On Tue, Jul 11, 2017 at 01:36:21PM -0700, Dustin Howett wrote:
> On Tue, Jul 11, 2017 at 1:02 PM, Ralph Böhme <[hidden email]> wrote:
> > Does this imply your patch is void? I think it is correct, essentially
> > triggering a reconect of an idle connection that got disconnected by the server
> > (at least that's my understanding of the retry logic).
> >
>
> Nah. The patch is correct and solves the transient connection loss case
> properly. What led me to submit a patch, though, is the aforementioned *client-
> side* connectivity issue I've been hunting.

ok, in that case: PUSHED! :)

(with the password server change requested by Andrew).

Cheerio!
-slow

Loading...