[DPE-5307][DPE-5236] update tls certs on node port change + fix flakey TLS tests #35

MiaAltieri · 2024-09-09T14:23:47Z

Issue(s)

DPE-5236 update tls certs on node port change

TLS does not support updating certs on the addition of external connections
TLS does not support updating certs on the removal of external connections
TLS does not support updating certs on the change of k8s IP addresses

DPE-5307 fix flakey TLS tests

TLS tests have a flakey performance

Solution

update tls certs on node port change
Generate new CSRs when 1/2/3 happens
fix flakey TLS tests

update tests to work on K8s
update tests to work on routers

prevent unnessary hooks from firing + request only certificates that are needed

…perly

MiaAltieri · 2024-09-12T12:46:53Z

src/node_port.py

@@ -4,13 +4,14 @@

 """Manager for handling mongos Kubernetes resources for a single mongos pod."""

+from typing import Optional


if you have reviewed node port for external clients then these changes are the same

MiaAltieri · 2024-09-12T12:52:56Z

tests/integration/tls/helpers.py

@@ -97,45 +99,55 @@ async def toggle_tls_mongos(
            f"{certs_app_name}:{CERT_REL_NAME}",
        )

+    await ops_test.model.wait_for_idle(apps=[MONGOS_APP_NAME], idle_period=30)


necessary to fix flakey TLS tests

MiaAltieri · 2024-09-12T12:54:14Z

tests/integration/tls/helpers.py


    return (
-        f"{MONGO_SHELL} '{client_uri}'  --eval 'sh.status()'"
+        f"{MONGO_SHELL} '{client_uri}'  --eval 'db.getUsers()'"


necessary to fix flakey TLS tests. sh.status is a privileged command

MiaAltieri · 2024-09-12T12:57:31Z

tests/integration/tls/test_tls.py

@@ -58,9 +62,62 @@ async def test_mongos_tls_enabled(ops_test: OpsTest) -> None:
    )

    await integrate_cluster_with_tls(ops_test)
+    await ops_test.model.wait_for_idle(


necessary to fix flakey TLS tests

MiaAltieri · 2024-09-12T12:58:20Z

tests/integration/tls/test_tls.py

@@ -81,12 +137,15 @@ async def test_mongos_tls_disabled(ops_test: OpsTest) -> None:
        timeout=300,
    )

+    await check_mongos_tls_disabled(ops_test)


necessary to fix flakey TLS tests - shouldn't check if disabled until model is stable

lib/charms/mongodb/v1/mongodb_tls.py

src/charm.py

marcoppenheimer · 2024-09-13T19:02:52Z

src/charm.py

+    def get_k8s_mongos_hosts(self) -> Set:
+        """Returns the K8s hosts for mongos"""
+        hosts = set()
+        for unit in self.get_units():
+            hosts.add(self.get_k8s_mongos_host(unit))
+
+        return hosts
+
+    def get_ext_mongos_host(self, unit: Unit, incl_port=True) -> str | None:
+        """Returns the ext hosts for mongos on the provided unit."""
+        if not self.is_external_client:
+            return None
+
+        try:
+            unit_ip = self.node_port_manager.get_node_ip(unit.name)
+            if not incl_port:
+                return unit_ip
+
+            unit_port = self.node_port_manager.get_node_port(
+                port_to_match=Config.MONGOS_PORT, unit_name=unit.name
+            )
+            if unit_ip and unit_port:
+                return "{unit_ip}:{unit_port}"
+            else:
+                raise NoExternalHostError(f"No external host for unit {unit.name}")
+        except (
+            NoExternalHostError,
+            FailedToFindNodePortError,
+            FailedToFindServiceError,
+        ) as e:
+            raise FailedToGetHostsError(
+                "Failed to retrieve external hosts due to %s", e
+            )


todo: I strongly recommend you sort() your hosts.

Off the top of my head, I see that you use these hosts in the mongos_config. Not sure what you do with this, but let's assume you use them in some env-var during service runtime:

HOSTS=host1,host2,host3 start-mongos.sh

Pebble replan looks for new runtime args, so if you now get:

HOSTS=host3,host1,host2 start-mongos.sh

you're going to restart without needing to.

fortunately we do not provide hosts to pebble plan! But I suppose we do supply them to clients, I suppose we could lead to unnecessary relation-changed events. However, we use Sets for hosts since we do many set operations on our hosts in different handlers and python sets make no gaurentee on ordering so I am not sure this suggestion is feasible?

I didn't look deeply, but if you do:

hosts = set(["1", "2", "3"]) relation.data[some.app].update({"hosts": ",".join(hosts)})

You'll get a random order, and have extraneous relation-changed events.
Well, strictly speaking you won't have random ordering WITHIN an event, but you will for a different event:

hosts = set(["1", "2", "3"]) print(",".join(hosts)) hosts = set(["3", "1", "2"]) print(",".join(hosts))

Because the sets in the above example are assigned to the same memory address, they 'coincidentally' end up evaluating the same due to a quirk of python's collision resolution (essentially it's the same stuff in the set, so just returns the original set).

But if you re-run it in a different Python execution (aka charm event), the order will now be different.

I see , thank you very much @marcoppenheimer ! I think we ought to handle this in where we update hosts (the provider lib) which is on the mongodb-vm operator GitHub, I added an issue for it

src/charm.py

src/node_port.py

src/charm.py

marcoppenheimer

Approving as it all looks kosher. Left a minor comment about the pebble restart part, but great job!

marcoppenheimer · 2024-10-03T00:07:03Z

src/charm.py

        container.replan()
+        container.restart(Config.SERVICE_NAME)


note: This will cause a double restart, is that what you want?

If the new layer is not the same as the original, pebble replan --> pebble stop; pebble start. Then you do pebble restart again. It's probably not the end of the world, but you might want to just have container.restart here.

MiaAltieri added 10 commits September 6, 2024 09:39

mongos k8s updates tls certs based on external node port

8cc9564

tls is properly updated for nodeport now

1e74374

re-request TLS certs on IP change

fee56a8

add int tests to verify node port with TLS is functional

e1fa06b

Finish TLS work for nodeport

232a68c

prevent unnessary hooks from firing + request only certificates that are needed

tests for tls and nodeport functional

6f3fb2a

fmt + lint

023cc2a

update file location

2d753d8

minor tweaks

258db0f

fix tests for tls

f4f8642

MiaAltieri changed the title ~~Dpe 5236 update tls certs on node port change~~ WIP - please do not reivew Dpe 5236 update tls certs on node port change Sep 9, 2024

MiaAltieri marked this pull request as draft September 9, 2024 14:24

MiaAltieri added 4 commits September 11, 2024 09:56

external config based on model config

c33294f

update lib

cd23d72

fmt + lint of src and update requesting of sans

6e00451

fmt+lint tests + update tests now that username / password is set pro…

3aee80a

…perly

MiaAltieri changed the title ~~WIP - please do not reivew Dpe 5236 update tls certs on node port change~~ [DEP-5236] update tls certs on node port change Sep 12, 2024

MiaAltieri changed the title ~~[DEP-5236] update tls certs on node port change~~ [DPE-5236] update tls certs on node port change Sep 12, 2024

update node port

86b7310

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch from 25f4489 to 86b7310 Compare September 12, 2024 12:42

MiaAltieri changed the title ~~[DPE-5236] update tls certs on node port change~~ [DPE-5307][DPE-5236] update tls certs on node port change + fix flakey TLS tests Sep 12, 2024

MiaAltieri commented Sep 12, 2024

View reviewed changes

personal nits

f3a0228

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch from 55d3fdd to f3a0228 Compare September 12, 2024 12:59

MiaAltieri marked this pull request as ready for review September 12, 2024 13:03

update lib

8938c34

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch from 722bf01 to 8938c34 Compare September 12, 2024 14:06

MiaAltieri requested a review from Mehdi-Bendriss September 13, 2024 08:52

marcoppenheimer reviewed Sep 13, 2024

View reviewed changes

update lib

74f5941

pr feedback

4d31669

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch from 86d8690 to 4d31669 Compare September 17, 2024 12:27

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch from 4727f5c to 4d31669 Compare September 26, 2024 15:52

add gaurdrails to tests

da65ec3

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch 3 times, most recently from e4975cd to 938704c Compare September 27, 2024 16:48

Merge branch '6/edge' into DPE-5236-update-tls-certs-on-node-port-change

1f63b30

MiaAltieri force-pushed the DPE-5236-update-tls-certs-on-node-port-change branch from 938704c to 1f63b30 Compare September 27, 2024 17:01

marcoppenheimer approved these changes Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-5307][DPE-5236] update tls certs on node port change + fix flakey TLS tests #35

[DPE-5307][DPE-5236] update tls certs on node port change + fix flakey TLS tests #35

MiaAltieri commented Sep 9, 2024 •

edited

Loading

MiaAltieri Sep 12, 2024

MiaAltieri Sep 12, 2024

MiaAltieri Sep 12, 2024

MiaAltieri Sep 12, 2024

MiaAltieri Sep 12, 2024

marcoppenheimer Sep 13, 2024

MiaAltieri Sep 16, 2024 •

edited

Loading

marcoppenheimer Sep 16, 2024 •

edited

Loading

MiaAltieri Sep 26, 2024

marcoppenheimer left a comment

marcoppenheimer Oct 3, 2024

		@@ -4,13 +4,14 @@

		"""Manager for handling mongos Kubernetes resources for a single mongos pod."""

		from typing import Optional

[DPE-5307][DPE-5236] update tls certs on node port change + fix flakey TLS tests #35

Are you sure you want to change the base?

[DPE-5307][DPE-5236] update tls certs on node port change + fix flakey TLS tests #35

Conversation

MiaAltieri commented Sep 9, 2024 • edited Loading

Issue(s)

Solution

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

MiaAltieri Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

marcoppenheimer Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoppenheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiaAltieri commented Sep 9, 2024 •

edited

Loading

MiaAltieri Sep 16, 2024 •

edited

Loading

marcoppenheimer Sep 16, 2024 •

edited

Loading