Refactor sdap_cli_connect#8282
Conversation
|
The underlying code will be reused in the new failover mechanism, but I expect the the new failover code will likely be part of the master branch before the code starts using it and the old failover is removed, so I'd like to get this reviewed and merged. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the LDAP connection logic by separating server resolution and kinit from the core connection function sdap_cli_connect. The server resolution and kinit logic is moved into a new set of functions, sdap_cli_resolve_and_connect_*. This change is a step towards a new failover mechanism and also changes the authentication flow to perform kinit before connecting to the LDAP server. The changes are mostly in src/providers/ldap/sdap_async_connection.c and affect several call sites. The refactoring is well-structured, but I found one issue where a configuration option check was missed during the refactoring.
pbrezina
force-pushed
the
conn-refactor
branch
from
7e7fcb4 to
c95978d
Compare
pbrezina
force-pushed
the
conn-refactor
branch
from
c95978d to
ddd2742
Compare
pbrezina
force-pushed
the
conn-refactor
branch
from
ddd2742 to
ed704a0
Compare
|
Note: Covscan is green. |
|
I did a first round and have no other comments so far (besides two questions inline), but I must admit it's easy to get lost in |
pbrezina
force-pushed
the
conn-refactor
branch
from
ed704a0 to
88af601
Compare
|
Thank you. Those lines were just a left over and can be safely removed.
Yes, the logic is wild and the code would welcome proper refactoring. But I want to avoid doing that along upcoming failover changes to avoid issues in this area. |
|
Thank you, ACK. |
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the patch, code wise it looks good and I'm using the patch since about a week on my laptop without issues, ACK.
bye,
Sumit
To indicate server communication error. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
This is a steps towards new implementation of new failover mechanism. The new code will reuse sdap_cli_connect to connect to the LDAP server but it will not use any be resolver stuff. This patch moves be resolver usage one level up so the connection code can be easily reused. It also moves kinit before connecting to LDAP into a separate, standalone step (previously it was connect -> kinit -> sasl bind, now it is kinit -> connect -> sasl bind). Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
sssd-bot
force-pushed
the
conn-refactor
branch
from
88af601 to
02b2bd4
Compare
4 participants
sdap: remove be context from sdap_cli_connect code
This is a steps towards new implementation of new failover mechanism.
The new code will reuse sdap_cli_connect to connect to the LDAP server
but it will not use any be resolver stuff. This patch moves be resolver
usage one level up so the connection code can be easily reused.
It also moves kinit before connecting to LDAP into a separate,
standalone step (previously it was connect -> kinit -> sasl bind,
now it is kinit -> connect -> sasl bind).