Skip to content

Use a dedicated LDAP host and port for background jobs if configured #35229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
blizzz merged 3 commits into from
Dec 20, 2022
Merged

Use a dedicated LDAP host and port for background jobs if configured #35229

blizzz merged 3 commits into from
Dec 20, 2022

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Nov 17, 2022

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

@come-nc come-nc added the 3. to review Waiting for reviews label Nov 17, 2022
@come-nc come-nc added this to the Nextcloud 26 milestone Nov 17, 2022
@come-nc come-nc requested review from a team, PVince81 and blizzz November 17, 2022 13:57
@come-nc come-nc self-assigned this Nov 17, 2022
@come-nc come-nc requested review from ArtificialOwl and removed request for a team November 17, 2022 13:57
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 17, 2022
View reviewed changes
apps/user_ldap/lib/Connection.php
$ldapLogin = @$this->ldap->bind($cr,
$this->configuration->ldapAgentName,
$this->configuration->ldapAgentPassword);
$this->configuration->ldapAgentName,

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 2 of OCA\User_LDAP\ILDAPWrapper::bind cannot be null, possibly null value provided
apps/user_ldap/lib/Connection.php
$this->configuration->ldapAgentName,
$this->configuration->ldapAgentPassword);
$this->configuration->ldapAgentName,
$this->configuration->ldapAgentPassword);

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 3 of OCA\User_LDAP\ILDAPWrapper::bind cannot be null, possibly null value provided
artonge
artonge requested changes Nov 30, 2022
View reviewed changes
@come-nc
Copy link
Contributor Author

come-nc commented Dec 1, 2022 •
edited
Loading

I pushed a commit to make code clearer, but it makes it clear that there is a logic flaw:

When in CLI, background host is used and backuphost is not used even if backgroundhost do not answer. But, if backuphost is used by web UI because a fallback, then it will be used in CLI as well because of the cache.

So we need to decide to either:

  • Fallback to backuphost even for CLI if backgroundhost do not anwser
  • Never use backuphost in CLI event if backgroundhost is not answering, we use no fallback

(These is considering there is a backgroundhost configured, if not we always fallback if needed to keep the same behavior as before)

@come-nc
Copy link
Contributor Author

come-nc commented Dec 1, 2022

(There is also the question what to do when backup host is forced from admin settings, should that apply to cli if there is a background host?)

@blizzz
Copy link
Member

blizzz commented Dec 1, 2022

Fallback to backuphost even for CLI if backgroundhost do not anwser

I would say so (provided that backuphost differs from background host)

@come-nc
Copy link
Contributor Author

come-nc commented Dec 1, 2022

Fallback to backuphost even for CLI if backgroundhost do not anwser

I would say so (provided that backuphost differs from background host)

Ok, makes sense to me to always fallback to the backup if first host is not answering.
It also means forcing backup applies everywhere, which is more intuitive.

The problem with that is that we’ll need to split cache into 2 vars, to not go to backup for CLI when main is down but backgroundHost is still working.
I’ll implement that.

@come-nc come-nc added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 1, 2022
@come-nc
Copy link
Contributor Author

come-nc commented Dec 1, 2022

mainHost reachable backupHost set backgroundHost set backgroundHost reachable backupForced CLI Host contacted
x main
x x main
x x main
x x x main
x x x main
x x x x background
x x x error
x x x x main
x x x x x background
x x x x backup
error
x error
x backup
x x backup
x x error
x x x background
x x error
x x x backup
x x x x background
x x x backup
any x any any x any backup

@come-nc
Copy link
Contributor Author

come-nc commented Dec 19, 2022

/rebase

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 19, 2022
View reviewed changes
apps/user_ldap/lib/Connection.php
$this->bindResult = [];
$bindStatus = $this->bind();
$error = $this->ldap->isResource($this->ldapConnectionRes) ?
$this->ldap->errno($this->ldapConnectionRes) : -1;

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 1 of OCA\User_LDAP\ILDAPWrapper::errno cannot be null, possibly null value provided
@come-nc
Use a dedicated LDAP host and port for background jobs if configured
4758bdc 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Make code clearer for background host
4067505 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fixed backup host logic
6b7ffcd 
Now forcing backup host applies to both main and background.
And background will fallback to backup if not responding.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the feat/user_ldap-specific-server-for-background branch from ce86ffe to 6b7ffcd Compare December 19, 2022 13:16
@come-nc
Copy link
Contributor Author

come-nc commented Dec 19, 2022

rebased

@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 20, 2022
blizzz
blizzz approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@blizzz blizzz merged commit 4d04030 into master Dec 20, 2022
@blizzz blizzz deleted the feat/user_ldap-specific-server-for-background branch December 20, 2022 10:47
@come-nc come-nc mentioned this pull request Dec 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blizzz blizzz blizzz approved these changes

@artonge artonge artonge approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

4 participants

@come-nc @blizzz @artonge