[DPE-3543] Zookeeper on Juju secrets (peer and for cross-charm relations #112

juditnovak · 2024-02-04T18:31:35Z

Zookeeper on Juju Secrets both for Peer and Cross-charms Relations.

NOTE: This PR relies on small data-platform-libs updates that are still to be merged: canonical/data-platform-libs#138

However I think the conten here is certainly ready for further discussions (while the above gets merged)

juditnovak · 2024-02-07T20:23:59Z

src/charm.py

+            PEER,
+            self.state.cluster.relation.id,
+            None,  # type:ignore noqa  -- Changes with the soon upcoming new version of DP-libs STILL within this POC
+        ):


NOTE: The # type: ignore should not be needed anymore once canonical/data-platform-libs#137 is merged (may take a while 😅 )

Calling ain INTERNAL function is CLEARLY wrong, should be replaced with a public call once canonical/data-platform-libs#124 is finally addressed.

(It's a small thing I just never gt to it...)

juditnovak · 2024-02-07T20:37:27Z

src/core/models.py

+        self,
+        relation: Relation | None,
+        data_interface: DataRelation,
+        component: Unit | Application,


Gotta check: do we really need component still...?

Yeah I think so. In ZKServer which inherits from that, we use component in deciding hosts.

marcoppenheimer · 2024-02-07T21:12:55Z

src/charm.py

+            peer_app_secrets=["sync-password", "super-password"],
+            peer_unit_secrets=[
+                "ca-cert",
+                "csr",
+                "certificate",
+                "truststore-password",
+                "keystore-password",
+                "private-key",
+            ],


todo: Could we move these to src/literals.py?

Can be two lists, PEER_APP_SECRETS and PEER_UNIT_SECRETS

Or maybe as class constants within ZKCluster and ZKServer respectively?

class ZKCluster: SECRETS = ["sync-password", "super-password"]

Yes, I should have done that, my bad.
In fact that's even how I'm advertising this method in the spec 😅

Fixing.

marcoppenheimer · 2024-02-07T21:22:58Z

src/charm.py

+        if not event.secret.label:
+            return


question: Under what conditions does this happen?

If your charm at any point just creates a secret w/o label. Or if a related charm may share (secret.grant()) a secret with your charm.

Thus your charm is an observer of that secret that doesn't have a label -- and gets secret-changed event notification once that secret changes.

We don't care about any of those potential secrets.

NOTE: label is an OPTIONAL "sticker" we stick on secrets, so we have a way to identify them. As without, secret identification becomes... complicated.

I'm assuming that the labelling of secrets is something we handle in our lib then?

marcoppenheimer · 2024-02-07T21:38:15Z

.github/workflows/ci.yaml

@@ -47,7 +47,7 @@ jobs:

  integration-test:
    strategy:
-      max-parallel: 1
+      # max-parallel: 1


suggestion/praise: Thank you for removing this! If the CI has been behaving, I'm 100% fine with removing max-parallel entirely! Saves us a lot of hassle.

marcoppenheimer · 2024-02-07T21:39:03Z

src/charm.py

+        if not self.state.cluster.relation:
+            return
+
+        if event.secret.label == self.state.cluster.data_interface._generate_secret_label(


question: What does this actually check? Ignorantly, to me it just seems like it's checking if a string == string.

I ask that, because generally we're trying to funnel every event in to cluster-relation-changed. To me, there isn't a distinction between a 'secret-changed' and a 'relation-changed', conceptually I mean.

Am I fair in guessing that we can still observe secret-changed, and then call the relation-changed handler as normal?

We check which secret changed.

Precisely since we don't actually care on a service level, if the unit TLS secrets changed. While we DO care on a service level whether the sync-password changed -- we need to cascade that down to the Zookeeper application.

So, while primarily you are right -- it's "just a relation-changed thing", we still need to have extra, secrets-specific checks on this occasion.

I'm wondering that it may rather worth to group functionalities that may happen both on secret changes AND on databag changes into an update_service function, which is called both in relation-changed and in secret-changed.

Does the response make sense...?

OK, can we:

Comment the if not event.secret.label to explain what it's for.

Comment the if event.secret.label == self.state.cluster.data_interface._generate_secret_label() line to explain why it's only app secrets we care about

Comment the docstring for the _on_secret_changed explaining why we handle this specifically (i.e the slow request time for secret content)

Replace the if: below to just call self._on_cluster_relation_changed(event)

marcoppenheimer · 2024-02-07T21:41:47Z

src/core/cluster.py

+class ClusterStateBase(Object):
+    """Collection of global cluster state for Framework/Object."""


nitpick: If this is just a Base, I think we can put it in src/core/models.py

marcoppenheimer · 2024-02-07T21:43:19Z

src/core/cluster.py

        )

+    @property
+    def peer_units_data_interfaces(self) -> Dict[Unit, DataPeerOtherUnit]:


nitpick/chore: Use dict[Unit, DataPeerOtherUnit]. Dict is deprecated.

marcoppenheimer · 2024-02-07T21:44:39Z

src/core/cluster.py

@@ -80,6 +114,7 @@ def clients(self) -> Set[ZKClient]:
            clients.add(
                ZKClient(
                    relation=relation,
+                    data_interface=self.charm.client_provider_interface,


praise: Good god I'm pleased it was that smooth! Great work!

marcoppenheimer · 2024-02-07T21:45:34Z

src/core/models.py

+        self,
+        relation: Relation | None,
+        data_interface: DataRelation,
+        component: Unit | Application,


Yeah I think so. In ZKServer which inherits from that, we use component in deciding hosts.

src/core/models.py

marcoppenheimer · 2024-02-07T21:56:09Z

src/core/models.py

            return

-        self.relation_data.update(items)
+        delete_fields = [key for key in items if not items[key]]


question: Maybe I'm being dim, but wouldn't this line ALWAYS return an empty list?

You're looping over a dict, which loops over keys. You're then keeping keys, that aren't keys in the dict, which they all are, because that's the loop value.

This is the point:

In [4]: items = {'a': '', 'b': "something"} In [5]: [key for key in items if not items[key]] Out[5]: ['a']

Using update() (but it's the same for set_secret() in old-style charms) the way we delete is to update() with "".

That's exactly what this list comprehension is distinguishing for.

Ah, right got it.

May I suggest making that a tad more explicit?:

[for key:value in items.items() if value]

deusebio · 2024-02-08T15:19:36Z

src/core/models.py

@@ -322,7 +363,7 @@ def certificate(self) -> str:
    @property
    def ca(self) -> str:
        """The root CA contents for the unit to use for TLS."""
-        return self.relation_data.get("ca", "")
+        return self.relation_data.get("ca-cert", "")


major We need to make sure this is backward compatible or we update the field during upgrade, otherwise after upgrade, this won't work properly

IMPORTANT
HIGH PRIOROTY
(Note to myself)

juditnovak · 2024-02-09T13:33:22Z

major check note-to-myself Though pipelines confirm external relations functionality, I want to follow step-by-step that there's zero interference regarding cross-charm relations protocols (ZK event handlers vs. DP-libs event handlers)

juditnovak · 2024-05-03T20:09:42Z

Early version of #129

juditnovak force-pushed the poc_judit_dp_libs_experiment branch 11 times, most recently from 7bcac6c to 6be5d8c Compare February 5, 2024 10:49

juditnovak added 2 commits February 5, 2024 12:13

data-platform-libs in zookeeper

8ef3b26

Unittests

c9c35e9

juditnovak force-pushed the poc_judit_dp_libs_experiment branch from 6be5d8c to 25837d8 Compare February 5, 2024 11:14

juditnovak added 2 commits February 5, 2024 13:05

Integration tests

e1d28b8

[REMOVE] Max parallel execution on CI increased

c71086f

juditnovak force-pushed the poc_judit_dp_libs_experiment branch from 25837d8 to c71086f Compare February 5, 2024 12:05

juditnovak changed the title ~~[IGNORE][POC] Judit experimenting with DP-libs design~~ [POC] Zookeeper using Juju secrets (both internally and for cross-charm relations Feb 6, 2024

juditnovak changed the title ~~[POC] Zookeeper using Juju secrets (both internally and for cross-charm relations~~ [POC] Zookeeper using Juju secrets (both peer and for cross-charm relations Feb 6, 2024

juditnovak changed the title ~~[POC] Zookeeper using Juju secrets (both peer and for cross-charm relations~~ [POC] Zookeeper on Juju secrets (both peer and for cross-charm relations Feb 6, 2024

juditnovak changed the title ~~[POC] Zookeeper on Juju secrets (both peer and for cross-charm relations~~ [POC] Zookeeper on Juju secrets (peer and for cross-charm relations Feb 6, 2024

juditnovak mentioned this pull request Feb 7, 2024

Minor changes required for new-style Charm Design usage canonical/data-platform-libs#138

Draft

juditnovak requested review from deusebio and marcoppenheimer February 7, 2024 20:19

juditnovak commented Feb 7, 2024

View reviewed changes

juditnovak changed the title ~~[POC] Zookeeper on Juju secrets (peer and for cross-charm relations~~ Zookeeper on Juju secrets (peer and for cross-charm relations Feb 7, 2024

juditnovak marked this pull request as ready for review February 7, 2024 20:27

juditnovak commented Feb 7, 2024

View reviewed changes

marcoppenheimer reviewed Feb 7, 2024

View reviewed changes

juditnovak changed the title ~~Zookeeper on Juju secrets (peer and for cross-charm relations~~ [DPE-3543] Zookeeper on Juju secrets (peer and for cross-charm relations Feb 8, 2024

deusebio reviewed Feb 8, 2024

View reviewed changes

juditnovak mentioned this pull request Feb 20, 2024

[POC][IGNORE] DP-libs with events split from data abstraction #121

Closed

juditnovak closed this May 3, 2024

deusebio deleted the poc_judit_dp_libs_experiment branch May 3, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-3543] Zookeeper on Juju secrets (peer and for cross-charm relations #112

[DPE-3543] Zookeeper on Juju secrets (peer and for cross-charm relations #112

juditnovak commented Feb 4, 2024 •

edited

Loading

juditnovak Feb 7, 2024

juditnovak Feb 7, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

juditnovak Feb 8, 2024

marcoppenheimer Feb 7, 2024

juditnovak Feb 8, 2024

marcoppenheimer Feb 8, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

juditnovak Feb 8, 2024

marcoppenheimer Feb 8, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

marcoppenheimer Feb 7, 2024

juditnovak Feb 8, 2024

marcoppenheimer Feb 8, 2024

deusebio Feb 8, 2024

juditnovak Feb 8, 2024

juditnovak commented Feb 9, 2024

juditnovak commented May 3, 2024

		class ClusterStateBase(Object):
		"""Collection of global cluster state for Framework/Object."""

[DPE-3543] Zookeeper on Juju secrets (peer and for cross-charm relations #112

[DPE-3543] Zookeeper on Juju secrets (peer and for cross-charm relations #112

Conversation

juditnovak commented Feb 4, 2024 • edited Loading

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

juditnovak commented Feb 9, 2024

juditnovak commented May 3, 2024

juditnovak commented Feb 4, 2024 •

edited

Loading