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

Multi tenancy hooks #3089

Merged
merged 12 commits into from
Apr 22, 2021
Merged

Multi tenancy hooks #3089

merged 12 commits into from
Apr 22, 2021

Conversation

DenysGonchar
Copy link
Collaborator

@DenysGonchar DenysGonchar commented Apr 18, 2021

this PR reworks ejabberd_hooks in the following way:

  • hooks are running either globally or for a host type, not domain name, so they must be registered globally or for a host type as well.
  • hooks execution is fully wrapped in mongoose_hooks module.
  • when hook handler expect some exact constant initial Acc value, such value is hardcoded mongoose_hooks module. It should not be possible to run the hook with another initial acc value.
  • mongoose_acc:t() type is extended with host_type field, this allows to reduce the number of calls to mongoose_domain_api:get_host_type/1 function.
  • ejabberd_c2s modules is now running hooks for a host_type, note that only hooks execution is updated. if some hook was expecting a domain name as one of the parameters, it still receives a domain name. we may need to change it in the future, but it requires updates of the hook handlers (which are mostly in the MIM modules).
  • remove_domain hook handler implementation for ejaberd_auth

Another big generic module that triggers hooks is ejabberd_sm (13 calls to mongoose_hook). But it should be reworked in a separate PR, and probably after switching back to the master branch.

@codecov
Copy link

codecov bot commented Apr 18, 2021

Codecov Report

Merging #3089 (f65aadf) into multi-tenancy-phase-1 (f991ee2) will increase coverage by 0.64%.
The diff coverage is 89.13%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           multi-tenancy-phase-1    #3089      +/-   ##
=========================================================
+ Coverage                  78.09%   78.74%   +0.64%     
=========================================================
  Files                        374      387      +13     
  Lines                      31219    31691     +472     
=========================================================
+ Hits                       24382    24954     +572     
+ Misses                      6837     6737     -100     
Impacted Files Coverage Δ
src/admin_extra/service_admin_extra.erl 100.00% <ø> (ø)
src/ejabberd_gen_sm.erl 90.90% <ø> (ø)
src/ejabberd_s2s_in.erl 77.35% <ø> (ø)
src/ejabberd_users.erl 81.63% <ø> (ø)
src/metrics/mongoose_metrics_mam_hooks.erl 86.66% <ø> (ø)
src/mod_stream_management.erl 85.10% <ø> (ø)
src/mongoose_hooks.erl 91.48% <ø> (-0.76%) ⬇️
src/offline/mod_offline_mnesia.erl 51.47% <0.00%> (ø)
src/pubsub/pubsub_form_utils.erl 56.60% <0.00%> (+1.04%) ⬆️
src/rdbms/rdbms_queries_mssql.erl 100.00% <ø> (ø)
... and 146 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 82330e3...f65aadf. Read the comment docs.

…are wrapped in mongoose_hooks module.

hint: all inderect calls to ejabberd_hooks found using 'ejabberd_hooks[^:]' search regex
@DenysGonchar DenysGonchar force-pushed the multi-tenancy-hooks branch 2 times, most recently from 2150d7f to 159e5c4 Compare April 19, 2021 01:25
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

I appreciate the huge amount of good work, especially putting the defaults in mongoose_hooks. I have a few comments:

  • Are you planning to use mongoose_acc:get_host_type inside the hooks to get rid of the host_type argument for each of them? For me it looks like a natural follow-up.
  • Please revise the line breaks in mongoose_hooks to break the comments so that expressions like a server, the client, should be, client jid etc remain in one line. I think it's good practice to do so.

src/auth/ejabberd_auth.erl Outdated Show resolved Hide resolved
src/auth/ejabberd_auth_dummy.erl Show resolved Hide resolved
src/ejabberd_c2s.erl Outdated Show resolved Hide resolved
src/ejabberd_c2s.erl Show resolved Hide resolved
src/mongoose_acc.erl Outdated Show resolved Hide resolved
src/mongoose_hooks.erl Outdated Show resolved Hide resolved
src/mongoose_hooks.erl Outdated Show resolved Hide resolved
test/acc_SUITE.erl Outdated Show resolved Hide resolved
test/acc_SUITE.erl Outdated Show resolved Hide resolved
test/acc_SUITE.erl Outdated Show resolved Hide resolved
@DenysGonchar
Copy link
Collaborator Author

* Are you planning to use `mongoose_acc:get_host_type` inside the hooks to get rid of the `host_type` argument for each of them? For me it looks like a natural follow-up.

The idea was to reduce calls to mongoose_acc:get_host_type/1 as much as possible. Ideally it should be limited to just one call per incoming message (to identify receiver's host_type, if required), we don't want it to become a bottleneck.

@arcusfelis
Copy link
Contributor

We could always ask for acc argument, that would have host_type field.
and if hook handlers need host_type - they could extract it from the acc.
And if handlers need a domain, they already have server in the acc.

But it's more lines of code to change :D

And after that we could pass Acc everywhere, even into the backend functions.

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

One minor comment, please tell me whether you are correcting it or if I should merge.

%% new with 'undefined' host type
Acc2 = mongoose_acc:new(AccParams#{lserver => <<"unknown.host1">>}),
%% host type changes from <<"host 1">> to 'undefined'
Acc2 = mongoose_acc:strip(StripParams#{lserver => <<"unknown.host1">>},Acc1),
Copy link
Member

Choose a reason for hiding this comment

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

Minor: missing commas before Acc1, this happens a few times below.

@chrzaszcz chrzaszcz merged commit 33af465 into multi-tenancy-phase-1 Apr 22, 2021
@chrzaszcz chrzaszcz deleted the multi-tenancy-hooks branch April 22, 2021 06:52
@Premwoik Premwoik added this to the 5.0.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants