mysql lib can only set a single secret

[tonyandrewmeyer]

Steps to reproduce

This charm demonstrates the issue:

#!/usr/bin/env python3

import logging
import ops
from charms.data_platform_libs.v0.data_secrets import SecretCache, generate_secret_label, SecretAlreadyExistsError

logger = logging.getLogger(__name__)

class MysqlOperator348BugCharm(ops.CharmBase):
    def __init__(self, *args):
        super().__init__(*args)
        self.secrets = SecretCache(self)
        self.framework.observe(self.on.install, self._on_install)

    def _on_install(self, event: ops.InstallEvent):
        secret1 = {"name1": "abcdefgh"}
        label1 = generate_secret_label(self, "app")
        self.secrets.add(label1, secret1, "app")
        secret2 = {"name2": "password"}
        label2 = generate_secret_label(self, "app")
        logger.info(f"label1: {label1}, label2: {label2}, secrets: {self.secrets._secrets}")
        try:
            self.secrets.add(label2, secret2, "app")
        except SecretAlreadyExistsError as e:
            logger.error("Failed to add second secret: %s", e)
        else:
            logger.info(f"label1: {label1}, label2: {label2}, secrets: {self.secrets._secrets}")

if __name__ == "__main__":  # pragma: nocover
    ops.main(MysqlOperator348BugCharm)  # type: ignore

Note that you need to have the main version of the data_platform_libs, not one from a charmcraft fetch.

Expected behavior

With the above charm, the first INFO logging line should look something like:

label1: something, label2: something-else, secrets: {'something': <...>}

And there should be a second one like:

label1: something, label2: something-else, secrets: {'something': <...>, 'something-else': <...>}

Actual behavior

With the above charm, the logging output is:

unit-secret-bug-0: 09:54:30 INFO unit.secret-bug/0.juju-log label1: secret-bug.app, label2: secret-bug.app, secrets: {'secret-bug.app': <charms.data_platform_libs.v0.data_secrets.CachedSecret object at 0x7f5da6ed24a0>}
unit-secret-bug-0: 09:54:30 ERROR unit.secret-bug/0.juju-log Failed to add second secret: Secret secret-bug.app already exists

The issue is that the generate_secret_label method in data_secrets.py takes the charm and the scope as arguments, and uses the charm's app name and the scope to form the label. The app name is obviously the same across the app, and the scope is either app or unit. This means that if you try to get a label for an app secret, you'll always get exactly the same string. This effectively means that the SecretCache system as used by mysql.py will only store a single app secret, because it believes that any secret is already in the cache after one is.

Versions

Operating system: Ubuntu 23.10

Juju CLI: 3.1.6-genericlinux-amd64

(Note that the code never gets to Juju in the problematic area).

Juju agent: 3.1.5

Charm revision: N/A

LXD: N/A

Log output

Juju debug log:

Copied above.

Additional context

This was introduced in #348. You can also see failing unit tests if you bump your ops requirement to 2.6.0 or higher (I haven't looked at exactly what changes there, but presumably some of the code starts exercising more of the secret support in ops).

[github-actions[bot]]

https://warthogs.atlassian.net/browse/DPE-3145

[tonyandrewmeyer]

Actually, I guess this is really from https://github.com/canonical/data-platform-libs/pull/117 and #348 is just when it made it into this Charm? I perhaps should have opened this against the data-platform-libs repo, sorry - I was just working through figuring out what happened with mysql-operator first, and didn't think to raise it against the lib.

Fwiw, it seems like the _generate_secret_label used in data-platform-libs is ok, because it adds in a "group" name. But the default generate_secret_label that data-platform-libs provides, and that mysql uses, can only provide one label per app+scope.

Let me know if you want me to re-open the issue there instead.

[taurus-forever]

@tonyandrewmeyer thank you for the report!

@juditnovak can you share your vision here. Thank you!

[juditnovak]

@tonyandrewmeyer @taurus-forever

Please correct me if I got anything wrong -- I may have confusion regarding the context.

By policy, most Data Platform charms have 2 secrets: 1.app-level secret: contains credentials for the charm (typically admin user, monitor user, etc.)

2. unit-level secret: contains generally TLS credentials

We have set up our libraries accordingly. This is not a bug but a policy.

If I get it right, in your tests you are aiming to break out of this frame -- i.e. aiming to do something that's purposefully not supported.

Am I getting it right?

PS: Since we do have one single charm (Opensearch) that has more secrets than listed above, we are likely to provide a generic solution in the future that would be available for all charms, supporting more than one secret. (However the number of secrets still may be restrained for charms that insist on a single secret).

[tonyandrewmeyer]

Firstly, I'm sorry for pinging about this on Matrix and then vanishing for a week :disappointed:. I had thought that this would be something that I'd be focusing on and then a bunch of unexpected work turned up and I ran out of time to come back to it until now.

I made a mistake here - apologies for that too! I thought that this was the reason that the mysql-operator and mysql-k8s-operator tests failed with ops 2.6+, but that's not the case. (I did manage to figure out what was needed and opened a data-platforms-libs PR and a mysql-operator PR, and the combination of those solve the issues).

It does seem a little odd the way that the label generation is done when there's only a pair (app, unit) of secrets, and where postgresql has secrets grouped. But I understand now that this is deliberate, so that's fine (and also that it's not causing issues, as I wrongly thought).

As an aside, the @canonical/charm-tech team would appreciate learning more about the data-platform-libs data_secrets module at some point. We're concerned that you've needed to create this pretty generic (ie. it's not really specific to the data charms) secret module on top of ops. It hints that maybe ops is missing some secret functionality that charmers need - we'd rather improve the secret functionality in ops than have all charmers need to implement wrappers on top of it.

[tonyandrewmeyer]

https://warthogs.atlassian.net/browse/DPE-3145

I don't think I can close this - if that doesn't happen automatically, could someone please do that? Sorry & thanks!