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

Listener configuration rework #3628

Merged
merged 27 commits into from
Apr 20, 2022
Merged

Listener configuration rework #3628

merged 27 commits into from
Apr 20, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Apr 11, 2022

The main goal is to use the maps with defaults for all listener options and to pass such maps to all started listeners.

Several parts of listener management logic are changed:

  • The new logic for listener management is placed in mongoose_listener, which is a behaviour. The modules ejabberd_c2s, ejabberd_s2s_in, ejabberd_service and ejabberd_cowboy implement this behaviour. The start_listener/1 function allows for a unified way to start particular listeners. They might delegate the startup to mongoose_tcp_listener or have their own special logic (only ejabberd_cowboy is doing so).
  • Options in a map are now passed to the listener modules. Each module has its own type with mandatory options. This makes it easier to reason about the mandatory options for each module. I think this approach is better than having one huge spec with options for all possible listeners, because it enforces mandatory options.
  • There is a separate module for the supervisor, mongoose_listener_sup. This makes it easier to understand the logic.

The listeners are (and were) parsed from a TOML section (which contained a nested list for each listener type) to a flat map. I decided to use two kinds of paths in test helpers:

  • config_parser_helper:default_config/1 has TOML paths, like [listen, c2s], which allows specifying defaults per listener type.
  • config_parser_SUITE uses the resulting paths, e.g. [listen, 1], which means the first listener. This way expected option values can be tested one by one. The support for list indexes (1 in the example) is a new feature. We can add it to mongoose_config in the future, should we need it outside the tests.

A bug was also fixed - wrong options were passed to the modules from BOSH/websockets. The new type specs revealed this issue.

The rework of TLS and handlers will be done in a separate task.

@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #3628 (01ed2ab) into master (30e9a1b) will increase coverage by 0.05%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #3628      +/-   ##
==========================================
+ Coverage   81.00%   81.06%   +0.05%     
==========================================
  Files         427      427              
  Lines       32085    31963     -122     
==========================================
- Hits        25991    25911      -80     
+ Misses       6094     6052      -42     
Impacted Files Coverage Δ
src/ejabberd_sup.erl 85.71% <ø> (ø)
src/mongoose_listener_config.erl 94.44% <50.00%> (+1.58%) ⬆️
src/ejabberd_service.erl 67.09% <66.66%> (-0.18%) ⬇️
src/mongoose_listener.erl 72.72% <72.72%> (ø)
src/ejabberd_receiver.erl 78.87% <80.00%> (-0.04%) ⬇️
src/ejabberd_socket.erl 55.81% <80.00%> (+0.13%) ⬆️
src/mongoose_tcp_listener.erl 82.92% <85.71%> (+6.33%) ⬆️
src/config/mongoose_config_spec.erl 99.22% <100.00%> (+<0.01%) ⬆️
src/ejabberd_app.erl 94.36% <100.00%> (ø)
src/ejabberd_c2s.erl 88.94% <100.00%> (+0.29%) ⬆️
... and 25 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 30e9a1b...01ed2ab. Read the comment docs.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz changed the title Listener options in maps Listener configuration rework Apr 15, 2022
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the listener-config-rework branch 2 times, most recently from a017e78 to 6b90f54 Compare April 15, 2022 15:24
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the listener-config-rework branch 3 times, most recently from 088506b to d4e2df8 Compare April 19, 2022 11:42
- Use maps with defaults for listener-sepcific opts.
- Avoid changing keys for options, e.g. for 'handlers' and 'tcp'.
- Accept only TCP in proto - all listeners support only TCP
  The 'proto' opt could be removed, but I didn't want to do it all at once.
- Make 'password' required for service - it was documented as such
  and the server wouldn't start without it.

HTTP handlers and TLS options will be amended separately.
Separate per-module logic from per-listener logic

The module is now a behaviour with the following implementations:
  - ejabberd_c2s
  - ejabberd_s2s_in
  - ejabberd_service
  - ejabberd_cowboy
This way the code is more organized
Define a type that enforces mandatory options.

- Make the options with defaults mandatory
- SSL opt filtering is now unnecessary
Using maps allowed for simpler option processing
The new logic is implemented in mongoose_listener
Add a type for expected mandatory options.
The map might contain general listener options as well.
Add a type for expected mandatory options.
The map might contain general listener options as well.
Add a type for expected mandatory options.
The map might contain general listener options as well.
Use maps and separate options from connection details.
Pass correct options to the receiver, as previously inet options
were passed there (which did not make sense, defaults were used)
Make a type for required opts, the map might include additional
listener-specific ones.

Simplify options processing.
There are no listeners using it and the code is dead.
- New names
- No UDP
- New require opts
The paths are TOML ones, e.g. [listen, s2s]
The resulting path would be [listen] or, more precisely,
[listen, N] where N is the N-th element of the list.
However, this would make it impossible to disinguish the listener types.
- Group the tests by listener types rather than by options.
  This makes test layout more similar to the config spec layout.
- Introduce a new path notion - an integer in the path means an index
  in a list (TOML array), e.g. [listen, 1] is the first listener.
  The indexes start at one to be consistent with lists:nth/2
  This change allows testing config options individually, just like e.g.
  for modules.
- Add several missing tests.
Adjust the changed options accordingly.
Previous logic would put 'undefined' in the config options.
The default value is 'infinity' so it makes sense to make this value valid.
@chrzaszcz chrzaszcz changed the base branch from master to cert-cache-fix April 19, 2022 12:38
@mongoose-im
Copy link
Collaborator

mongoose-im commented Apr 19, 2022

small_tests_24 / small_tests / ef3e1e4
Reports root / small


small_tests_23 / small_tests / ef3e1e4
Reports root / small


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / ef3e1e4
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / ef3e1e4
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / ef3e1e4
Reports root/ big
OK: 2841 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / ef3e1e4
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / ef3e1e4
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / ef3e1e4
Reports root/ big
OK: 1547 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / ef3e1e4
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / ef3e1e4
Reports root/ big
OK: 1854 / Failed: 0 / User-skipped: 368 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / ef3e1e4
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / ef3e1e4
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / ef3e1e4
Reports root/ big
OK: 3227 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / ef3e1e4
Reports root/ big
OK: 1697 / Failed: 0 / User-skipped: 367 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / ef3e1e4
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0

Base automatically changed from cert-cache-fix to master April 19, 2022 13:08
@chrzaszcz chrzaszcz marked this pull request as ready for review April 19, 2022 13:11
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.

ok

doc/configuration/listen.md Outdated Show resolved Hide resolved
TLSOpts = verify_opts(Verify) ++ TLSOpts1,
[ssl_crl_cache:insert({file, CRL}) || CRL <- proplists:get_value(crlfiles, Opts, [])],
TLSOpts = verify_opts(Verify) ++ TLSConfig,
[ssl_crl_cache:insert({file, CRL}) || CRL <- proplists:get_value(crlfiles, TLSConfig, [])],
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, that crlfiles are not tested and insert should probably be on the listener init.
(not related to the PR though)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I left TLS options unchanged for now.

fsm_limit_opts(#{max_fsm_queue := N}) ->
[{max_queue, N}];
fsm_limit_opts(#{}) ->
case mongoose_config:lookup_opt(max_fsm_queue) of
Copy link
Contributor

Choose a reason for hiding this comment

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

the lookup could be done once in the init of the listener

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe limit the extent of changes for this PR?

src/ejabberd_s2s_in.erl Show resolved Hide resolved
src/ejabberd_service.erl Outdated Show resolved Hide resolved
src/mongoose_listener.erl Outdated Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

mongoose-im commented Apr 20, 2022

small_tests_24 / small_tests / 01ed2ab
Reports root / small


small_tests_23 / small_tests / 01ed2ab
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 01ed2ab
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 01ed2ab
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / 01ed2ab
Reports root/ big
OK: 2841 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 01ed2ab
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 01ed2ab
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 01ed2ab
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 01ed2ab
Reports root/ big
OK: 1547 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 01ed2ab
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 01ed2ab
Reports root/ big
OK: 1854 / Failed: 0 / User-skipped: 368 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 01ed2ab
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 01ed2ab
Reports root/ big
OK: 3239 / Failed: 1 / User-skipped: 147 / Auto-skipped: 0

muc_SUITE:hibernation:hibernated_room_can_be_queried_for_archive
{error,{{assertion_failed,assert,is_groupchat_message,
              [<<"Restorable message">>],
              undefined,"undefined"},
    [{escalus_new_assert,assert_true,2,
               [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
                {line,84}]},
     {muc_SUITE,wait_for_mam_result,3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4383}]},
     {muc_SUITE,'-hibernated_room_can_be_queried_for_archive/1-fun-0-',3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4124}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {muc_SUITE,hibernated_room_can_be_queried_for_archive,1,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4120}]},
     {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


mssql_mnesia_24 / odbc_mssql_mnesia / 01ed2ab
Reports root/ big
OK: 3244 / Failed: 1 / User-skipped: 142 / Auto-skipped: 0

muc_SUITE:hibernation:hibernated_room_can_be_queried_for_archive
{error,{{assertion_failed,assert,is_groupchat_message,
              [<<"Restorable message">>],
              undefined,"undefined"},
    [{escalus_new_assert,assert_true,2,
               [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
                {line,84}]},
     {muc_SUITE,wait_for_mam_result,3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4383}]},
     {muc_SUITE,'-hibernated_room_can_be_queried_for_archive/1-fun-0-',3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4124}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {muc_SUITE,hibernated_room_can_be_queried_for_archive,1,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4120}]},
     {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


riak_mnesia_24 / riak_mnesia / 01ed2ab
Reports root/ big
OK: 1697 / Failed: 0 / User-skipped: 367 / Auto-skipped: 0

@arcusfelis arcusfelis merged commit 6d1c24b into master Apr 20, 2022
@arcusfelis arcusfelis deleted the listener-config-rework branch April 20, 2022 09:40
@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.

4 participants