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
To indicate server communication error.
pbrezina
force-pushed
the
conn-refactor
branch
from
c95978d to
ddd2742
Compare
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).
pbrezina
force-pushed
the
conn-refactor
branch
from
ddd2742 to
ed704a0
Compare
|
Note: Covscan is green. |
| state->srv = NULL; | ||
| state->srv_opts = NULL; | ||
| state->use_rootdse = !skip_rootdse; | ||
| state->rootdse_access = decide_rootdse_access (opts->basic); |
There was a problem hiding this comment.
What is the use of rootdse_access in sdap_cli_resolve_and_connect_state?
Btw, copy-pasted with a space before "(" that was removed in sdap_cli_connect_send().
| } | ||
|
|
||
| ret = decide_tls_usage(state->force_tls, state->opts->basic, | ||
| state->service->uri, &state->use_tls); |
There was a problem hiding this comment.
Is it really needed here? It will be done (again) in sdap_cli_connect_step() later.
|
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 |
3 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).