[SSSD] Patch to fix LDAP ID backend GSSAPI credential expired messages

Simo Sorce ssorce at redhat.com
Wed Mar 31 19:08:24 UTC 2010


On Mon, 29 Mar 2010 19:41:57 +0400
Eugene Indenbom <eindenbom at gmail.com> wrote:

> Hi Simon,
> 
> I have to admit that the patch is really quite big and, actually, it
> has by far exceeded size and time limits I would normally apply to
> patches to third party components.
> 
> The patch can be theoretically split into 3 parts:
> 1. Changes to ldap_child related to returned ticket expiration date;

This should be a separate patch.

> 2. Changes to failover subsystem needed to return number of servers
> registered for failover;

I am trying to understand what's the reason for this.
Why the retries should depend on server availability ?
I would expect to retry once per server until there are servers
available. Can you explain why you think you should try to calculate
servers availability in advance ?

Just fyi, the problem is that we might not know in advance, in future
we plan to add support for reading SRV records to determine the list of
servers, so the number of available servers may change w/o notice.

> 3. Changes to LDAP ID backend connection and retry logic.
> 
> As you can see, the first two items are really small and absolutely
> pointless without the last.
> 
> The reason why the changes to LDAP ID backend connection and retry
> logic must go together are very simple: old logic relies on gsh member
> of sdap_id_ctx, while in new logic there is no such a member.
> 
> The reason why gsh needs to go away is as follows:
> 1. gsh enforces that there will be one and only one connection to DS;

Yes, this is completely intentional. Hence the questions.

> 2. When connection is about to expire we can not use it for new
> request as it will expire halfway;

ok

> 3. But at the same time connection could be yet busy with previous
> request;

I am not sure why this would be relevant though.

> 4. Therefore we have to make a new connection and close old
> one as soon as requests using it are finished.

Yes, but this can be easily done with a destructor attached to the
queue code within the sdap_handle, that's why the ops list is in there.

> > The goal of SSSD is to never use more than one connection at a time
> > for account information. So your patch is kind of changing our
> > fundamental goal by allowing multiple connections. We need to
> > carefully evaluate that part.
> 
> As I have explained above, the only time we have more then one
> connection to DS is when old connection is about to expire and we need
> to open new one. So when ticket lifetime is long enough (as it is in
> normal Kerberos configuration) there will be no more then 2
> connections open.

I still don't see why we should remove gsh from sdap_ctx. gsh is "the
connection you should currently use". We probably need to be able to
tell if a re-connection is happening already and delay new requests
until that is done.

I think this would be something similar to the pooling code we have in
the nss daemon where we check if we are already performing a specific
request and use a queue to wait for a reply.

> > I see you started passing around sdap_id_op. The memory hierarchy
> > around sdap_is_op is very delicate and required a lot of very
> > careful handling to avoid having it disappear under our feet at the
> > wrong time. It is meant to represent a single ldap operation tied
> > to a specific ldap context, any changes to its use should be in a
> > separate patch that I want to review carefully. But ideally
> > sdap_id_op is opaque to most of the code and is internal to the
> > processing of replies from the openldap libraries. It should never
> > be used out of this context.

I have to caution here that I confused sdap_id_op and sdap_op when I
wrote this remark. It looks like sdap_id_op is a queue for requests,
and that goes in the right direction, but it looks a bit overly complex.
Yet I have not fully analyzed the patch because I can't apply it.

> I agree that both sdap_id_op and sdap_id_connection are opaque types.
> You can move the definitions to ldap_common.c from headers. More over
> even declaration of sdap_id_connection can be visible only to
> ldap_common.c.

The point I am trying to make here is that I don't like
sdap_id_op_handle(), as that means you are probably using an old
connection. Newer calls should always use new connections, so what is
the point of trying to fetch and old handler ?

Btw can you avoid comments as /* sdap handle */ when defining a
sdap_handle in a structure? it look pointless to me. Comments are
good but only when they tell you something that is not clear from
the code at hand already :)

Also given the complexity of the patch can you use the --patience
switch to git format-patch and resubmit it, preferably rebased on top
of master, but if it is too much work just tell me on top of which
commit you created it so I can simply rebase my test branch and apply
it w/o issues.
It should make things much more readable as unfortunately it seem the
patch does not apply on top of master now, so reviewing it properly is
a bit hard.

> I were not really sure what coding style is used in project. There are
> files coded quite differently from each other.
> 
> On the other hand I do not see why you find handling of these
> structures delicate:
> 
> 1. sdap_id_op is owned by operation state (e.g. by global_enum_state).
> So it will be automatically destroyed as operation (tevent request) is
> completed

Why sdap_id_op_connect() is not a tevent request ?
Passing around callbacks is usually frowned upon, as tevent requests is
the way we want to handle any async event if at all possible.

Unless there is a *very* good reason why it is not a tevent_req then
this is one of the things that needs to change before the patch can be
accepted.

I know that at first it seem it doesn't matter, but trust me, there is
an almost 4 years of attempts in the samba community to get up with the
tevent_req style for a number of subtle and painful reasons. We have
gone through at least 4 different ways to do continuations, and
tevent_req is the one that finally makes thing bearable. The style is
important both formally and substantially for too many reasons to
explain in this mail (you can jump on IRC in the #freeipa channel if you
want to discuss it).

> 2. sdap_id_connection is owned by sdap_id_ctx and logic of its
> life-cycle boils down to single method - sdap_id_release_connection.
> Connection is released when:
>    a) There is no operation using it
>    b) It is not cached
>    c) It is not in connection notify loop (notify_lock == 0)
> 
> I hope I have explained why changes were made the way they have been
> done.

What I don't understand here is what you added sdap_id_op_connection at
all, sdap_handle is meant to represent a connection, why adding yet
another structure here ?

Also I *really* don't like the fact that sdap_id_op_connection has
members like: connect_req, expire_timer

The only way to access a request should always be through the
tevent_req_callback_data() function, because that guarantee the request
is around (it's the parent of the subreq you are in).
If you store it in a structure, you can generally end up trying to
access freed memory during clean up operations.

Storing these pointers usually means the code is wrong, and you may
have been so careful and took it in account, but in general I prefer to
avoid it, because even if you were careful enough, almost certainly the
next person that will touch the code will screw it up.
I am still reading through it, but these are normally signs that the
architecture needs to be heavily adjusted.

> I really do not see a way to split the patch and would appreciate very
> much if you give me some advice on how to make it more readable and
> easier to understand.

Each major architectural change should be in a separate patch even
though it doesn't adding anything useful until the next patch comes
in. The only rule is that the code compiles and works.

> If you have any ideas on how to split the patch, I am ready to discuss
> them and implement if needed.

I'd really like to see an explanation of the re-connection
code, if you can provide a new patch as requested above I will be able
to better evaluate the re-connection logic.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York



More information about the sssd-devel mailing list