[DPE-3689, DPE-4179] Expose read-write and read-only endpoints when related to data-integrator + TLS support #119

shayancanonical · 2024-04-04T20:25:57Z

Issue

We are currently not reacting to external-node-connectivity that may be sent to mysql-router via data-interfaces when related with data-integrator
We are not able to relate with the TLS operators
We broke upgrades (Upgrades broken #130) when we implemented reconcile pattern for COS

Solution

Support external-node-connectivity by binding to address 0.0.0.0 when related to data-integrator. Else, only expose router through a unix socket
Add capability to relate to TLS operators
Refactor to fix upgrades -- remaining integration test will be added in a follow up PR

Testing

Added integration tests for TLS + COS, TLS, and TLS + Data Integrator

todo

~~Change the charmed-mysql snap version to the official 8.0/edge version instead of a branch build~~

…ntegrator

…al node connectivity

…e in data integrator test

taurus-forever

LGTM, some questions inside.
BTW: are we missing CA chain support on mysql-router? See canonical/mysql-operator#396 + canonical/mysql-k8s-operator#383

.github/workflows/ci.yaml

pyproject.toml

taurus-forever · 2024-04-12T12:42:53Z

src/abstract_charm.py

+                    if self.is_exposed():
+                        for port in (6446, 6447):
+                            with socket.socket() as s:
+                                assert s.connect_ex(("localhost", port)) == 0
+                    else:
+                        assert self._container.path("/run/mysqlrouter/mysql.sock").exists()
+                        assert self._container.path("/run/mysqlrouter/mysqlro.sock").exists()


Just to be in sync. When m-r related to data-integrator, users should expose data-integrator and not m-r.

@taurus-forever how will data-integrator know which ports to expose?

I agree with @carlcsaposs-canonical. Isn't it ideal that mysqlrouter exposes its own ports and then unexpose when the relation with data integrator is removed? Since they are on the same machine, it is easiest if router exposes it own ports

src/machine_charm.py

paulomach

Like the tests. Minor comments

src/snap.py

src/relations/database_provides.py

src/machine_charm.py

pyproject.toml

.github/workflows/ci.yaml

poetry.lock

src/relations/tls.py

carlcsaposs-canonical · 2024-04-15T10:18:28Z

src/relations/database_provides.py

+            [data.get("external-node-connectivity") == "true" for data in relation_data.values()]
+        )
+
+    def reconcile_ports(self) -> None:


IMO, this doesn't belong in database_provides since it's not part of the database_provides relation

also, this should maybe be merged with https://github.com/canonical/mysql-router-k8s-operator/blob/481b23de75a1662210201294ab1efbd952fe4bd0/src/kubernetes_charm.py#L250-L252
also, do ports 6448 and 6449 need to be opened?

imo ports 6448 and 6449 are not going to be used externally (they're related to X protocol) and so should not be exposed

removed from database_provides in ee4b098

https://chat.canonical.com/canonical/pl/6gmu9q65q3bntdoowet5gw8bgr

src/relations/tls.py

src/machine_workload.py

carlcsaposs-canonical · 2024-04-15T10:25:56Z

src/abstract_charm.py

+                    if self.is_exposed():
+                        for port in (6446, 6447):
+                            with socket.socket() as s:
+                                assert s.connect_ex(("localhost", port)) == 0
+                    else:
+                        assert self._container.path("/run/mysqlrouter/mysql.sock").exists()
+                        assert self._container.path("/run/mysqlrouter/mysqlro.sock").exists()


@taurus-forever how will data-integrator know which ports to expose?

pyproject.toml

carlcsaposs-canonical · 2024-04-19T10:23:16Z

src/relations/database_provides.py

+            [data.get("external-node-connectivity") == "true" for data in relation_data.values()]
+        )
+
+    def reconcile_ports(self) -> None:


https://chat.canonical.com/canonical/pl/6gmu9q65q3bntdoowet5gw8bgr

carlcsaposs-canonical · 2024-04-19T10:26:18Z

poetry.lock

@@ -989,19 +1073,22 @@ signedtoken = ["cryptography (>=3.0.0)", "pyjwt (>=2.0.0,<3)"]

 [[package]]
 name = "ops"
-version = "2.6.0"
+version = "2.12.0"


ops is 2.12, not 2.8

#119 (comment)

updated to 2.12.0 in 0cf8b71

was canonical/operator#1091 (comment) addressed? from comment linked above

if not, please revert to <2.10

reverted to < 2.10.0 in 0a36768. the linked comment seems like a separate issue than the goal of this PR and should be addressed separately

pyproject.toml

src/abstract_charm.py

src/machine_charm.py

src/relations/database_provides.py

src/workload.py

…me.sleep()

carlcsaposs-canonical

overall looks great! few minor potential issues

pyproject.toml

carlcsaposs-canonical · 2024-04-23T11:33:56Z

src/abstract_charm.py

+    @abc.abstractmethod
    def wait_until_mysql_router_ready(self) -> None:
        """Wait until a connection to MySQL Router is possible.

        Retry every 5 seconds for up to 30 seconds.
        """
-        logger.debug("Waiting until MySQL Router is ready")
-        self.unit.status = ops.MaintenanceStatus("MySQL Router starting")
-        try:
-            for attempt in tenacity.Retrying(
-                reraise=True,
-                stop=tenacity.stop_after_delay(30),
-                wait=tenacity.wait_fixed(5),
-            ):
-                with attempt:
-                    for port in (6446, 6447):
-                        with socket.socket() as s:
-                            assert s.connect_ex(("localhost", port)) == 0
-        except AssertionError:
-            logger.exception("Unable to connect to MySQL Router")
-            raise
-        else:
-            logger.debug("MySQL Router is ready")


nit: perhaps leave this as concrete implementation (if can be used in k8s)

and then override in vm with super() call if exposed

i'd rather have them be methods in the subclass if they are separate in implementation (the vm charm needs to use sockets to test if the charm is not exposed)

the check is identical to k8s if the charm is exposed, right?

then you can do something like this

# abstract_charm.py logger.debug("Waiting until MySQL Router is ready") self.unit.status = ops.MaintenanceStatus("MySQL Router starting") try: for attempt in tenacity.Retrying( reraise=True, stop=tenacity.stop_after_delay(30), wait=tenacity.wait_fixed(5), ): with attempt: for port in (6446, 6447): with socket.socket() as s: assert s.connect_ex(("localhost", port)) == 0 except AssertionError: logger.exception("Unable to connect to MySQL Router") raise else: logger.debug("MySQL Router is ready")

# machine_charm.py if self.is_exposed(): super().wait_until_mysql_router_ready() else:

will address in a followup

src/abstract_charm.py

src/snap.py

carlcsaposs-canonical · 2024-04-23T11:46:13Z

src/workload.py

-        if not self._container.mysql_router_service_enabled:
+        is_charm_exposed = self._charm.is_exposed
+        socket_file_exists = self._container.path("/run/mysqlrouter/mysql.sock").exists()
+        require_rebootstrap = is_charm_exposed == socket_file_exists


really nice!

src/workload.py

carlcsaposs-canonical · 2024-04-23T11:49:58Z

src/machine_workload.py

+        if not self._charm.is_exposed:
+            self._update_configured_socket_file_locations_and_bind_address()


could this affect future re-bootstraps if charm is bootstrapped with is_exposed = False and then with is_exposed = True?

see other comment about deleting files before re-bootstrap

carlcsaposs-canonical · 2024-04-23T11:50:04Z

src/relations/database_provides.py

+        relation_data = self._interface.fetch_relation_data(fields=["external-node-connectivity"])
+        return any(
+            [data.get("external-node-connectivity") == "true" for data in relation_data.values()]
+        )


please sync with k8s code

synced in 1b0bb19

pyproject.toml

src/abstract_charm.py

src/snap.py

carlcsaposs-canonical · 2024-04-24T12:23:06Z

src/workload.py

+    def _enable_router(self, *, tls: bool, unit_name: str) -> None:
+        """Enable router after setting up all the necessary prerequisites."""
+        logger.debug("Enabling MySQL Router service")
+        self._cleanup_after_upgrade_or_potential_container_restart()
+        # create an empty credentials file, if the file does not exist
+        self._container.create_router_rest_api_credentials_file()
+        self._bootstrap_router(tls=tls)
+        self.shell.add_attributes_to_mysql_router_user(
+            username=self._router_username, router_id=self._router_id, unit_name=unit_name
+        )
+        self._container.update_mysql_router_service(enabled=True, tls=tls)
+        self._logrotate.enable()
+        logger.debug("Enabled MySQL Router service")
+        self._charm.wait_until_mysql_router_ready()
+
+    def _enable_exporter(
+        self, *, tls: bool, exporter_config: "relations.cos.ExporterConfig"
+    ) -> None:
+        """Enable the mysqlrouter exporter."""
+        logger.debug("Enabling MySQL Router exporter service")
+        self.setup_monitoring_user()
+        self._container.update_mysql_router_exporter_service(
+            enabled=True,
+            config=exporter_config,
+            tls=tls,
+            key_filename=str(self._tls_key_file),
+            certificate_filename=str(self._tls_certificate_file),
+            certificate_authority_filename=str(self._tls_certificate_authority_file),
+        )
+        logger.debug("Enabled MySQL Router exporter service")


question: could this be simplified by moving back into reconcile?

and then reconcile would look like

if require reboostrap: disable if router not enabled: enable if exporter not enabled: enabled

and upgrade would look like

disable self.reconcile()

i think that this simplification should be handled separately (if at all) -- likely when we add integration tests for upgrade (which i hope to add as soon as this PR is merged)

could you add this in a follow-up PR? I think moving back into reconcile will make the code easier to maintain, specifically it alleviates concerns with _enable_router getting called without "all the necessary prerequisites"

yes, will address in a followup

carlcsaposs-canonical · 2024-04-24T12:24:56Z

src/abstract_charm.py

+    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
+        """Whether router is externally accessible"""


question: is this only used by vm charm?

yes, it is only used by the vm charm. but needs to be defined in abstract_charm because reconcile in workload calls to it. in vm, it will return True/False, in k8s it will return None

could you mention that it's only used by machine charm in docstring?

done in 0978187

src/machine_charm.py

src/machine_logrotate.py

carlcsaposs-canonical · 2024-04-24T14:51:25Z

src/abstract_charm.py

+        """The exposed read-only endpoint"""
+
+    @abc.abstractmethod
+    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:


nit: please remove default None since event should always be passed (even if in reconcile, the value of event is none because it's not a relation-breaking event, that value should always be passed here—this function should never be called without an event arg I believe)

addressed in 0978187

carlcsaposs-canonical · 2024-04-24T14:52:24Z

src/machine_charm.py

+    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
+        return self._database_provides.external_connectivity(event)


same here with event should always be passed

addressed in 0978187

carlcsaposs-canonical · 2024-04-24T14:52:46Z

src/machine_charm.py

+                wait=tenacity.wait_fixed(5),
+            ):
+                with attempt:
+                    if self.is_externally_accessible():


does event need to be passed here?

addressed in 0978187

carlcsaposs-canonical · 2024-04-24T14:54:18Z

src/workload.py

+    def _enable_router(self, *, tls: bool, unit_name: str) -> None:
+        """Enable router after setting up all the necessary prerequisites."""
+        logger.debug("Enabling MySQL Router service")
+        self._cleanup_after_upgrade_or_potential_container_restart()
+        # create an empty credentials file, if the file does not exist
+        self._container.create_router_rest_api_credentials_file()
+        self._bootstrap_router(tls=tls)
+        self.shell.add_attributes_to_mysql_router_user(
+            username=self._router_username, router_id=self._router_id, unit_name=unit_name
+        )
+        self._container.update_mysql_router_service(enabled=True, tls=tls)
+        self._logrotate.enable()
+        logger.debug("Enabled MySQL Router service")
+        self._charm.wait_until_mysql_router_ready()
+
+    def _enable_exporter(
+        self, *, tls: bool, exporter_config: "relations.cos.ExporterConfig"
+    ) -> None:
+        """Enable the mysqlrouter exporter."""
+        logger.debug("Enabling MySQL Router exporter service")
+        self.setup_monitoring_user()
+        self._container.update_mysql_router_exporter_service(
+            enabled=True,
+            config=exporter_config,
+            tls=tls,
+            key_filename=str(self._tls_key_file),
+            certificate_filename=str(self._tls_certificate_file),
+            certificate_authority_filename=str(self._tls_certificate_authority_file),
+        )
+        logger.debug("Enabled MySQL Router exporter service")


could you add this in a follow-up PR? I think moving back into reconcile will make the code easier to maintain, specifically it alleviates concerns with _enable_router getting called without "all the necessary prerequisites"

carlcsaposs-canonical · 2024-04-24T14:55:45Z

only blocking comment is #119 (comment); otherwise approved

Addressed all pending feedback and also received Carl's written approval

shayancanonical added 14 commits April 1, 2024 17:14

WIP: Expose read-write and read-only endpoints when related to data-i…

0e93ef5

…ntegrator

Merge branch 'main' into feature/external_connectivity

b4f85d0

A working version of charm with simultaneous relations with COS and TLS

ad8b129

Avoid using kw_only in dataclasses due to python3.8 in juju 3.1.7

6dae91c

Add TLS integration test + test COS when related to TLS operator

716da08

Add integration test for data integrator and tls operator; for extern…

8293c94

…al node connectivity

Skip data integrator tests on focal

5b3627b

Run format

56c0c94

Use latest/stable for data integrator

61e2c8c

Avoid running data integration tests when focal only

6b47d09

Attempt at correcting ci.yaml invalid workflow file

eddf296

Use matrix exclude instead of if conditions

5fa3bc7

Remove quotes from exclusion group

26235fd

Another attempt at matrix exclusion

3657e83

shayancanonical requested review from paulomach, taurus-forever and carlcsaposs-canonical April 12, 2024 12:14

shayancanonical added 3 commits April 12, 2024 12:20

Use juju_.has_secrets to be correctly handle action result return cod…

b7afd29

…e in data integrator test

Fix mixup in return code keys across juju versions

d9c072c

Fix typo

79fd8ef

taurus-forever approved these changes Apr 12, 2024

View reviewed changes

paulomach approved these changes Apr 12, 2024

View reviewed changes

src/snap.py Show resolved Hide resolved

src/relations/database_provides.py Outdated Show resolved Hide resolved

src/machine_charm.py Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

carlcsaposs-canonical reviewed Apr 15, 2024

View reviewed changes

shayancanonical added 4 commits April 17, 2024 20:11

Address PR feedback

ee4b098

Add missing monkeypatch method for unit tests

6f4b271

Avoid re-bootstrapping in workload's reconcile method

eba3c8f

Update charmed-mysql snap revision to latest 8.0/edge revision

2e276ba

juditnovak mentioned this pull request Apr 18, 2024

[POC][DPE-4034] Feature/external connectivity data interfaces v34 #127

Closed

carlcsaposs-canonical previously requested changes Apr 19, 2024

View reviewed changes

carlcsaposs-canonical reviewed Apr 19, 2024

View reviewed changes

Address PR feedback

0cf8b71

shayancanonical requested a review from carlcsaposs-canonical April 19, 2024 20:47

shayancanonical added 5 commits April 22, 2024 12:43

Extend wait time in exporter tests + revert ops dependency to <2.10.0

0a36768

Use tenacity to retry checking exporter endpoints instead of using ti…

1134e43

…me.sleep()

Run format

41d7616

Update data_interfaces charm lib to v0.34

6f60488

Miscellaneous integration test improvements

40beb97

carlcsaposs-canonical reviewed Apr 23, 2024

View reviewed changes

Address PR feedback + fix broken upgrades

1b0bb19

shayancanonical changed the title ~~[DPE-3689] Expose read-write and read-only endpoints when related to data-integrator + TLS support~~ [DPE-3689, DPE-4179] Expose read-write and read-only endpoints when related to data-integrator + TLS support Apr 24, 2024

shayancanonical added 2 commits April 24, 2024 12:03

Merge branch 'main' into feature/external_connectivity

996ba1e

Merge branch 'main' into feature/external_connectivity

b99ecce

carlcsaposs-canonical reviewed Apr 24, 2024

View reviewed changes

shayancanonical added 2 commits April 24, 2024 13:41

Address PR feedback

10748cf

Fix assertion message in integration test

03d4ed4

carlcsaposs-canonical reviewed Apr 24, 2024

View reviewed changes

shayancanonical added 2 commits April 24, 2024 15:29

Address PR feedback

0978187

Fix bugs introduced while addressing PR feedback; pass event as kwarg

f8828dc

shayancanonical merged commit cecb229 into main Apr 24, 2024
30 checks passed

shayancanonical deleted the feature/external_connectivity branch April 24, 2024 19:45

carlcsaposs-canonical mentioned this pull request Apr 26, 2024

upgrade_unit does match signature of abstract method #139

Closed

shayancanonical mentioned this pull request May 22, 2024

Upgrades broken #130

Closed

		if not self._charm.is_exposed:
		self._update_configured_socket_file_locations_and_bind_address()

		def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
		"""Whether router is externally accessible"""

		def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
		return self._database_provides.external_connectivity(event)

[DPE-3689, DPE-4179] Expose read-write and read-only endpoints when related to data-integrator + TLS support #119

[DPE-3689, DPE-4179] Expose read-write and read-only endpoints when related to data-integrator + TLS support #119

Conversation

shayancanonical commented Apr 4, 2024 • edited Loading

Issue

Solution

Testing

todo

taurus-forever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulomach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlcsaposs-canonical commented Apr 24, 2024

shayancanonical commented Apr 4, 2024 •

edited

Loading