Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 59 additions & 5 deletions common/common/mysql_shell/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
if typing.TYPE_CHECKING:
from ..relations import database_requires

_ROLE_DML = "charmed_dml"
_ROLE_READ = "charmed_read"
_ROLE_MAX_LENGTH = 32

logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -123,17 +127,67 @@ def _get_attributes(self, additional_attributes: dict = None) -> str:
attributes.update(additional_attributes)
return json.dumps(attributes)

def create_application_database_and_user(self, *, username: str, database: str) -> str:
"""Create database and user for related database_provides application."""
# TODO python3.10 min version: Use `set` instead of `typing.Set`
def _get_mysql_roles(self, name_pattern: str) -> typing.Set[str]:
"""Returns a set with the MySQL roles."""
logger.debug(f"Getting MySQL roles with {name_pattern=}")
output_file = self._container.path("/tmp/mysqlsh_output.json")
self._run_code(
_jinja_env.get_template("get_mysql_roles_with_pattern.py.jinja").render(
name_pattern=name_pattern,
output_filepath=output_file.relative_to_container,
)
)
with output_file.open("r") as file:
rows = json.load(file)
output_file.unlink()
logger.debug(f"MySQL roles found for {name_pattern=}: {len(rows)}")
return {row[0] for row in rows}

def _create_application_database(self, *, database: str, rolename: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def _create_application_database(self, *, database: str, rolename: str) -> str:
def _create_application_database(self, *, database: str, rolename: str):

nit; return value unused

"""Create database for related database_provides application."""
statements = [
f"CREATE DATABASE IF NOT EXISTS `{database}`",
f"CREATE ROLE IF NOT EXISTS `{rolename}`",
f"GRANT SELECT, INSERT, DELETE, UPDATE, EXECUTE ON `{database}`.* TO {rolename}",
f"GRANT ALTER, ALTER ROUTINE, CREATE, CREATE ROUTINE, CREATE VIEW, DROP, INDEX, LOCK TABLES, REFERENCES, TRIGGER ON `{database}`.* TO {rolename}",
]

mysql_roles = self._get_mysql_roles("charmed_%")
if _ROLE_READ in mysql_roles:
statements.append(
f"GRANT SELECT ON `{database}`.* TO {_ROLE_READ}",
)
if _ROLE_DML in mysql_roles:
statements.append(
f"GRANT SELECT, INSERT, DELETE, UPDATE ON `{database}`.* TO {_ROLE_DML}",
)

logger.debug(f"Creating {database=}")
self._run_sql(statements)
logger.debug(f"Created {database=}")
return database

def _create_application_user(self, *, database: str, username: str) -> str:
"""Create database user for related database_provides application."""
attributes = self._get_attributes()
logger.debug(f"Creating {database=} and {username=} with {attributes=}")
password = utils.generate_password()
logger.debug(f"Creating {username=} with {attributes=}")
self._run_sql([
f"CREATE DATABASE IF NOT EXISTS `{database}`",
f"CREATE USER `{username}` IDENTIFIED BY '{password}' ATTRIBUTE '{attributes}'",
f"GRANT ALL PRIVILEGES ON `{database}`.* TO `{username}`",
])
logger.debug(f"Created {database=} and {username=} with {attributes=}")
logger.debug(f"Created {username=} with {attributes=}")
return password

def create_application_database(self, *, database: str, username: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def create_application_database(self, *, database: str, username: str) -> str:
def create_application_database_and_user(self, *, database: str, username: str) -> str:

"""Create both the database and the relation user, returning its password."""
rolename = f"charmed_dba_{database}"
if len(rolename) >= _ROLE_MAX_LENGTH:
raise ValueError("Database DBA role longer than 32 characters")

________ = self._create_application_database(database=database, rolename=rolename)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
________ = self._create_application_database(database=database, rolename=rolename)
self._create_application_database(database=database, rolename=rolename)

nit

password = self._create_application_user(database=database, username=username)
return password

def add_attributes_to_mysql_router_user(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import json

result = session.run_sql(
"SELECT user FROM mysql.user WHERE user LIKE '{{ name_pattern }}'"
)
rows = result.fetch_all()
# mysqlsh objects are weird—they quack (i.e. duck typing) like standard Python objects (e.g. list,
# dict), but do not serialize to JSON correctly.
# Cast to str & load from JSON str before serializing
rows = json.loads(str(rows))
with open("{{ output_filepath }}", "w") as file:
json.dump(rows, file)
47 changes: 35 additions & 12 deletions common/common/relations/database_provides.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,25 @@ class _UnsupportedExtraUserRole(status_exception.StatusException):

def __init__(self, *, app_name: str, endpoint_name: str) -> None:
message = (
f"{app_name} app requested unsupported extra user role on {endpoint_name} endpoint"
f"{app_name} app requested unsupported extra user role on "
f"{endpoint_name} endpoint"
Comment on lines -33 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: why wrap? line length is 99 characters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make both exceptions have break lines in the same exact places. Easier to see where they differ (i.e. one includes the underlying exception message, while the other do not).

)
logger.warning(message)
super().__init__(ops.BlockedStatus(message))


class _InvalidDatabaseName(status_exception.StatusException):
"""Application charm requested an invalid database name"""

def __init__(self, *, app_name: str, endpoint_name: str, exception_msg: str) -> None:
message = (
f"{app_name} app requested an invalid database name on "
f"{endpoint_name} endpoint: {exception_msg}"
Comment on lines +45 to +46
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
f"{app_name} app requested an invalid database name on "
f"{endpoint_name} endpoint: {exception_msg}"
f"{app_name} app requested invalid database name >20 characters on "
f"{endpoint_name} endpoint"

to keep message short (under 120 characters) so it doesn't get truncated in juju status & to give user clear actionable next step (request a shorter database name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather have a distinction between the log message (long format), and the message displayed when doing juju status (short format). As it is proposed at the moment:

  • Log: {app} requested an invalid database name on {endpoint} endpoint: DBA role longer than 32 characters
  • Status: DBA role longer than 32 characters.

)
logger.warning(message)
super().__init__(ops.BlockedStatus(exception_msg))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
super().__init__(ops.BlockedStatus(exception_msg))
super().__init__(ops.BlockedStatus(message))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered in this comment.



class _Relation:
"""Relation to one application charm"""

Expand Down Expand Up @@ -79,9 +92,11 @@ def __init__(
if isinstance(event, ops.RelationBrokenEvent) and event.relation.id == self._id:
raise _RelationBreaking
self._database: str = self._databag["database"]
self._app_name: str = relation.app.name
self._endpoint_name: str = relation.name
if self._databag.get("extra-user-roles"):
raise _UnsupportedExtraUserRole(
app_name=relation.app.name, endpoint_name=relation.name
app_name=self._app_name, endpoint_name=self._endpoint_name
)
Comment on lines 97 to 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you raise here if database name > 20 characters so that the _RelationThatRequestedUser object does not get instantiated?

the current pattern/design is _RelationThatRequestedUser is only successfully initialized if the request is valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that approach too, to have the exception being raised in the __init__ method. I am on the fence whether that is a good separation of concerns or not, as it will require the _RelationThatRequestedUser class to know implementation details about the MySQLshell.create_application_database function.

Maybe exposing a new mysql_shell's MYSQL_DBA_ROLE_PREFIX constant with the charmed_dba_ value?


def _set_databag(
Expand Down Expand Up @@ -121,16 +136,24 @@ def create_database_and_user(
shell.delete_user(username, must_exist=False)
logger.debug("Deleted user if exists before creating user")

password = shell.create_application_database_and_user(
username=username, database=self._database
)

self._set_databag(
username=username,
password=password,
router_read_write_endpoints=router_read_write_endpoints,
router_read_only_endpoints=router_read_only_endpoints,
)
try:
password = shell.create_application_database(
database=self._database,
username=username,
)
except ValueError as exception:
raise _InvalidDatabaseName(
app_name=self._app_name,
endpoint_name=self._endpoint_name,
exception_msg=str(exception),
)
else:
self._set_databag(
username=username,
password=password,
router_read_write_endpoints=router_read_write_endpoints,
router_read_only_endpoints=router_read_only_endpoints,
)


class _UserNotShared(Exception):
Expand Down
1 change: 1 addition & 0 deletions kubernetes/tests/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ def patch(monkeypatch):
)
monkeypatch.setattr("common.workload.RunningWorkload._router_username", "")
monkeypatch.setattr("common.mysql_shell.Shell._run_code", lambda *args, **kwargs: None)
monkeypatch.setattr("common.mysql_shell.Shell._get_mysql_roles", lambda *args, **kwargs: set())
monkeypatch.setattr(
"common.mysql_shell.Shell.get_mysql_router_user_for_unit", lambda *args, **kwargs: None
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,9 @@ def create_database_and_user(
shell.delete_user(self._username, must_exist=False)
logger.debug("Deleted user if exists before creating user")

password = shell.create_application_database_and_user(
username=self._username, database=self._database
password = shell.create_application_database(
database=self._database,
username=self._username,
)
self._peer_app_databag[self.peer_databag_password_key] = password
self.set_databag(password=password)
Expand Down
1 change: 1 addition & 0 deletions machines/tests/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def patch(monkeypatch):
)
monkeypatch.setattr("common.workload.RunningWorkload._router_username", "")
monkeypatch.setattr("common.mysql_shell.Shell._run_code", lambda *args, **kwargs: None)
monkeypatch.setattr("common.mysql_shell.Shell._get_mysql_roles", lambda *args, **kwargs: set())
monkeypatch.setattr(
"common.mysql_shell.Shell.get_mysql_router_user_for_unit", lambda *args, **kwargs: None
)
Expand Down