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

Do not change the options in mongoose_config #3356

Merged
merged 9 commits into from
Oct 26, 2021

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Oct 22, 2021

Remove the usage of set_opt and unset_opt from the code, making the config in persistent terms constant for the whole lifetime of the MongooseIM app.

Highlights:

  • gen_mod is no longer modifying the config - it only sets the opts in ejabberd_modules now. The only module using the config directly was mod_mam_meta and this is now fixed.
  • ejabberd_auth is not setting any options when it starts. JWT secret is just stored in a separate persistent term.
  • Randomly generated S2S shared secred is stored in a Mnesia table as it has to be the same for all cluster nodes.
  • Listeners are never modified when MongooseIM is running, so the unused functions for doing so were removed.

See the commit messages for more details.

@mongoose-im

This comment has been minimized.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #3356 (0d0f1fa) into persistent-term-config (397ca5f) will decrease coverage by 0.09%.
The diff coverage is 84.61%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           persistent-term-config    #3356      +/-   ##
==========================================================
- Coverage                   80.80%   80.71%   -0.10%     
==========================================================
  Files                         397      397              
  Lines                       32287    32234      -53     
==========================================================
- Hits                        26091    26017      -74     
- Misses                       6196     6217      +21     
Impacted Files Coverage Δ
src/auth/ejabberd_auth.erl 83.69% <ø> (-0.44%) ⬇️
src/config/mongoose_config.erl 96.87% <ø> (ø)
src/config/mongoose_config_spec.erl 98.55% <ø> (ø)
src/ejabberd_listener.erl 70.00% <ø> (-2.80%) ⬇️
src/mam/mod_mam_cassandra_arch.erl 84.68% <0.00%> (ø)
src/mam/mod_mam_cassandra_prefs.erl 80.61% <0.00%> (ø)
src/mam/mod_mam_meta.erl 95.18% <ø> (-1.67%) ⬇️
.../system_metrics/mongoose_system_metrics_sender.erl 96.55% <ø> (-0.51%) ⬇️
src/ejabberd_s2s.erl 81.25% <88.88%> (-0.26%) ⬇️
...system_metrics/service_mongoose_system_metrics.erl 73.01% <90.00%> (+0.70%) ⬆️
... and 16 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 dc25c80...0d0f1fa. Read the comment docs.

@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.

Base automatically changed from mongoose_config to persistent-term-config October 25, 2021 22:11
@mongoose-im

This comment has been minimized.

Module options should be set only in module opts,
stored by gen_mod in the 'ejabberd_modules' ETS table.

The only exception was mod_mam_meta, which stored config for mod_mam.
It is possible to avoid this completely and just use the module opts.

There is no dynamic module reload now, so nothing will get out of sync.
This was used only by ejabberd_auth_jwt to store the secret.
This secret can be stored in a persistent term as it remains constant
for the whole runtime.

Also: do not convert the algorithm from binary to list and back
This key needs to be the same for all nodes for successful server dialback.
It was stored in global config before.
Listeners are configured only once at startup
and their setup does not change.
There is no need to rewrite config options by system metrics.
Remove the tests relying on the overwritten config options,
replacing them with actually checking all the tracking ids.
@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@chrzaszcz chrzaszcz marked this pull request as ready for review October 26, 2021 08:19
Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Very nice refactoring of the tracking ids for sytem metrics 👍🏽

@@ -75,7 +70,7 @@ authorize(Creds) ->
LServer :: jid:lserver(),
Password :: binary()) -> boolean().
check_password(HostType, LUser, LServer, Password) ->
Key = case ejabberd_auth:get_opt(HostType, jwt_secret) of
Key = case persistent_term:get({?MODULE, HostType, jwt_secret}) of
Key1 when is_binary(Key1) -> Key1;
{env, Var} -> list_to_binary(os:getenv(Var))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering that env-vars are private to a process and they're impossible to change externally, I'd also cache the value of the env-var in the persistent_term entry, instead of repeatedly reading such env-var. The reason why somebody would prefer deploying the secret as an env-var is that UNIX process memory has much stronger security semantics than filesystem permissions. Otherwise, the value might be kept cached in the process memory for much faster access.

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 mostly agree, but for me this modification would be out of scope of this PR.

src/system_metrics/service_mongoose_system_metrics.erl Outdated Show resolved Hide resolved
big_tests/tests/service_mongoose_system_metrics_SUITE.erl Outdated Show resolved Hide resolved
src/mam/mod_mam_meta.erl Show resolved Hide resolved
src/ejabberd_s2s.erl Show resolved Hide resolved
src/ejabberd_s2s.erl Show resolved Hide resolved
Copy link
Contributor

@vkatsuba vkatsuba left a comment

Choose a reason for hiding this comment

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

Could you please also fix this comment in the scope of current PR? https://github.com/esl/MongooseIM/pull/3343/files#r735601211

@NelsonVides
Copy link
Collaborator

Hmmm technically speaking my comments on hashes and security are not the intent of this PR, so we might as well fix separately in other PRs reserved for that topic and then get this one approved and merged. Or in separated commits here. I have no preference, just want those fixes eventually 🙂

@mongoose-im
Copy link
Collaborator

mongoose-im commented Oct 26, 2021

small_tests_24 / small_tests / e85c4ea
Reports root / small


internal_mnesia_24 / internal_mnesia / e85c4ea
Reports root/ big
OK: 1586 / Failed: 0 / User-skipped: 297 / Auto-skipped: 0


small_tests_23 / small_tests / e85c4ea
Reports root / small


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / e85c4ea
Reports root/ big
OK: 2699 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / e85c4ea
Reports root/ big
OK: 2699 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / e85c4ea
Reports root/ big
OK: 1483 / Failed: 0 / User-skipped: 400 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / e85c4ea
Reports root/ big
OK: 2699 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / e85c4ea
Reports root/ big
OK: 1483 / Failed: 0 / User-skipped: 400 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / e85c4ea
Reports root/ big
OK: 2688 / Failed: 1 / User-skipped: 201 / Auto-skipped: 0

mam_SUITE:rdbms_async_pool_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster
{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}

Report log


pgsql_mnesia_24 / pgsql_mnesia / e85c4ea
Reports root/ big
OK: 3067 / Failed: 1 / User-skipped: 211 / Auto-skipped: 0

service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join
{error,{test_case_failed,{[<<"example1.com">>,<<"example2.com">>,
               <<"example3.com">>],
              [<<"example1.com">>,<<"example2.com">>,
               <<"example3.com">>,<<"example4.com">>]}}}

Report log


mysql_redis_24 / mysql_redis / e85c4ea
Reports root/ big
OK: 3056 / Failed: 2 / User-skipped: 228 / Auto-skipped: 0

mam_SUITE:rdbms_prefs_cases:messages_filtered_when_prefs_default_policy_is_never
{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}

Report log

mam_SUITE:rdbms_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster
{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}

Report log


pgsql_mnesia_23 / pgsql_mnesia / e85c4ea
Reports root/ big
OK: 3068 / Failed: 0 / User-skipped: 211 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / e85c4ea
Reports root/ big
OK: 1859 / Failed: 0 / User-skipped: 323 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / e85c4ea
Reports root/ big
OK: 3093 / Failed: 1 / User-skipped: 211 / Auto-skipped: 0

rest_client_SUITE:muc:messages_can_be_paginated_in_room
{error,
  {{assertion_failed,assert,is_chat_message,
     [<<"15303add">>],
     {xmlel,<<"message">>,
       [{<<"to">>,<<"[email protected]">>},
        {<<"type">>,<<"chat">>}],
       [{xmlel,<<"body">>,[],[{xmlcdata,<<"93916f82">>}]}]},
     "<message to='[email protected]' type='chat'><body>93916f82</body></message>"},
   [{escalus_new_assert,assert_true,2,
      [{file,
         "/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
       {line,84}]},
    {rest_client_SUITE,assert_room_messages,2,
      [{file,"/home/circleci/app/big_tests/tests/rest_client_SUITE.erl"},
       {line,772}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/app/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


riak_mnesia_24 / riak_mnesia / e85c4ea
Reports root/ big
OK: 1712 / Failed: 1 / User-skipped: 326 / Auto-skipped: 0

mod_ping_SUITE:server_ping_kill:server_ping_pong
{error,{{badmatch,[{[<<"localhost">>,mod_ping,ping_response],
          {expected_diff,5},
          {before_story,5},
          {after_story,9}}]},
    [{escalus_mongooseim,post_story_check_metrics,1,
               [{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
                {line,74}]},
     {escalus_mongooseim,maybe_check_metrics_post_story,1,
               [{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
                {line,51}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,75}]},
     {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

@mongoose-im
Copy link
Collaborator

mongoose-im commented Oct 26, 2021

small_tests_24 / small_tests / 0d0f1fa
Reports root / small


internal_mnesia_24 / internal_mnesia / 0d0f1fa
Reports root/ big
OK: 1586 / Failed: 0 / User-skipped: 297 / Auto-skipped: 0


small_tests_23 / small_tests / 0d0f1fa
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 0d0f1fa
Reports root/ big
OK: 2699 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 0d0f1fa
Reports root/ big
OK: 1483 / Failed: 0 / User-skipped: 400 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 0d0f1fa
Reports root/ big
OK: 2705 / Failed: 2 / User-skipped: 184 / Auto-skipped: 0

service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join
{error,{test_case_failed,{[<<"example1.com">>,<<"example2.com">>,
               <<"example3.com">>],
              [<<"example1.com">>,<<"example2.com">>,
               <<"example3.com">>,<<"example4.com">>]}}}

Report log

sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_stop_c2s
{error,{thrown,{timeout,msg}}}

Report log


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 0d0f1fa
Reports root/ big
OK: 2699 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / 0d0f1fa
Reports root/ big
OK: 2688 / Failed: 1 / User-skipped: 201 / Auto-skipped: 0

mam_SUITE:rdbms_simple_prefs_cases:messages_filtered_when_prefs_default_policy_is_never
{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}

Report log


ldap_mnesia_23 / ldap_mnesia / 0d0f1fa
Reports root/ big
OK: 1483 / Failed: 0 / User-skipped: 400 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 0d0f1fa
Reports root/ big
OK: 1914 / Failed: 3 / User-skipped: 323 / Auto-skipped: 0

sm_SUITE:parallel:messages_are_properly_flushed_during_resumption
{error,
  {{badmatch,
     {error,
       {connection_step_failed,
         {#Fun<sm_SUITE.11.118723036>,
          {client,
            <<"alicE_messages_are_properly_flushed_during_resumption_18.117060@localhost">>,
            escalus_tcp,<0.20181.1>,undefined,
            [{username,
               <<"alicE_messages_are_properly_flushed_during_resumption_18.117060">>},
             {server,<<"localhost">>},
             {password,<<"matygrysa">>},
             {stream_management,true},
             {host,<<"localhost">>},
             {stream_id,<<"4f7d3039a35bf615">>}]},
          [{compression,[<<"zlib">>]},
           {starttls,true},
           {stream_management,true},
           {advanced_message_processing,true},
           {client_state_indication,false},
           {sasl_mechanisms,[<<"SCRAM-SHA-256">>,<<"PLAIN">>]},
           {caps,undefined}]},
         {timeout,get_resumed}}}},
   [{sm_SUITE,'-messages_are_properly_flushed_during_resumption/1-fun-1-',
      3,
      [{file,"/home/circleci/app/big_tests/tests/sm_SUITE.erl"},
       {line,1230}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/app/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

sm_SUITE:parallel:messages_are_properly_flushed_during_resumption
{error,
  {{badmatch,
     {error,
       {connection_step_failed,
         {#Fun<sm_SUITE.11.118723036>,
          {client,
            <<"alicE_messages_are_properly_flushed_during_resumption_24.87395@localhost">>,
            escalus_tcp,<0.20653.1>,undefined,
            [{username,
               <<"alicE_messages_are_properly_flushed_during_resumption_24.87395">>},
             {server,<<"localhost">>},
             {password,<<"matygrysa">>},
             {stream_management,true},
             {host,<<"localhost">>},
             {stream_id,<<"584fe25d409ad866">>}]},
          [{compression,[<<"zlib">>]},
           {starttls,true},
           {stream_management,true},
           {advanced_message_processing,true},
           {client_state_indication,false},
           {sasl_mechanisms,[<<"SCRAM-SHA-256">>,<<"PLAIN">>]},
           {caps,undefined}]},
         {timeout,get_resumed}}}},
   [{sm_SUITE,'-messages_are_properly_flushed_during_resumption/1-fun-1-',
      3,
      [{file,"/home/circleci/app/big_tests/tests/sm_SUITE.erl"},
       {line,1230}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/app/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

sm_SUITE:parallel:messages_are_properly_flushed_during_resumption_p1_fsm_old
{error,
  {{badmatch,
     {error,
       {connection_step_failed,
         {#Fun<sm_SUITE.11.118723036>,
          {client,
            <<"alicE_messages_are_properly_flushed_during_resumption_p1_fsm_old_24.66127@localhost">>,
            escalus_tcp,<0.20678.1>,undefined,
            [{username,
               <<"alicE_messages_are_properly_flushed_during_resumption_p1_fsm_old_24.66127">>},
             {server,<<"localhost">>},
             {password,<<"matygrysa">>},
             {stream_management,true},
             {host,<<"localhost">>},
             {stream_id,<<"7899516f30e3b3ca">>}]},
          [{compression,[<<"zlib">>]},
           {starttls,true},
           {stream_management,true},
           {advanced_message_processing,true},
           {client_state_indication,false},
           {sasl_mechanisms,[<<"SCRAM-SHA-256">>,<<"PLAIN">>]},
           {caps,undefined}]},
         {timeout,get_resumed}}}},
   [{sm_SUITE,
      '-messages_are_properly_flushed_during_resumption_p1_fsm_old/1-fun-1-',
      3,
      [{file,"/home/circleci/app/big_tests/tests/sm_SUITE.erl"},
       {line,1274}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/app/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,...

Report log


pgsql_mnesia_24 / pgsql_mnesia / 0d0f1fa
Reports root/ big
OK: 3068 / Failed: 0 / User-skipped: 211 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 0d0f1fa
Reports root/ big
OK: 3051 / Failed: 0 / User-skipped: 228 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 0d0f1fa
Reports root/ big
OK: 3068 / Failed: 0 / User-skipped: 211 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 0d0f1fa
Reports root/ big
OK: 3067 / Failed: 1 / User-skipped: 211 / Auto-skipped: 0

service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join
{error,{test_case_failed,{[<<"example1.com">>,<<"example2.com">>,
               <<"example3.com">>],
              [<<"example1.com">>,<<"example2.com">>,
               <<"example3.com">>,<<"example4.com">>]}}}

Report log


riak_mnesia_24 / riak_mnesia / 0d0f1fa
Reports root/ big
OK: 1731 / Failed: 1 / User-skipped: 326 / Auto-skipped: 0

rest_client_SUITE:muc:messages_can_be_paginated_in_room
{error,
  {{assertion_failed,assert,is_chat_message,
     [<<"7a7aa227">>],
     {xmlel,<<"message">>,
       [{<<"to">>,<<"[email protected]">>},
        {<<"type">>,<<"chat">>}],
       [{xmlel,<<"body">>,[],[{xmlcdata,<<"1e0b62c9">>}]}]},
     "<message to='[email protected]' type='chat'><body>1e0b62c9</body></message>"},
   [{escalus_new_assert,assert_true,2,
      [{file,
         "/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
       {line,84}]},
    {rest_client_SUITE,assert_room_messages,2,
      [{file,"/home/circleci/app/big_tests/tests/rest_client_SUITE.erl"},
       {line,772}]},
    {rest_client_SUITE,'-messages_can_be_paginated_in_room/1-fun-0-',3,
      [{file,"/home/circleci/app/big_tests/tests/rest_client_SUITE.erl"},
       {line,538}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/app/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


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 0d0f1fa
Reports root/ big
OK: 2699 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 0d0f1fa
Reports root/ big
OK: 1859 / Failed: 0 / User-skipped: 323 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 0d0f1fa
Reports root/ big
OK: 3068 / Failed: 0 / User-skipped: 211 / Auto-skipped: 0

@chrzaszcz
Copy link
Member Author

chrzaszcz commented Oct 26, 2021

Could you please also fix this comment in the scope of current PR? https://github.com/esl/MongooseIM/pull/3343/files#r735601211

done

Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Love it 🎉

@NelsonVides NelsonVides merged commit d4ff52f into persistent-term-config Oct 26, 2021
@NelsonVides NelsonVides deleted the static-mongoose-config branch October 26, 2021 12:21
@Premwoik Premwoik modified the milestone: 5.1.0 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