[SSSD] LDAP connection tracking, sharing and fail-over retry framework

Stephen Gallagher sgallagh at redhat.com
Wed Jul 7 12:02:40 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/07/2010 04:06 AM, Eugene Indenbom wrote:
> On 07/06/2010 11:17 PM, Stephen Gallagher wrote:
>> However, since dp_error is assigned in the very next line (in a way that
>> can't fail) I don't see a good reason to initialize it here.
>>
>>    
> It's a bit complicated question and generally matter of coding style. 
> What can we assume about output parameters of functions defined in the 
> other source file?
> 
> My opinion is that we should always initialize output parameters in the 
> calling method, just to be on the safe side. What is your opinion?

I follow an admittedly loose rule about this. I'll usually skip the
initialization when dealing with static functions that I control.
However, with public functions or third-party functions I'll always
initialize.

I agree that it's always safe to pre-initialize, so if you want to be
overly-careful, I won't disagree :)

>> Using hbac_ctx->sdap_op == NULL to mean "operating in offline mode" is
>> very hard to read. Please just set a boolean state like
>> hbac_ctx->offline instead. When I read "if (!hbac_ctx->sdap_op)", my
>> natural expectation is for the following lines to set up the sdap_op.
...
> The problem here is that we get 2 interrelated fields in hbac_ctx and 
> will have to add asserts testing in online mode that hbac_ctx->sdap_op 
> is not NULL.
> The same applies to the prototypes of HBAC stages:
>      hbac_get_host_info_send()
>      hbac_get_rules_send()
>      hbac_get_service_data_send()
> 
> We will have to pass both hbac_ctx->offline and hbac_ctx->sdap_op as 
> parameters with the same problem as above.
> 
>  From my point of view, the solution could be to pass the above methods 
> hbac_ctx itself and define hbac_ctx_is_offline() test. This way the code 
> will be readable and no duplicate info will be kept in data structures.

Yes, I think hbac_ctx_is_offline() would be a better idea. That way it
reads clearly.

> 
> 
>>    
>>>      0011-Use-new-LDAP-connection-framework-in-IPA-dynamic-DNS.patch
>>>      
>> Nack.
>>
>> There are several problems with this patch:
>>
>> 1) When the dynamic DNS update code was written, it expected that when
>> we are online, we would always have an active LDAP connection. This is
>> no longer guaranteed to be true. As we have discussed previously, it
>> will be possible with this new interface (eventually) to be able to
>> configure it so that connections to LDAP are not maintained after the
>> currently-running set of operations completes. So it's possible that it
>> will not be available to sdap_id_get_cached_connection() here. This
>> function needs to be able to create a connection when necessary (so
>> we'll need to convert part of the dynamic DNS update code to expect an
>> async operation at this point.
>>    
> Well, actually I have modelled the implementation after current approach 
> that can currently even segfault if connection gets dropped during 30ms 
> delay. Another reason is that ldap/ipa backend ONLINE callbacks are only 
> called when LDAP connection is established, so it does make sense to 
> assume that connection is established.

Yeah, I knew when I wrote it that this was a possible risk. I was hoping
that we could eliminate it with your new connection wrapper.

> 
> Please, let me know your opinion and I will add async establishment of 
> LDAP connection if needed.

I think this would be the better way to go. It will avoid the potential
race condition.

>> 2) sdap_id_op_connect_cached() is never used anywhere. As mentioned
>> above, we should ask the connection cache if there is an available
>> connection and using it if so, creating a new connection if not.
>>
>>    
> I have added this method for the sake of completeness. It can be safely 
> removed. Currently I do not see any use for it.

Yeah, if it's not used, let's drop it. Dead code is a distraction.



I had a discussion about these patches with Simo yesterday, and he
raised an interesting question. His thought was that perhaps we are
wrapping the LDAP code at too high of a level. His thoughts were that,
instead of creating an sdap_op at the top level of the LDAP request, we
could instead wrap individual calls to sdap_generic_send(), so that if
we reconnect in the middle of a multi-step operation, we do not have to
restart and rerun all of the earlier steps.

This is an interesting idea, but it also occurred to me that there is a
potential risk as well: if we fail over to another LDAP server, there is
no guarantee that it is perfectly in sync with the primary server in the
failover chain. The result could be that we would get some data from one
server or the other that doesn't align.

With this in mind, I think our current approach is a bit safer (even if
it might be slower in a failover situation). With this approach, we at
least guarantee consistency within a single LDAP server.

What are your thoughts, Eugene?


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkw0bOAACgkQeiVVYja6o6MdXQCdGQMOOhLZSrQWwBgqcOFbr6Kn
rZAAoLEFa/4lKfjVUN1aPd/ATNlna60z
=pEOb
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list