Prepare for dynamic domains in big tests #3109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3109 +/- ##
==========================================
+ Coverage 79.40% 79.42% +0.01%
==========================================
Files 395 395
Lines 32127 32147 +20
==========================================
+ Hits 25512 25533 +21
+ Misses 6615 6614 -1
Continue to review full report at Codecov.
|
chrzaszcz
force-pushed
the
multi-tenancy-tests
branch
from
4f3b6bc
to
f29e37d
Compare
chrzaszcz
force-pushed
the
multi-tenancy-tests
branch
8 times, most recently
from
4b05240
to
e7b1d55
Compare
The implementation of 'get_host_type' changed, but the calls did not,
which led to accepting subdomains where they should not be accepted.
Also:
- Call 'filter_local_packet' for host type
- To work correctly for subdomains,
'mongoose_subhosts' needs to be used temporarily.
- Configure the host type in the test spec
- Remove domain_isolation_SUITE (for now)
- It was passing before because both the test and the code
were not using dynamic domains, it started failing
after the previous commit, which fixed the code.
- The module itself has a bug (reported)
- It uses MUC Light which lacks dynamic domain support (for now)
- It needs to use host types
Motivation:
- Avoiding unnecessary ETS lookups.
- Limiting 'mongoose_acc:new' logic to handling the data structure
- It is not clear whether the default mapping should use
'get_host_type' or 'get_domain_host_type'.
- Sometimes it is much better for the module to verify the result,
e.g. make sure the host type exists.
- The host type was sometimes not needed, but it was obtained
implicitly. Most often when it is actually needed,
it is already known to the caller so it can be set explicitly,
sparing the extra ETS lookup.
IMO this good practice is further encouraged by not having a default.
chrzaszcz
force-pushed
the
multi-tenancy-tests
branch
from
e7b1d55
to
a5ccb12
Compare
chrzaszcz
changed the title
| @@ -9,7 +9,7 @@ | |||
| %% http://www.erlang.org/doc/apps/common_test/run_test_chapter.html#test_specifications | |||
| {include, "tests"}. | |||
|
|
|||
| {suites, "tests", domain_isolation_SUITE}. | |||
| {suites, "tests", acc_e2e_SUITE}. | |||
There was a problem hiding this comment.
why do we remove domain_isolation_SUITE?
There was a problem hiding this comment.
This suite was passing only because filter_local_packet was executed per domain. The mod_domain_isolation module does not support dynamic domains yet. The tests suite itself also does not use host types correctly and it needs to be addressed. I think it should be done in a separate story.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes it possible to run big tests with the dynamic domains test spec.
It includes a few separate changes, but they are all connected:
acc_e2e_SUITE.get_host_typewithget_domain_host_type...mongoose_subhosts...filter_local_packetfor host types...The last commit is optional. The code can work without it, but after spending some time on testing different versions this one seems to be the best. I explained the motivation in the commit message.
This PR does not include tests for the bug related to the use in
get_host_typein too many places. I think it can be done in a follow-up PR. Manual test shows that the bug is not present anymore.