On Fri, Oct 25, 2013 at 03:31:58PM +0200, Sumit Bose wrote:
On Fri, Oct 25, 2013 at 02:46:31PM +0200, Lukas Slebodnik wrote:
> On (25/10/13 11:54), Sumit Bose wrote:
> >On Thu, Oct 24, 2013 at 11:54:30AM +0200, Lukas Slebodnik wrote:
> >> ehlo,
> >>
> >> Global catalog port was ignored from SRV record, hard coded value was used
> >> every time.
> >> 1st patch should fix it.
> >
> >good catch, but see my comment below.
> >
> >>
> >> 2nd patch add fall back to LDAP if GC is not available.
> >> with this patch SSSD will not go offline if GC is not available,
> >> but it may happen that SSSD will try to connect to GC often.
> >> I am not sure, but we can add new option do disable GC lookup completely.
> >
> >The patch is working as expected, so ACK. I still can see some debug
> >messages saying that SSD will go offline before the fallback to LDAP
> >happens, but I think this is aceptable.
> >
> I forgot to move this debug message inside the if statement.
> and debug message has high debug level SSSDBG_CRIT_FAILURE.
> It make sense.
>
> >I don't think that we need an option to disable the GC lookup
> >completely. Since we are doing SRV lookups to find the GC server there
> >should be always one wokring GC. If not, there is either a firewall
> >issue or the AD domain has other problems.
> >
> Thank you for review.
>
> >bye,
> >Sumit
> >
> >>
> >> LS
> >
> >> From abf7cfeaa9826fb52e9a18d3bd5812003c38f118 Mon Sep 17 00:00:00 2001
> >> From: Lukas Slebodnik <lslebodn(a)redhat.com>
> >> Date: Thu, 24 Oct 2013 11:03:32 +0200
> >> Subject: [PATCH 1/3] AD: Prefer GC port from SRV record
> >>
> >> We had a hard coded value of Global Catalog port (3268).
> >> Informations from SRV record was ignored.
> >> This patch prefer port number from SRV record and hard coded value is used
only
> >> as a fall back if port number was not initialized.
> >> ---
> >> src/providers/ad/ad_common.c | 7 +++++--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> >> index
ab62d64a943a726724698d1b7eb9f15bc40b389e..1c4bd088b30eed4f71a787ce4c3bd60366b72def 100644
> >> --- a/src/providers/ad/ad_common.c
> >> +++ b/src/providers/ad/ad_common.c
> >> @@ -729,12 +729,15 @@ ad_resolve_callback(void *private_data, struct
fo_server *server)
> >> talloc_zfree(service->gc->uri);
> >> talloc_zfree(service->gc->sockaddr);
> >> if (sdata && sdata->gc) {
> >> + int new_port = fo_get_server_port(server);
> >
> >Please move the declaration to the top of the function (see
> >http://www.freeipa.org/page/Coding_Style)
> >
> Fixed.
> (I thought that declaration at the beginning of block is allowed.)
>
> >> + new_port = (new_port == 0) ? AD_GC_PORT : new_port;
> >> +
> >> service->gc->uri = talloc_asprintf(service->gc,
"%s:%d",
> >> - new_uri, AD_GC_PORT);
> >> + new_uri, new_port);
> >>
> >> service->gc->sockaddr =
resolv_get_sockaddr_address(service->gc,
> >> srvaddr,
> >> - AD_GC_PORT);
> >> + new_port);
> >> } else {
> >> /* Make sure there always is an URI even if we know that this
> >> * server doesn't support GC. That way the lookup would go
through
> >> --
> >> 1.8.3.1
> >>
>
> Updated patches are attached.
ACK
bye,
Sumit
>
> LS
Pushed to master and sssd-1-11