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

Expose some homeserver functionality to spam checkers #6259

Merged
merged 8 commits into from Oct 31, 2019

Conversation

@turt2live
Copy link
Member

turt2live commented Oct 26, 2019

Primarily for use in matrix-org/mjolnir#7

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off (NV hat)
turt2live added 3 commits Oct 25, 2019
@turt2live turt2live marked this pull request as ready for review Oct 26, 2019
@turt2live turt2live requested a review from matrix-org/synapse-core Oct 26, 2019
@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Oct 28, 2019

I've tried to avoid passing homeserver object to the modules, as it basically ends up forcing us to keep the entire Synapse API stable so as to not break installed modules. Generally we've tried to document the interface of the objects we're passing in, e.g. for password providers we pass in

class ModuleApi(object):

@turt2live turt2live changed the title Offer the homeserver instance to the spam checker Expose some homeserver functionality to spam checkers Oct 28, 2019
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Oct 28, 2019

@erikjohnston please take another look

turt2live added 2 commits Oct 28, 2019
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Oct 29, 2019

Looks like the linter is failing on this, but for everything except the code I'm adding?

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Oct 31, 2019

Looks like the linter is failing on this, but for everything except the code I'm adding?

The linters broke, should be fixed by merging in develop.


As per #synapse-dev:matrix.org, we've tried hard to avoid giving the homeserver object to pluggable modules, as otherwise its impossible to know what interfaces modules depend on. This adds a maintenance burden to synapse devs (makes it harder to know what functionality is relied on by third party modules), third party module devs (they don't know what APIs are stable or when they might just break) and server admins (don't know whether a new version of synapse will break their modules or not without testing, possibly causing service outages).

The flip side to this is that its more annoying to develop modules that use functionality that is not currently exposed by Synapse, as it means opening a PR on Synapse and waiting for that to be merged and released.

However given the fact that modules hook into core parts of Synapse and are security sensitive (e.g. auth providers), we think that the need for stability outweighs the need to be able to quickly add new features.

As such the solution we've adopted is to pass in a well-defined ModuleApi interface to modules, and I'd suggest you use that and adding any necessary extra functionality. (Other potential solutions exist, including adding version constraints, but none of those have been implemented or thought through.)

Copy link
Member

erikjohnston left a comment

c.f. comment above

@turt2live turt2live requested a review from erikjohnston Oct 31, 2019
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Oct 31, 2019

I added said module the day we had that discussion - please take a look.

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Oct 31, 2019

Argh, sorry Travis I completely missed that this had been updated 🤦‍♂

@turt2live turt2live merged commit 3a74c03 into develop Oct 31, 2019
20 checks passed
20 checks passed
buildkite/synapse Build #5303 passed (23 minutes, 11 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 28 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 42 seconds)
Details
buildkite/synapse/isort Passed (17 seconds)
Details
buildkite/synapse/mypy Passed (23 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (15 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pipeline Passed (7 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (17 minutes, 23 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (7 minutes, 6 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (8 minutes, 23 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (7 minutes, 26 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (17 minutes, 53 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 25 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 59 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 54 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (2 minutes, 15 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (19 minutes, 50 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (15 minutes, 25 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (14 minutes, 34 seconds)
Details
@turt2live turt2live deleted the travis/spam-checker branch Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.