CETS support for S2S #4046

arcusfelis · 2023-06-29T14:09:56Z

This PR addresses MIM-1931

Proposed changes include:

Add backend module
Both mnesia and cets are supported
Added s2s/ directory, because there are a lot of s2s files now (total 6). We could also move ejabberd_s2s, ejabberd_s2s_in, ejabberd_s2s_out into this directory later, so all s2s files are located together (not in this PR, because it would make git merging of the feature branch harder).

Changes after the first review:

Use ejabberd_s2s:fromto() type in a lot of places where possible. It simplifies logic a bit (one less argument to pass too).
Move logic functions into mongoose_s2s_lib. ejabberd_s2s module is pretty big already, so separating logic functions and functions that make side effects (i.e. routing, opening connections and writing data across the cluster) helps to understand code faster.
There are limited number of exported functions from mongoose_s2s_lib, so the reader don't need to worry about all extra functions when trying to understand the logic for opening s2s connection.
Handle joining cluster with conflicting secrets working with CETS. There is a new option handle_conflict which helps to choose which record to keep, when record is on both nodes that are trying to join together Conflict handler option cets#4
Added comments to dialback functions, use exml in some places instead of deprecated xml functions.
Removed ejabberd_s2s_out:stop_connection call from ejabberd_s2s_in. There is a comment about it. If we want to stop a connection, it should not be just one connection. But probably we should not stop connections from ejabberd_s2s_in
Made a module with dialback steps mongoose_s2s_dialback - because it is extremely hard to follow dialback logic with the format for packets defined in https://xmpp.org/extensions/xep-0220.html (same tag is reused for key sending and dialback result, i.e. step1 and step4). While db:result for step1 makes no sense at all... So, at least step numbers kinda make sense if you also keep the diagram open.

chrzaszcz

It looks good in general. I added some comments.

src/config/mongoose_config_spec.erl

src/s2s/mongoose_s2s_mnesia.erl

chrzaszcz · 2023-07-03T10:18:12Z

src/s2s/mongoose_s2s_mnesia.erl

+          pid :: pid() | '$1'
+         }).
+
+-record(s2s_secret, {host_type, source, secret}).


The original s2s_shared record had types, could you include them here?

chrzaszcz · 2023-07-03T10:19:48Z

src/ejabberd_s2s.erl

+get_shared_secret(HostType) ->
+    mongoose_s2s_backend:get_shared_secret(HostType).
+
+-spec register_secret(mongooseim:host_type(), ejabberd_s2s:secret_source(), ejabberd_s2s:base16_secret()) -> ok.


Suggested change

-spec register_secret(mongooseim:host_type(), ejabberd_s2s:secret_source(), ejabberd_s2s:base16_secret()) -> ok.

-spec register_secret(mongooseim:host_type(), secret_source(), base16_secret()) -> ok.

src/ejabberd_s2s.erl

chrzaszcz · 2023-07-03T10:36:29Z

src/ejabberd_s2s.erl

    {ok, maps:put(?MODULE, Res, Acc)}.

 -spec key(mongooseim:host_type(), {jid:lserver(), jid:lserver()}, binary()) ->
    binary().
 key(HostType, {From, To}, StreamID) ->
-    Secret = get_shared_secret(HostType),
+    {ok, {_, Secret}} = get_shared_secret(HostType),


Is source used anywhere?

It is used in set_shared_secret.
There is some logic that decides when to generate a random secret, if not specified in the config (and when to overwrite the secret, if specified in the config). Kinda complicated to begin with, just adding Source makes code a bit more understandable in that function.

It is removed now.

src/ejabberd_s2s.erl

src/s2s/mongoose_s2s_cets.erl

chrzaszcz · 2023-07-03T10:41:22Z

src/mongoose_listener_sup.erl

@@ -16,7 +16,8 @@ start_link() ->

 -spec start_child(supervisor:child_spec()) -> ok.
 start_child(ChildSpec) ->
-    {ok, _Pid} = supervisor:start_child(?MODULE, ChildSpec),
+    Res = supervisor:start_child(?MODULE, ChildSpec),
+    check_start_child_result(Res, ChildSpec),


Looks complicated. What's the benefit?

It would add ChildSpec into the error, not just a badmatch.
So, we would know which listener is crashing.

Technically we would get some info from the SASL about the the child failing to start.
But it is still useful to see ChildSpec in {ok, Pid} badmatch.

I could use export function from ejabberd_sup:start_child and use it for logging.
I still like some reporting from start_child.

src/ejabberd_s2s.erl

mongoose-im · 2023-07-07T22:35:26Z

small_tests_24 / small_tests / e567c73
Reports root / small

small_tests_25_arm64 / small_tests / e567c73
Reports root / small

small_tests_25 / small_tests / e567c73
Reports root / small

ldap_mnesia_24 / ldap_mnesia / e567c73
Reports root/ big
OK: 2224 / Failed: 0 / User-skipped: 838 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / e567c73
Reports root/ big
OK: 4198 / Failed: 0 / User-skipped: 90 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / e567c73
Reports root/ big
OK: 4195 / Failed: 0 / User-skipped: 93 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / e567c73
Reports root/ big
OK: 2224 / Failed: 0 / User-skipped: 838 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / e567c73
Reports root/ big
OK: 4198 / Failed: 0 / User-skipped: 90 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / e567c73
Reports root/ big
OK: 4172 / Failed: 0 / User-skipped: 116 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / e567c73
Reports root/ big
OK: 4571 / Failed: 0 / User-skipped: 99 / Auto-skipped: 0

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / e567c73
Reports root/ big
OK: 2730 / Failed: 0 / User-skipped: 671 / Auto-skipped: 0

pgsql_cets_25 / pgsql_cets / e567c73
Reports root/ big
OK: 4547 / Failed: 0 / User-skipped: 123 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / e567c73
Reports root/ big
OK: 2370 / Failed: 0 / User-skipped: 692 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / e567c73
Reports root/ big
OK: 4571 / Failed: 0 / User-skipped: 99 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / e567c73
Reports root/ big
OK: 4557 / Failed: 0 / User-skipped: 113 / Auto-skipped: 0

mongoose-im · 2023-07-10T23:10:56Z

small_tests_24 / small_tests / 26a517b
Reports root / small

small_tests_25_arm64 / small_tests / 26a517b
Reports root / small

small_tests_25 / small_tests / 26a517b
Reports root / small

ldap_mnesia_24 / ldap_mnesia / 26a517b
Reports root/ big
OK: 2224 / Failed: 2 / User-skipped: 838 / Auto-skipped: 0

mod_global_distrib_SUITE:hosts_refresher:test_host_refreshing

{error,
  {{trees_for_connections_present,true,[{times,50,false}],ok},
   [{mongoose_helper,do_wait_until,2,
      [{file,"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
       {line,371}]},
    {mod_global_distrib_SUITE,test_host_refreshing,1,
      [{file,
         "/home/circleci/project/big_tests/tests/mod_global_distrib_SUITE.erl"},
       {line,384}]},
    {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}]}]}}

	-spec register_secret(mongooseim:host_type(), ejabberd_s2s:secret_source(), ejabberd_s2s:base16_secret()) -> ok.
	-spec register_secret(mongooseim:host_type(), secret_source(), base16_secret()) -> ok.

+              is_s2s_allowed_for_host(FromTo, []) ->
+                  not is_service(FromTo) andalso allow_host(FromTo).
+              %% Check if host is in blacklist or white list

	%% Check if host is in blacklist or white list
	%% Check if host is in blacklist or whitelist

		@@ -0,0 +1,46 @@
		%% Some ugly code only used in tests.

		@@ -178,13 +178,20 @@ init([]) ->
		MucIQ,
		ShaperSup]}}.

CETS support for S2S #4046

CETS support for S2S #4046

Conversation

arcusfelis commented Jun 29, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mongoose-im commented Jul 7, 2023 • edited Loading

mongoose-im commented Jul 10, 2023 • edited Loading

mongoose-im commented Jul 10, 2023 • edited Loading

mongoose-im commented Jul 11, 2023 • edited Loading

mongoose-im commented Jul 11, 2023 • edited Loading

pawlooss1 left a comment

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

chrzaszcz Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mongoose-im commented Jul 20, 2023 • edited Loading

mongoose-im commented Jul 20, 2023 • edited Loading

chrzaszcz left a comment

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

mongoose-im commented Jul 21, 2023 • edited Loading

arcusfelis commented Jun 29, 2023 •

edited

Loading

mongoose-im commented Jul 7, 2023 •

edited

Loading

mongoose-im commented Jul 10, 2023 •

edited

Loading

mongoose-im commented Jul 10, 2023 •

edited

Loading

mongoose-im commented Jul 11, 2023 •

edited

Loading

mongoose-im commented Jul 11, 2023 •

edited

Loading

chrzaszcz Jul 14, 2023 •

edited

Loading

mongoose-im commented Jul 20, 2023 •

edited

Loading

mongoose-im commented Jul 20, 2023 •

edited

Loading

mongoose-im commented Jul 21, 2023 •

edited

Loading