Skip to content
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

Extract common LDAP options and update mod_shared_roster_ldap config #3558

Merged
merged 11 commits into from
Mar 4, 2022
Merged

Extract common LDAP options and update mod_shared_roster_ldap config #3558

merged 11 commits into from
Mar 4, 2022

Conversation

gustawlippa
Copy link
Contributor

@gustawlippa gustawlippa commented Feb 22, 2022 •
edited

  • Config options in a map for mod_shared_roster_ldap with defaults.
  • Extracted a subsection of common LDAP options that is shared between different modules. This changes the module configuration in some places: ldap_X option would now be ldap.X.
  • mod_vcard_ldap stores the state in a persistent_term, instead of recreating it with every request.
  • updated and simplified docs a bit.
  • removed config getters from ldap_helper

@mongoose-im

This comment was marked as outdated.

Sign in to view
@codecov
Copy link

codecov bot commented Feb 22, 2022 •
edited

Codecov Report

Merging #3558 (c227259) into master (35dd227) will increase coverage by 0.04%.
The diff coverage is 90.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3558      +/-   ##
==========================================
+ Coverage   81.01%   81.06%   +0.04%     
==========================================
  Files         424      424              
  Lines       32337    32299      -38     
==========================================
- Hits        26199    26184      -15     
+ Misses       6138     6115      -23
Impacted Files Coverage Δ
src/config/mongoose_config_spec.erl 99.21% <ø> (ø)
src/eldap_utils.erl 50.45% <ø> (-5.90%) ⬇️
src/wpool/mongoose_wpool_ldap.erl 100.00% <ø> (ø)
src/mongoose_ldap_worker.erl 44.89% <66.66%> (-3.44%) ⬇️
src/mod_shared_roster_ldap.erl 62.68% <78.26%> (+0.18%) ⬆️
src/auth/ejabberd_auth_ldap.erl 70.42% <100.00%> (ø)
src/mongoose_ldap_config.erl 100.00% <100.00%> (ø)
src/vcard/mod_vcard.erl 81.48% <100.00%> (+0.42%) ⬆️
src/vcard/mod_vcard_ldap.erl 92.66% <100.00%> (-0.72%) ⬇️
src/mam/mod_mam_muc_rdbms_arch_async.erl 82.85% <0.00%> (-2.86%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35dd227...c227259. Read the comment docs.

@gustawlippa gustawlippa changed the title [WIP] mod_shared_roster config options in maps [WIP] mod_shared_roster_ldap config options in maps Feb 23, 2022
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch 2 times, most recently from 109712b to c706a4f Compare February 23, 2022 14:21
@esl esl deleted a comment from mongoose-im Feb 23, 2022
@esl esl deleted a comment from mongoose-im Feb 23, 2022
@mongoose-im

This comment was marked as outdated.

Sign in to view
@mongoose-im

This comment was marked as outdated.

Sign in to view
@mongoose-im mongoose-im mentioned this pull request Feb 23, 2022
Closed
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from c706a4f to d8449dc Compare February 24, 2022 15:13
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from d8449dc to b3be00c Compare February 25, 2022 16:08
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from b3be00c to 717530f Compare February 25, 2022 16:27
@mongoose-im

This comment was marked as outdated.

Sign in to view
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 74663d1 to eeb92ff Compare February 28, 2022 12:52
@mongoose-im

This comment was marked as outdated.

Sign in to view
@mongoose-im

This comment was marked as outdated.

Sign in to view
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 9378d85 to e2c100b Compare March 1, 2022 14:21
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from e2c100b to 6dec854 Compare March 1, 2022 15:15
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 6dec854 to 9c65a76 Compare March 2, 2022 08:18
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 9c65a76 to 350454a Compare March 2, 2022 09:46
@mongoose-im

This comment was marked as outdated.

Sign in to view
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 350454a to c3085e3 Compare March 2, 2022 15:48
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 0c2b3ea to 72bec62 Compare March 3, 2022 09:19
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 3, 2022 •
edited

small_tests_24 / small_tests / 72bec62
Reports root / small

small_tests_23 / small_tests / 72bec62
Reports root / small

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 72bec62
Reports root/ big
OK: 2678 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 72bec62
Reports root/ big
OK: 2678 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mysql_redis_24 / mysql_redis / 72bec62
Reports root/ big
OK: 2661 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 72bec62
Reports root/ big
OK: 2678 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

internal_mnesia_24 / internal_mnesia / 72bec62
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 357 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / 72bec62
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 398 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 72bec62
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 398 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 72bec62
Reports root/ big
OK: 3052 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / 72bec62
Reports root/ big
OK: 3047 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / 72bec62
Reports root/ big
OK: 3052 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / 72bec62
Reports root/ big
OK: 3052 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 72bec62
Reports root/ big
OK: 1846 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 72bec62
Reports root/ big
OK: 1692 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

@gustawlippa gustawlippa marked this pull request as ready for review March 3, 2022 10:19
chrzaszcz
chrzaszcz reviewed Mar 3, 2022
View reviewed changes
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

It looks good in general, I added a few comments.

big_tests/tests/vcard_simple_SUITE.erl Outdated Show resolved Hide resolved
src/config/mongoose_config_spec.erl Outdated Show resolved Hide resolved
test/config_parser_SUITE.erl Outdated Show resolved Hide resolved
src/mod_shared_roster_ldap.erl Outdated Show resolved Hide resolved
src/mod_shared_roster_ldap.erl Outdated Show resolved Hide resolved
src/mod_shared_roster_ldap.erl Outdated Show resolved Hide resolved
src/mod_shared_roster_ldap.erl Outdated Show resolved Hide resolved
src/mod_shared_roster_ldap.erl Show resolved Hide resolved
src/mod_shared_roster_ldap.erl Outdated Show resolved Hide resolved
src/mongoose_ldap_config.erl Outdated Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 4, 2022 •
edited

small_tests_23 / small_tests / 260d49e
Reports root / small

small_tests_24 / small_tests / 260d49e
Reports root / small

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 260d49e
Reports root/ big
OK: 2678 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mysql_redis_24 / mysql_redis / 260d49e
Reports root/ big
OK: 2661 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 260d49e
Reports root/ big
OK: 2678 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 260d49e
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 398 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / 260d49e
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 398 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 260d49e
Reports root/ big
OK: 2678 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

internal_mnesia_24 / internal_mnesia / 260d49e
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 357 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / 260d49e
Reports root/ big
OK: 3052 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / 260d49e
Reports root/ big
OK: 3052 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 260d49e
Reports root/ big
OK: 3052 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / 260d49e
Reports root/ big
OK: 3047 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 260d49e
Reports root/ big
OK: 1846 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 260d49e
Reports root/ big
OK: 1692 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 77689c4 to 6488a21 Compare March 4, 2022 11:06
@mongoose-im

This comment was marked as outdated.

Sign in to view
chrzaszcz
chrzaszcz approved these changes Mar 4, 2022
View reviewed changes
Copy link
Member

@chrzaszcz chrzaszcz 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!

big_tests/tests/vcard_simple_SUITE.erl Outdated Show resolved Hide resolved
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from b7079a5 to aea4835 Compare March 4, 2022 13:49
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from aea4835 to 1868792 Compare March 4, 2022 13:52
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 4, 2022 •
edited

small_tests_24 / small_tests / aea4835
Reports root / small

small_tests_23 / small_tests / aea4835
Reports root / small

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / aea4835
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mysql_redis_24 / mysql_redis / aea4835
Reports root/ big
OK: 2756 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / aea4835
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / aea4835
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / aea4835
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / aea4835
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0

internal_mnesia_24 / internal_mnesia / aea4835
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / aea4835
Reports root/ big
OK: 3142 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / aea4835
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / aea4835
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / aea4835
Reports root/ big
OK: 1846 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / aea4835
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / aea4835
Reports root/ big
OK: 1692 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 4, 2022 •
edited

small_tests_24 / small_tests / 1868792
Reports root / small

small_tests_23 / small_tests / 1868792
Reports root / small

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 1868792
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mysql_redis_24 / mysql_redis / 1868792
Reports root/ big
OK: 2756 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 1868792
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 1868792
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 1868792
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / 1868792
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0

internal_mnesia_24 / internal_mnesia / 1868792
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / 1868792
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 1868792
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / 1868792
Reports root/ big
OK: 3142 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 1868792
Reports root/ big
OK: 1846 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / 1868792
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 1868792
Reports root/ big
OK: 1692 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

gustawlippa and others added 5 commits March 4, 2022 15:38
@gustawlippa
Extract common LDAP options in mod_vcard to a subsection
202ff0a
@gustawlippa
Add defaults to shared_roster_ldap config
961fdb3
@gustawlippa
Use a map for config in mongoose_ldap_worker and remove eldap_utils:g…
0ea0025 
…et_mod_opt/4

This was the last place that was using it. It's better to use the new config
with defaults, or maps directly.
@gustawlippa
Correct docs
5b47a48
@gustawlippa @chrzaszcz
Improve code style
c227259 
Co-authored-by: Paweł Chrząszcz <pawel.chrzaszcz@erlang-solutions.com>
@gustawlippa gustawlippa force-pushed the mod_shared_roster_ldap-map-config branch from 1868792 to c227259 Compare March 4, 2022 14:38
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 4, 2022 •
edited

small_tests_24 / small_tests / c227259
Reports root / small

small_tests_23 / small_tests / c227259
Reports root / small

dynamic_domains_mysql_redis_24 / mysql_redis / c227259
Reports root/ big
OK: 2756 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c227259
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / c227259
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / c227259
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / c227259
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / c227259
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0

internal_mnesia_24 / internal_mnesia / c227259
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / c227259
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / c227259
Reports root/ big
OK: 3142 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / c227259
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / c227259
Reports root/ big
OK: 1692 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / c227259
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / c227259
Reports root/ big
OK: 1846 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0

chrzaszcz
chrzaszcz approved these changes Mar 4, 2022
View reviewed changes
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

It's looking good, LDAP logic is going in the right direction 🙂

@chrzaszcz chrzaszcz merged commit 2de7bee into master Mar 4, 2022
@chrzaszcz chrzaszcz deleted the mod_shared_roster_ldap-map-config branch March 4, 2022 15:45
@Premwoik Premwoik added this to the 5.1.0 milestone May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrzaszcz chrzaszcz chrzaszcz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gustawlippa @mongoose-im @chrzaszcz @Premwoik