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

s2s config per host type #3523

Merged
merged 11 commits into from
Feb 3, 2022
Merged

s2s config per host type #3523

merged 11 commits into from
Feb 3, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jan 31, 2022

The goal is to put all s2s options in a map and allow specifying them globally or per host type. The host_config.s2s section completely overrides the global s2s section now, as this is more straightforward and allows resetting optional values.

Main changes:

  • Handling of incoming connections is reorganized to get host type as early as possible.
  • As a side effect, more errors are now properly caught and reported. Tests for them are added.
  • Coverage of s2s logic is improved.
  • Defaults are used for all options that have defaults in the doc.

Smaller changes

  • Removed s2s.address default as it had no benefit.

@chrzaszcz chrzaszcz changed the title WIP s2s config per host type Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #3523 (db060ab) into master (76166cc) will increase coverage by 0.02%.
The diff coverage is 91.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3523      +/-   ##
==========================================
+ Coverage   81.06%   81.09%   +0.02%     
==========================================
  Files         419      419              
  Lines       32298    32296       -2     
==========================================
+ Hits        26182    26190       +8     
+ Misses       6116     6106      -10     
Impacted Files Coverage Δ
src/ejabberd_s2s.erl 81.11% <81.25%> (-0.35%) ⬇️
src/ejabberd_s2s_out.erl 60.54% <88.46%> (-0.04%) ⬇️
src/ejabberd_s2s_in.erl 78.68% <95.65%> (+2.11%) ⬆️
src/config/mongoose_config_spec.erl 99.21% <100.00%> (-0.01%) ⬇️
src/ejabberd_tls.erl 81.08% <100.00%> (+0.52%) ⬆️
src/cassandra/mongoose_cassandra.erl 77.77% <0.00%> (-3.71%) ⬇️
src/cassandra/mongoose_cassandra_worker.erl 65.72% <0.00%> (-2.82%) ⬇️
src/mam/mod_mam_rdbms_arch_async.erl 96.66% <0.00%> (-1.12%) ⬇️
src/mod_muc_log.erl 78.11% <0.00%> (ø)
... and 8 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 76166cc...db060ab. Read the comment docs.

@chrzaszcz chrzaszcz force-pushed the s2s-config-host-only branch 8 times, most recently from 3a24ffd to 77126ba Compare February 2, 2022 08:35
@chrzaszcz chrzaszcz marked this pull request as ready for review February 2, 2022 09:00
- All options can be specified globally and per host type
- Host-type s2s section completely overrides the global one
- Options have defaults as specified in the docs
- Certfile path is checked now
- Nested maps are always included as they contain defaults
There is now a separate s2s shared secret for each host type
Main changes:
- Server is always checked as host type is needed
- Error handling is more consistent and covers more cases
- Test options per host type
- Ensure that host-type options override global ones
- Put existing files as certificates
Also: parallelize, remove repetition
- All options are allowed per host type
- s2s section overrides the whole global section now
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@esl esl deleted a comment from mongoose-im Feb 2, 2022
@mongoose-im
Copy link
Collaborator

mongoose-im commented Feb 2, 2022

small_tests_23 / small_tests / 6babcd7
Reports root / small


small_tests_24 / small_tests / 6babcd7
Reports root / small


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


ldap_mnesia_23 / ldap_mnesia / 6babcd7
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


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


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


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


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 6babcd7
Reports root/ big
OK: 1840 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 6babcd7
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 6babcd7
Reports root/ big
OK: 1686 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 6babcd7
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 6babcd7
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 6babcd7
Reports root/ big
OK: 3079 / Failed: 0 / User-skipped: 252 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 6babcd7
Reports root/ big
OK: 1547 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0

Copy link
Contributor

@Premwoik Premwoik 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 to me :D

src/ejabberd_s2s_in.erl Outdated Show resolved Hide resolved
src/ejabberd_s2s_in.erl Outdated Show resolved Hide resolved
mnesia:write(#s2s_shared{host_type = HostType, secret = Secret}).

-spec lookup_certfile(mongooseim:host_type()) -> {ok, string()} | {error, not_found}.
lookup_certfile(HostType) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we first look up the global domain_certfile then c2s certfile and not in the reverse order?

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 wouldn't say domain_certfile is global - it is per host type, but it is in the general section.

According to the docs for domain_certfile:

This option overrides the configured certificate file for specific local XMPP domains. This option applies to S2S and C2S connections.

It took precedence over s2s and c2s settings before and I didn't want to change that. The only difference is that you can set s2s.certfile per domain now, but usually you would set it globally, and there is no way to tell how it was set.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Feb 3, 2022

small_tests_24 / small_tests / db060ab
Reports root / small


small_tests_23 / small_tests / db060ab
Reports root / small


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


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


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


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


ldap_mnesia_24 / ldap_mnesia / db060ab
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / db060ab
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / db060ab
Reports root/ big
OK: 1547 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / db060ab
Reports root/ big
OK: 1840 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / db060ab
Reports root/ big
OK: 3079 / Failed: 0 / User-skipped: 252 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / db060ab
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / db060ab
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / db060ab
Reports root/ big
OK: 1686 / Failed: 0 / User-skipped: 363 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / db060ab
Reports root/ big
OK: 3094 / Failed: 2 / User-skipped: 247 / Auto-skipped: 0

pep_SUITE:pep_tests:delayed_receive
{error,{{badmatch,[]},
    [{pep_SUITE,'-delayed_receive/1-fun-0-',3,
          [{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
           {line,276}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {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

pep_SUITE:pep_tests:h_ok_after_notify_test
{error,{{assertion_failed,assert_many,false,[is_roster_set],[],[]},
    [{escalus_new_assert,assert_true,2,
               [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
                {line,84}]},
     {escalus_story,'-make_all_clients_friends/1-fun-0-',2,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,108}]},
     {escalus_utils,'-each_with_index/3-fun-0-',3,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
             {line,87}]},
     {lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
     {escalus_utils,'-each_with_index/3-fun-0-',3,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
             {line,87}]},
     {lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
     {escalus_utils,distinct_pairs,2,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
             {line,60}]},
     {escalus_story,make_all_clients_friends,1,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,106}]}]}}

Report log

@Premwoik Premwoik merged commit 6a008ef into master Feb 3, 2022
@Premwoik Premwoik deleted the s2s-config-host-only branch February 3, 2022 08:04
@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.

3 participants