Synapse charm review PR #43
Conversation
|
Test coverage for 73677fa Static code analysis report |
| @@ -0,0 +1,9 @@ | |||
| ignore_files: | |||
There was a problem hiding this comment.
Perhaps check if all of these still need to be on the ignore list? Or if we can take some off
| @@ -0,0 +1,4 @@ | |||
| lib/charms/nginx_ingress_integrator/v0/nginx_route.py | |||
There was a problem hiding this comment.
Looks like we have both wokieignoreandignoreinwoke.yaml`?
There was a problem hiding this comment.
Perhaps a good chance to merge this into charmcraft.yaml along with config.yaml and anything else?
There was a problem hiding this comment.
Also potentially merge this into charmcraft.yaml
| requires: | ||
| ingress: | ||
| interface: ingress | ||
| limit: 2 |
There was a problem hiding this comment.
What is the reason for making this 2 please? I would have guessed that 1 ingress is sufficient? Perhaps worthwhile documenting the reason since others in the future might also wonder
There was a problem hiding this comment.
We should probably consider dropping this relation
| nginx-route: | ||
| interface: nginx-route | ||
| limit: 1 | ||
| optional: true |
There was a problem hiding this comment.
I guess with ingress v2 we should drop support for this since it should do what we need?
There was a problem hiding this comment.
Just a note to remind us that we should consider making this a separate snap rather than including it in the repo since this might be useful elsewhere as well. Like if someone wants to install synapse manually and still would like mjolnir on that installation they might like having a snap available
There was a problem hiding this comment.
Out of curiosity, if we turn this into a snap do we maintain that too? I'm asking to understand how much overhead that would increase :D
| straightforward through Juju’s clean interface. It will allow easy deployment | ||
| into multiple environments for testing of changes. | ||
|
|
||
| ## Project and community |
There was a problem hiding this comment.
This section is not needed anymore here and we lack "Contributing to this documentation" and "In this documentation" sections
| parts: | ||
| add-user: | ||
| plugin: nil | ||
| overlay-script: | |
There was a problem hiding this comment.
Now there's the possibility of using the user rocks provides out of the box
There was a problem hiding this comment.
daemon with the next available UID on that range, 584792.
From the rockcraft spec RK011 might be helpful as a reference here!
|
|
||
| """This module defines constants used throughout the Synapse application.""" | ||
|
|
||
| CHECK_ALIVE_NAME = "synapse-alive" |
There was a problem hiding this comment.
Some of these constants are only used in pebble.py. I suggest we move them there
There was a problem hiding this comment.
That is a good point, constants, types and anything else should be defined as close as possible to where they are used unless there are circular import issues in which case it makes sense to move them to separate files. The drawback of moving them to separate files is that it is an additional layer of indirection which adds mental overhead to reading the code. This is ok as long as there is a benefit to the additional layer of indirection
| COMMAND_MIGRATE_CONFIG = "migrate_config" | ||
| SYNAPSE_CONFIG_DIR = "/data" | ||
| MJOLNIR_CONFIG_PATH = f"{SYNAPSE_CONFIG_DIR}/config/production.yaml" | ||
| MJOLNIR_HEALTH_PORT = 7777 |
There was a problem hiding this comment.
MJOLNIR_HEALTH_PORT and perhaps other constants are used only in tests. They shouldn't be here
| """Exceptions used by the Synapse charm.""" | ||
|
|
||
|
|
||
| class CharmDatabaseRelationNotFoundError(Exception): |
There was a problem hiding this comment.
Perhaps this would be better placed in database_client.py
| PEER_RELATION_NAME = "synapse-peers" | ||
| PROMETHEUS_TARGET_PORT = "9000" | ||
| # Disabling it since these are not hardcoded password | ||
| SECRET_ID = "secret-id" # nosec |
There was a problem hiding this comment.
Some of these also seem to be only used in mjoinir.py
| @@ -0,0 +1,76 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Are these needed on these files? I think they are only needed to make the file executable, which is required for the src/charm.py, I don't think they are needed here since the file isn't actually intended to be executed directly
There was a problem hiding this comment.
Similar comment applies for any other files with this line
| server: str = "", | ||
| admin_access_token: str = "", # nosec |
There was a problem hiding this comment.
What is the behaviour here when the default empty string is used please?
There was a problem hiding this comment.
A 'f"User {user.username} exists but there is no server/admin access token set."' will be raised. The token is used in case the user already exists so it will return the existing access token.
| def __init__(self, msg: str): | ||
| """Initialize a new instance of the ResetInstanceError exception. | ||
|
|
||
| Args: | ||
| msg (str): Explanation of the error. | ||
| """ | ||
| self.msg = msg |
There was a problem hiding this comment.
Since these are used in a few places perhaps there could be a BaseException for the entire charm which takes a message in the constructor and then the other exceptions derive from it and don't need to have this constructor defined
| db_client.erase() | ||
| synapse.execute_migrate_config(container=container, charm_state=charm_state) | ||
| except (psycopg2.Error, synapse.WorkloadError) as exc: | ||
| raise ResetInstanceError(str(exc)) from exc |
There was a problem hiding this comment.
Since from exc is used, the message from that exception is already preserved. Perhaps for these, and any similar other re-raises, it would be better to include a message here saying "something went wrong during the reset" or something similar based on the context of the function which will add a little more information to the stack trace to help resolve the issue
| require_nginx_route( | ||
| charm=self, | ||
| service_hostname=self.app.name, | ||
| service_name=self.app.name, | ||
| service_port=SYNAPSE_NGINX_PORT, | ||
| ) | ||
| self._ingress = IngressPerAppRequirer( | ||
| self, | ||
| port=SYNAPSE_NGINX_PORT, | ||
| # We're forced to use the app's service endpoint | ||
| # as the ingress per app interface currently always routes to the leader. | ||
| # https://github.com/canonical/traefik-k8s-operator/issues/159 | ||
| host=f"{self.app.name}-endpoints.{self.model.name}.svc.cluster.local", | ||
| strip_prefix=True, | ||
| ) |
There was a problem hiding this comment.
Looks like we're supporting both nginx_route and ingress?
There was a problem hiding this comment.
Yes, both are supported.
|
|
||
|
|
||
| class DatabaseClient: | ||
| """A class representing the Synapse application.""" |
There was a problem hiding this comment.
Is this docstring accurate? Based on the name it seems to represent a database rather than synapse?
| """A class representing the Synapse application.""" | ||
|
|
||
| def __init__( | ||
| self, datasource: typing.Optional[DatasourcePostgreSQL], alternative_database: str = "" |
There was a problem hiding this comment.
Do you think a None default would be better than an empty string? That way the type is a bit clearer and we can differentiate between the None case and someone accidentally passing an empty string.
Generally speaking empty strings as defaults should be avoided. Either None is good or even a sentinel value can help. Here is an example of a sentinel value: https://github.com/canonical/repo-policy-compliance/blob/fcdc9fb819b50ad3e874e8015d3b5e7134ebd60a/repo_policy_compliance/__init__.py#L15
| def __init__( | ||
| self, datasource: typing.Optional[DatasourcePostgreSQL], alternative_database: str = "" | ||
| ): | ||
| """Initialize a new instance of the Synapse class. |
There was a problem hiding this comment.
This also seems to refer to Synapse
| except psycopg2.Error as exc: | ||
| logger.exception("Failed to prepare database: %r", exc) | ||
| raise |
There was a problem hiding this comment.
For these public methods, should the exception be remapped? That way the caller of the database doesn't need to know the library being used to interact with the database reducing the coupling
There was a problem hiding this comment.
Similar comment applies for the other public methods
There was a problem hiding this comment.
Based on this discussion, it could be even removed. It was left only for logging reason:
#13 (comment)
I agree regarding the library but Synapse only supports sqlite3 (for SQLite, local file) or psycopg2 (for PostgreSQL).
| @property | ||
| def _pebble_service(self) -> typing.Any: | ||
| """Return instance of pebble service. | ||
|
|
||
| Returns: | ||
| instance of pebble service or none. | ||
| """ | ||
| return getattr(self._charm, "pebble_service", None) |
There was a problem hiding this comment.
This doesn't quite feel right yet, it would be better not to need this. I believe this comes from that pebble service requires the charm state, charm state requires information from the database and the database observer requires the pebble service.
From memory, one of the options was to instantiate the DatabaseRequires in charm.py and pass that to CharmState for it to retrieve the necessary data from it if the relation is defined and also pass DatabaseRequires as an argument to the initialiser of this class. That way CharmState doesn't depend on the database observer anymore and it is safe to pass CharmState as an argument to the database observer which then means that the pebble service doesn't need to keep track of charm state and can be passed as an argument to the database observer as well
There was a problem hiding this comment.
Actually, this is here because after getting information from the relation with Database, the configuration must be updated and the container replaned.
This way avoids a circular dependency.
The idea was gently offered by Yanks at the time :-) : https://github.com/canonical/jenkins-agent-k8s-operator/blob/d094425a33a30b0bd08d36849d949c555e2f1511/src/agent.py#L20
There was a problem hiding this comment.
I'm thinking of proposing a Protocol based Charm class that can be passed on so that such circular dependencies can be mitigated.
|
FYI there are existing stories for addressing:
|
There was a problem hiding this comment.
I'm wondering if it would be better to have a charm specific .gitignore file, things like *.so seems unnecessary.
| Set a new server_name before running this action. | ||
| Once a server_name is configured, you must start a new instance if you wish a different one. | ||
| This actions will erase all data and create a instance with the new server_name. |
There was a problem hiding this comment.
Out of curiosity, is there a reason for server_name? I'm wondering why a variable type name is being used instead of a regular server name.
Edit: I got it now, it's referring to the configuration option server_name, perhaps clarifying that might be a tiny bit more helpful :D
| - cargo | ||
| - rustc | ||
| charm-binary-python-packages: | ||
| - psycopg2-binary==2.9.7 |
There was a problem hiding this comment.
I'm wondering if this specific version pinning is intended
There was a problem hiding this comment.
Out of curiosity, if we turn this into a snap do we maintain that too? I'm asking to understand how much overhead that would increase :D
| parts: | ||
| add-user: | ||
| plugin: nil | ||
| overlay-script: | |
There was a problem hiding this comment.
daemon with the next available UID on that range, 584792.
From the rockcraft spec RK011 might be helpful as a reference here!
| Raises: | ||
| Error: something went wrong while connecting to the database. | ||
| """ | ||
| if self._conn is None or self._conn.closed != 0: |
There was a problem hiding this comment.
Perhaps returning in the not case might help reduce indentation here as well :D
| if self._conn is None or self._conn.closed != 0: | |
| if self._conn is not None and self._conn.closed == 0: | |
| return |
| @property | ||
| def _pebble_service(self) -> typing.Any: | ||
| """Return instance of pebble service. | ||
|
|
||
| Returns: | ||
| instance of pebble service or none. | ||
| """ | ||
| return getattr(self._charm, "pebble_service", None) |
There was a problem hiding this comment.
I'm thinking of proposing a Protocol based Charm class that can be passed on so that such circular dependencies can be mitigated.
| # The username is random because if the user exists, register_user will try to get the | ||
| # access_token. |
There was a problem hiding this comment.
I'm wondering if checking if the username admin already exists and then returning that user might be an enhancement in UX which matches the behavior with other charms. It might be difficult to do if someone does register with admin username though..
There was a problem hiding this comment.
Loki would be a good addition here :D!
There was a problem hiding this comment.
To be addressed in future story :)
| random password. | ||
| """ | ||
| choices = string.ascii_letters + string.digits | ||
| password = "".join([secrets.choice(choices) for i in range(16)]) |
There was a problem hiding this comment.
| password = "".join([secrets.choice(choices) for i in range(16)]) | |
| password = "".join(secrets.choice(choices) for _ in range(16)) |
The list wrapper can be removed since join takes an iterable :D
| relation_data = {} | ||
| relations = list(self._charm.model.relations[self._RELATION_NAME]) | ||
| relation_id = relations[0].id | ||
| for relation in relations: | ||
| relation_data[relation.id] = ( | ||
| {key: value for key, value in relation.data[relation.app].items() if key != "data"} | ||
| if relation.app | ||
| else {} | ||
| ) |
There was a problem hiding this comment.
It looks like this can be refactored to a dict comprehension which is easier to read than a for loop and has a better underlying implementation
Also, it looks like relation_id = relations[0].id could raise an exception if the relations list is empty
Also, it doesn't look like a list is needed - a tuple would work better here which is generally preferred if possible as it doesn't have the same overheads as a list
| registration_shared_secret: str, | ||
| user: User, | ||
| server: typing.Optional[str] = None, | ||
| admin_access_token: typing.Optional[str] = None, # nosec |
There was a problem hiding this comment.
Is this nosec still needed since it seems that it is being defaulted to None now?
| Returns: | ||
| Access token to be used by the user. | ||
| """ | ||
| # get nonce |
There was a problem hiding this comment.
Is this comment needed? The method get_nonce seems to be quite descriptive and is potentially enough to explain what is going on without a comment?
There was a problem hiding this comment.
similar comment for get_mac
| ) | ||
| except (requests.exceptions.JSONDecodeError, TypeError, KeyError) as exc: | ||
| logger.exception("Failed to decode access_token: %r. Received: %s", exc, res.text) | ||
| raise RegisterUserError(str(exc)) from exc |
There was a problem hiding this comment.
Similar comment as before, because from exc is used here, the error message in that exception will already be shown in the stack trace so the RegisterUserError could be passed a message which has a bit more context about what this module is trying to do
| raise RegisterUserError(str(exc)) from exc | ||
|
|
||
|
|
||
| def _generate_mac( |
There was a problem hiding this comment.
The general guidance is to list functions that are used by others before the function that is using them to make the code easier to read (i.e., the reader should encounter the definition of a function in a module before it is used for the first time)
| user: user to be used for requesting access token. | ||
| admin_access_token: server admin access token to be used. | ||
| charm_state: Instance of CharmState. |
There was a problem hiding this comment.
Is this description right? It looks like this function does something other than granting access tokens
| Returns: | ||
| Mjolnir configuration | ||
| """ | ||
| with open("templates/mjolnir_production.yaml", encoding="utf-8") as mjolnir_config_file: |
There was a problem hiding this comment.
What about using pathlib.Path.read_text? Pathlib is a more modern interface compared to open and you can get what is needed without a context manager
| config["homeserverUrl"] = SYNAPSE_URL | ||
| config["rawHomeserverUrl"] = SYNAPSE_URL | ||
| config["accessToken"] = access_token | ||
| config["managementRoom"] = room_id |
There was a problem hiding this comment.
Reading the method _get_mjolnir_config i wouldn't expect it to add to the configuration - just to retrieve and return it in its current form. This seems to be modifying the configuration by adding more elements to it?
There was a problem hiding this comment.
Agree, the method name should be different.
Related: #52
| CreateMjolnirConfigError: something went wrong creating mjolnir config. | ||
| """ | ||
| try: | ||
| config = _get_mjolnir_config(access_token, room_id) |
There was a problem hiding this comment.
Perhaps add the additional configuration in this function? Or just in-line _get_mjolnir_config since it seems to be pretty simple?
| current_yaml["listeners"] = updated_listeners | ||
| current_yaml["saml2_enabled"] = True | ||
| current_yaml["saml2_config"] = {} | ||
| current_yaml["saml2_config"]["sp_config"] = _create_pysaml2_config(charm_state) |
There was a problem hiding this comment.
It is somewhat confusing the read current_yaml being modified - at that point it isn't really current anymore, right? Perhaps consider renaming it to something else?
There was a problem hiding this comment.
The term relation is still used in this file. I think it should be replaced by integration.
I have seen the term used in several places in the source code. I think it should be replaced there as well.
|
|
||
| import ops | ||
|
|
||
| # pydantic is causing this no-name-in-module problem |
There was a problem hiding this comment.
There is a common problem with pylint and pydantic due to their use of c extensions. We can ignore most of pylint's problems with pydantic by using a .pylintrc with the option extension-pkg-whitelist=pydantic.
See also
- https://pylint.pycqa.org/en/2.3.0/technical_reference/c_extensions.html
- pylint: No name 'BaseModel' in module 'pydantic' (no-name-in-module) pydantic/pydantic#1961 (comment)
- Improve the situation for
no-name-in-modulefor C extensions and modules using dynamic features pylint-dev/pylint#1524 (comment) - https://github.com/canonical/github-runner-operator/blob/17b783a7e2f2d7e7da0f80ce20fad531fcf935a4/.pylintrc
|
|
||
| """Helper module used to manage interactions with Synapse API.""" | ||
|
|
||
| # pylint: disable=too-few-public-methods, too-many-arguments |
There was a problem hiding this comment.
It seems that too-few-public-methods is no longer needed. too-many-arguments only applies to _generate_mac, I would suggest moving it there.
| try: | ||
| nonce = res.json()["nonce"] | ||
| except (requests.exceptions.JSONDecodeError, TypeError, KeyError) as exc: | ||
| logger.exception("Failed to decode nonce: %r. Received: %s", exc, res.text) | ||
| raise GetNonceError(str(exc)) from exc |
There was a problem hiding this comment.
It seems the pattern
try:
x = res.json()["x"]
except (requests.exceptions.JSONDecodeError, TypeError, KeyError) as exc:
logger.exception("Failed to decode x: %r. Received: %s", exc, res.text)
is repeated multiple times in this module. Maybe introduce a private function _get_from_response(response, key, err_type) in order to avoid code duplication?
| exec_process = container.exec(command, environment=environment, working_dir=SYNAPSE_CONFIG_DIR) | ||
| try: | ||
| stdout, stderr = exec_process.wait_output() | ||
| return ExecResult(0, typing.cast(str, stdout), typing.cast(str, stderr)) |
There was a problem hiding this comment.
- I think the cast is not necessary for
stdout. - Can we always assume that
stderrwill be notNone?
| # ignoring duplicate-code with container connect check in the database observer. | ||
| # pylint: disable=R0801 |
There was a problem hiding this comment.
I think this should be moved to _enable_saml, as disabling should be done as specifically as possible (https://github.com/canonical/is-charms-contributing-guide/blob/main/CONTRIBUTING.md#static-code-analysis).
| - 'LICENSE' | ||
| - 'trivy.yaml' | ||
| - 'zap_rules.tsv' | ||
| - 'lib/**' |
There was a problem hiding this comment.
Perhaps we should generally agree on how to handle licences in libs, as @arturo-seijas has pointed out here https://chat.canonical.com/canonical/pl/6sr9h8uod3nimeddpc8dquxzao . In some charms we don't exclude and update the copyright header.
| Run the following command: | ||
|
|
||
| ```bash | ||
| echo -e "tox -e src-docs\ngit add src-docs\n" > .git/hooks/pre-commit |
There was a problem hiding this comment.
| echo -e "tox -e src-docs\ngit add src-docs\n" > .git/hooks/pre-commit | |
| echo -e "tox -e src-docs\ngit add src-docs\n" >> .git/hooks/pre-commit |
I generally prefer not to override existing hooks.
| harness = harness_with_postgresql | ||
| harness = harness_with_postgresql |
There was a problem hiding this comment.
| harness = harness_with_postgresql | |
| harness = harness_with_postgresql | |
| harness = harness_with_postgresql |
| username = "any-user" | ||
| user = User(username=username, admin=True) | ||
| # Prepare mock to get the access token | ||
| mock_response = mock.MagicMock() |
There was a problem hiding this comment.
| mock_response = mock.MagicMock() |
There was a problem hiding this comment.
Whats the change here please?
There was a problem hiding this comment.
mock_response is redefined in line 171, so this line is not needed.
|
The charm has changed a lot since this was opened. Closing as it’s no longer relevant. |
This is a PR to perform a charm review for the Synapse charm - no need to merge
Finished on 6th of October