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.

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

Keep s2s options: 'outgoing', 'dns', 'address' in maps #3516

Merged
merged 5 commits into from
Jan 27, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jan 25, 2022

These sections used to be converted from maps to top-level options. This PR gets rid of this conversion, keeping them in maps. The address option is now a map of maps, to be precise.

Other changes:

  • A split in the config spec items was necessary to introduce defaults for global options.
  • Docs are fixed as they provided incorrect time units. Although it would be nice to have consistent time units (always seconds or msec), I am not changing the code because this might result in unwanted changes if the option is not updated when the server is upgraded.
  • Refactoring of s2s address lookup to make it more organized.

Test coverage for s2s is poor because the uncovered code involves DNS lookup. I am not sure if it makes sense to cover it in this PR as it is quite tricky.

The next planned change would be to store the entire s2s section in a map, but it contains both global and host-specific options, so it is not possible without additional workarounds or making all options host-specific. I will try to evaluate what the best option is.

@mongoose-im

This comment has been minimized.

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #3516 (8369f6f) into master (40c7551) will increase coverage by 0.07%.
The diff coverage is 77.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3516      +/-   ##
==========================================
+ Coverage   80.99%   81.07%   +0.07%     
==========================================
  Files         419      419              
  Lines       32310    32293      -17     
==========================================
+ Hits        26169    26180      +11     
+ Misses       6141     6113      -28     
Impacted Files Coverage Δ
src/ejabberd_s2s_out.erl 60.58% <75.00%> (+1.47%) ⬆️
src/config/mongoose_config_spec.erl 99.19% <100.00%> (ø)
src/mod_private.erl 79.66% <0.00%> (-3.39%) ⬇️
src/pubsub/mod_pubsub_db_mnesia.erl 92.43% <0.00%> (-0.43%) ⬇️
src/mod_muc_log.erl 78.37% <0.00%> (+0.25%) ⬆️
src/mod_muc_room.erl 77.26% <0.00%> (+0.28%) ⬆️
src/logger/mongoose_log_filter.erl 79.45% <0.00%> (+1.36%) ⬆️
src/mam/mod_mam_elasticsearch_arch.erl 86.84% <0.00%> (+1.75%) ⬆️
src/rdbms/mongoose_rdbms.erl 64.04% <0.00%> (+1.87%) ⬆️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 73.58% <0.00%> (+1.88%) ⬆️
... and 2 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 40c7551...8369f6f. Read the comment docs.

@chrzaszcz chrzaszcz changed the title S2S config in a map Keep s2s options: 'outgoing' and 'dns' in maps Jan 25, 2022
@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@chrzaszcz chrzaszcz marked this pull request as ready for review January 25, 2022 15:08
@chrzaszcz chrzaszcz marked this pull request as draft January 26, 2022 07:18
@chrzaszcz chrzaszcz changed the title Keep s2s options: 'outgoing' and 'dns' in maps Keep s2s options: 'outgoing', 'dns', 'address' in maps Jan 26, 2022
@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

The spec needs to be split, because options with 'wrap = {host_config, *}'
would be invalid in host_config, but they contain defaults,
and would need to be always included.

IP protocol versions are kept as a list of integers for simplicity.
- No double conversion of IP families anymore.
- Options are expected to be always present.
Expect the new defaults for global options o be always present
Store addresses in maps, do a fold over all 3 lookup methods
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 26, 2022

small_tests_24 / small_tests / 8369f6f
Reports root / small


small_tests_23 / small_tests / 8369f6f
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 2691 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / 8369f6f
Reports root/ big
OK: 2674 / Failed: 0 / User-skipped: 255 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 8369f6f
Reports root/ big
OK: 2691 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 2691 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 8369f6f
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 8369f6f
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 8369f6f
Reports root/ big
OK: 1541 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 3078 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 8369f6f
Reports root/ big
OK: 3073 / Failed: 0 / User-skipped: 252 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 3078 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 8369f6f
Reports root/ big
OK: 3078 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 8369f6f
Reports root/ big
OK: 1680 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0


small_tests_24 / small_tests / 8369f6f
Reports root / small


small_tests_23 / small_tests / 8369f6f
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 2691 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 2691 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / 8369f6f
Reports root/ big
OK: 2674 / Failed: 0 / User-skipped: 255 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 8369f6f
Reports root/ big
OK: 1501 / Failed: 1 / User-skipped: 393 / Auto-skipped: 0

rest_client_SUITE:messages:msg_is_sent_and_delivered_over_sse
{error,{{badmap,{error,timeout}},
    [{erlang,map_get,
         [data,{error,timeout}],
         [{error_info,#{module => erl_erts_errors}}]},
     {rest_client_SUITE,msg_is_sent_and_delivered_over_sse,1,
              [{file,"/home/circleci/project/big_tests/tests/rest_client_SUITE.erl"},
               {line,217}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1292}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1224}]}]}}

Report log


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 8369f6f
Reports root/ big
OK: 2691 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 8369f6f
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 8369f6f
Reports root/ big
OK: 1541 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 3078 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 8369f6f
Reports root/ big
OK: 3078 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 8369f6f
Reports root/ big
OK: 1834 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 8369f6f
Reports root/ big
OK: 3073 / Failed: 0 / User-skipped: 252 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 8369f6f
Reports root/ big
OK: 3078 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 8369f6f
Reports root/ big
OK: 1680 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review January 26, 2022 17:27
Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

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

looks ok



-spec open_socket2(Type :: 'inet' | 'inet6',
-spec open_socket2(Type :: inet | inet6,
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: without open_socket1 there is only open_socket2 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I just didn't think there was any rule here, because fun, fun2, fun3... looks to me just as good as fun, fun1, fun2. That's why I decided to minimize the change set.

@gustawlippa gustawlippa merged commit a3b5ce9 into master Jan 27, 2022
@gustawlippa gustawlippa deleted the s2s_config_map branch January 27, 2022 15:07
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants