[DPE-2342] Charm Relation (i.e. 'external') Secrets

[juditnovak]

Disclaimer

Here we introduce Relations Secrets on top of the existing data_interfaces logic.

New interfaces:

• get_relation_secret_data() (Provides, Requires)
• set_relation_secret_fields() (Provides)
• get_relation_secret_fields(), get_relation_secret_fields() (Requires)

POC demo of usage demonstrated on Opensearch: canonical/opensearch-operator#116

[marceloneppel]

Overall it looks great! I think we are in the right path with this implementation.

About the cache, from what I could see, it will be restricted to the event lifetime only.

I left comments about other details.

[shayancanonical]

I really like the direction that this implementation is taking! Abstracting away whether a relation data is stored in a secrets backend or in the relational databag is an excellent idea!

A couple of high level comments/questions I had after my first review were:

1. SecretCache is not really a cache - it is a wrapper around a Secret in a Juju secrets backend
2. Should we allow the Requires application to decide which fields are SECRET_FIELDS instead of hardcoding them? (and fallback to the hardcoded value if they are not specified?) -> this will allow users to specify all the fields that they want as secrets

[juditnovak]

@shayancanonical

SecretCache: Your point is valid, it is

1. An abstraction layer to handle direct interaction with the Juju Secret Strore
2. However it IS a cache too :-)

The point is that, once you fetch a secret via a SecretCache object, it won't be fetched again within the same event scope.
So it's kinda providing a smart layer between DataInterfaces and Juju. It's both handling the interactions AND if the interaction is avoidable it would just return the previously cached data.

(I may need to polish up a code, there could be errors on this still :-) )

[shayancanonical]

Looks great! Just a flood of small nits/questions

[paulomach]

I'm missing some of the secret context, but could follow the implementation. Overall looking good, my nits are well represented by other reviews

[juditnovak]

Alternatively, I can add this init() function:

def __init__(self, handle: Handle, relation: Relation, app: Optional[Application] = None, unit: Optional[Unit] = None):
    super().__init__(handle, relation, app, unit)
    self._cached_jujuversion = None
    self._cached_secrets = None

...and then copy this into 6 --currently very pretty, one-liner-- descendant classes:

def __init__(self, handle: Handle, relation: Relation, app: Optional[Application] = None, unit: Optional[Unit] = None):
    super().__init__(handle, relation, app, unit)

...but this is so ugly, that it doesn't even feel right -- just for two internal helper variables, that aren't supposed to be used outside of their encapsulated scope.... That I'd rather go with the technically less safe, but in another sense still more elegant solution. As instructions on the property functions clearly indicate: the helper variabes are meant to be encapsulated inside, noone should refer to them otherwise.

Anyway, if strong feelings on the other side, I can go for the __init__() solution.

[paulomach]

less ugly?

Suggested change

	self._cached_jujuversion = None
	if not hasattr(self, "_cached_jujuversion") or not self._cached_jujuversion:
	self._cached_jujuversion = JujuVersion.from_environ()

[paulomach]

nit (use walrus):

if secret := self._get_secret("tls"):
    return secret.get("tls")

[juditnovak]

[CRITICAL] Increase LIBVERSION !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

[shayancanonical]

@juditnovak is this PR still revelant? if not, should we close?