Don't cache secret content in Ops

[tonyandrewmeyer]

With this charm:

class DifferentSecretRefreshesCharm(ops.CharmBase):
    def __init__(self, framework: ops.Framework):
        super().__init__(framework)
        framework.observe(self.on.start, self._on_start)
        framework.observe(self.on.run_action, self._on_run)

    def _on_start(self, event: ops.StartEvent):
        try:
            self.model.get_secret(label="my-secret")
        except ops.SecretNotFoundError:
            self.unit.add_secret({"foo": "bar"}, label="my-secret")
        self.unit.status = ops.ActiveStatus()

    def _on_run(self, event: ops.ActionEvent):
        secret1 = self.model.get_secret(label="my-secret")
        secret1.set_content({"foo": "baz"})
        secret2 = self.model.get_secret(label="my-secret")
        content1 = secret1.get_content(refresh=True)
        content2 = secret2.get_content()
        event.set_results({"content1": content1, "content2": content2})

What is the expect output?

Answer 1: the two Secret objects are distinct, and should have the same content unless I call get_content(refresh=True) on that specific object.

Answer 2: the two Secret objects are wrappers on the same Juju secret, and get_content() should give me the content that the Juju secret-get command provides.

At the moment, ops gives the former:

$ juju run different-secret-refreshes/0 run
Running operation 472 with 1 task
  - task 473 on unit-different-secret-refreshes-0

Waiting for task 473...
content1:
  foo: baz
content2:
  foo: bar

I have some sympathy for the idea that the content is part of the Python Secret object and so shouldn't change between get_content() calls if that specific object hasn't told it to. However, I think this is confusing, because charmers don't think of these as Python object, they think of them as the Juju secret, and that has changed.

[dimaqq]

Let's validate whether the juju agent makes live calls to some vault to get secret data or already has a copy of secret data when the charm is invoked.

[tonyandrewmeyer]

One additional piece of context: prior to ops 2.14.1, set_content() had this line:

self._content = None  # invalidate cache so it's refetched next get_content()

I removed that, making this worse. If there's only one Secret object for the underlying Juju secret, then it didn't really do anything other than cause an additional secret-get call. If a new revision is to be tracked, then the refresh=True argument to get_content would replace the cache anyway, and if the revision is unchanged, then secret-get would just return the same value as in the cache anyway.

However, if there are multiple Secret objects for the same underlying Juju secret, then this sequence:

1. secret1.set_content({'value': 'refreshed'})
2. secret2.get_content(refresh=True)
3. secret1.get_content()

would give the new content in (3) (ignoring the Juju issue). However, this sequence:

1. secret1.set_content({'value': 'refreshed'})
2. secret2.get_content()
3. secret1.get_content(refresh=True)
4. secret2.get_content()

would give the old content in both (2) and (4). So it was still inconsistent, just differently.

[benhoyt]

Yeah, I think this is too confusing. Let's discuss this (and the Juju bug) with the Juju team next week, and then we should figure out a fix. I like the idea of removing caching here altogether, if it's not too expensive. Otherwise we'll have to cache on the class, as you suggested.

[tonyandrewmeyer]

Juju does not cache the retrieved content even for the duration of the hook. So we either say that if the charm doesn't want to call out to the secret backend multiple times it has the responsibility of caching it, or we fix the existing ops Secret caching.

[benhoyt]

I think we should probably fix this issue in that case (eg: cache on the class). I suspect reaching out to the secret backend (say Vault) via the controller is relatively expensive, and wouldn't want each charm to have to reimplement this. And possibly get it wrong -- better to get it wrong in one place in Ops. :-)

[benhoyt]

Per discussion with Juju, we've decided to remove Ops-level caching of secret content, and just re-fetch from the Juju secret backend every time it's accessed.

[dimaqq]

Sprint discussion:

• we'll talk to Juju to figure out if they are willing to extend secret-info-get to do a "secret check" operation
• publish advice for charmers (within our documentation) that:
  • share a Secret object on your charm, try not to do model.get_secret() repeatedly
  • ensure that there's only one Secret object for a given secret