Repository: canonical/operator
Pull request #855 (2022-11-16 09:35:54)

Add JujuVersion.has_secrets()
add logic to check if secrets supported

```python
if juju_version.secrets_supported:
    # do smth with secrets
```

jnsgruk: I'm not sure this is necessarily the right approach. We already have `assumes` that can be used in `metadata.yaml` to specify a Juju version, and we could probably add a `secrets` entry (@wallyworld?).

The secrets implementation (currently WIP) can just raise an exception if the `secret-*` hook tool being called is not present in the hook tools dir, allowing charm authors to catch that case and handle things differently?
beliaev-maksim: @jnsgruk 
if I got `assumes` right, then we cannot create a charm that will work on both 2.x and 3.x
while providing a logic to check, we can branch decision

also, doing if/else check looks to be a more cleaner alternative and to my knowledge faster than try/except block in scenarios when except is called

but if we already have a good way to do a check, then we just might need document it instead of modifying the code
jnsgruk: > if I got `assumes` right, then we cannot create a charm that will work on both 2.x and 3.x
> while providing a logic to check, we can branch decision

Yeh, you're right there. If you use `assumes: ["secrets"]` then the charm would only deploy on `3.x`

As for whether to provide a simple check method or rely on `try`/`except`, I'll defer to @benhoyt for his preference there :+1: 
jameinel: Historically if you want your charm to support versions of juju that both do and don't support a feature, then you would probe to see if the command was available (LookPath secret-get, etc). That was used when we introduced leadership and 'is-leader'.
And then you would want branching code paths in your charm.

However, if you don't want branching code paths, and you just want to move to secrets always, then Juju should definitely have an `assumes: [secrets]`, I'm not sure if we have that with 3.0.0, but we should definitely add it in 3.0.
That would prevent 2.9 from deploying the charm, and the charm just always assumes that secret-get is available.

The harder problem (IMO), is more about relation interface versioning. If the new version of postgresql wants to use secrets to share its database password, it needs to know whether Juju supports it (but as above that is relatively obvious), but it also needs to know if the *related application* knows how to interact with secrets.
And that is more a relation-interface versioning problem. (eg, what is the actual protocol of `interface: pgsql`). We've had lots of conversations around versioning, and how to maintain compatibility, etc. It gets quite tricky because if postgresql wants to start using secrets, it can put that information in the relation data, but wordpress will read the relation data, and not find a password in the relation, and assume it just needs to wait until a password shows up, without sending a message back to postgresql that "I don't know what this `password-secret:` field means". 
Which means that the *compatible* relation interface is to still fill a plaintext password into relation data, and a password secret identifier that isn't yet helping.
(or you introduce version negotiation into the relation data, or this pushes us to actually do relation interface versioning in Juju.)
benhoyt: I think this is actually a reasonable addition, given that this would be useful for folks that want to create charms that work on 2.9 and 3.0 (without secrets or with secrets), and we have some precedent for these helpers (`has_controller_storage` and others). It seems nicer for the charmer than having to explicitly know to do something like `shutil.which('secret-get')`.

As @jameinel mentioned, if you just want to say "this charm *requires* secrets", `assumes` seems like the right way to go. We don't have an `assumes: [secrets]` right now; you'd have to say `assumes: [juju >= 3.0]` or more precisely `assumes: [juju >= 3.0.2]`. Is that enough, given that it's basically 2.9 vs 3.0 anyway?

> The harder problem (IMO), is more about relation interface versioning.

Yes, that's definitely true. Though that's an existing problem with any relation data and should probably be considered separately.

PietroPasotti: Let's not do relation versioning in the databag -- got PTSD from the last shot we took at that.

I'm on board with adding `secrets_supported`, and this also makes me realize that we probably need something like a `ops.testing.SECRETS: bool` global that we can use to toggle whether the harness has secret simulation enabled or not.
Specifically for the case where one wants to test a charm that is compatible both with secret-less and newer juju versions.

Precedent for that pattern would be `testing.SIMULATE_CAN_CONNECT`, or perhaps we need a broader abstraction of the api version we're mocking against. @benhoyt is this something I should be writing also in the ops secrets spec?
benhoyt: > Precedent for that pattern would be testing.SIMULATE_CAN_CONNECT, or perhaps we need a broader abstraction of the api version we're mocking against. @benhoyt is this something I should be writing also in the ops secrets spec?

Ops secrets spec -- I wouldn't worry about adding it to that at this point. I'll think about `ops.testing.SECRETS` type of thing as I work though the secrets code.

Patch:
diff --git a/ops/jujuversion.py b/ops/jujuversion.py
index e1f83d3..7ee7f45 100755
--- a/ops/jujuversion.py
+++ b/ops/jujuversion.py
@@ -112,3 +112,8 @@ class JujuVersion:
     def has_controller_storage(self) -> bool:
         """Determine whether this juju version supports controller-side storage."""
         return (self.major, self.minor, self.patch) >= (2, 8, 0)
+
+    @property
+    def has_secrets(self) -> bool:
+        """Determine whether this Juju version supports the `secrets` feature."""
+        return (self.major, self.minor, self.patch) >= (3, 0, 2)


Test patch:
diff --git a/test/test_jujuversion.py b/test/test_jujuversion.py
index 214f602..542b81e 100755
--- a/test/test_jujuversion.py
+++ b/test/test_jujuversion.py
@@ -13,7 +13,6 @@
 # limitations under the License.
 
 import os
-import unittest
 import unittest.mock  # in this file, importing just 'patch' would be confusing
 
 from ops.jujuversion import JujuVersion
@@ -69,6 +68,11 @@ class TestJujuVersion(unittest.TestCase):
         self.assertTrue(JujuVersion('2.8.0').has_controller_storage())
         self.assertFalse(JujuVersion('2.7.9').has_controller_storage())
 
+    def test_has_secrets(self):
+        self.assertTrue(JujuVersion('3.0.2').has_secrets)
+        self.assertFalse(JujuVersion('3.0.1').has_secrets)
+        self.assertFalse(JujuVersion('2.9.30').has_secrets)
+
     def test_parsing_errors(self):
         invalid_versions = [
             "xyz",