Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
62f42b0
Tox changes: adding pytest-mock to integration tests
juditnovak Aug 26, 2023
b6a9eb0
Adding secrets to data_interfaces module
juditnovak Aug 24, 2023
4f7857e
Unittests
juditnovak Aug 26, 2023
a6ad997
Integration tests
juditnovak Aug 27, 2023
510d00b
Changes on the Kafka tests
juditnovak Aug 27, 2023
57caab5
Openserch tests
juditnovak Aug 27, 2023
595a225
Multi-secret mechansim (mostly) in place
juditnovak Aug 29, 2023
99ca0dc
Unittests on multi-secrets
juditnovak Aug 29, 2023
18f3d6c
Integration tests updated with multi-secrets
juditnovak Aug 29, 2023
96d890d
No braking change
juditnovak Sep 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 63 additions & 6 deletions lib/charms/data_platform_libs/v0/data_interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent):
from enum import Enum
from typing import Dict, List, Optional, Union

from ops import Handle, JujuVersion, Secret, SecretInfo
from ops import JujuVersion, Secret, SecretInfo
from ops.charm import (
CharmBase,
CharmEvents,
Expand Down Expand Up @@ -905,19 +905,61 @@ def extra_user_roles(self) -> Optional[str]:


class AuthenticationEvent(RelationEvent):
"""Base class for authentication fields for events."""
"""Base class for authentication fields for events.

def __init__(
self, handle: Handle, relation: Relation, app: Optional[Application], unit: Optional[Unit]
):
super().__init__(handle, relation, app, unit)
The amount of logic added here is not ideal -- but this was the only way to preserve
the interface when moving to Juju Secrets
"""

@property
def _secrets(self) -> dict:
"""Caching secrets to avoid fetching them each time a field is referrd.

DON'T USE the encapsulated helper variable outside of this function
"""
if not hasattr(self, "_cached_secrets"):
self._cached_secrets = {}
return self._cached_secrets

@property
def _jujuversion(self) -> JujuVersion:
"""Caching jujuversion to avoid a Juju call on each field evaluation.

DON'T USE the encapsulated helper variable outside of this function
"""
if not hasattr(self, "_cached_jujuversion"):
self._cached_jujuversion = None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

if not self._cached_jujuversion:
self._cached_jujuversion = JujuVersion.from_environ()
return self._cached_jujuversion

def _get_secret(self, label) -> Optional[Dict[str, str]]:
"""Retrieveing secrets."""
if not self.app:
return
if not self._secrets.get(label):
self._secrets[label] = None
if secret_uri := self.relation.data[self.app].get(f"secret-{label}"):
secret = self.framework.model.get_secret(id=secret_uri)
self._secrets[label] = secret.get_content()
return self._secrets[label]

@property
def secrets_enabled(self):
"""Is this Juju version allowing for Secrets usage?"""
return self._jujuversion.has_secrets

@property
def username(self) -> Optional[str]:
"""Returns the created username."""
if not self.relation.app:
return None

if self.secrets_enabled:
secret = self._get_secret("user")
if secret:
return secret.get("username")

return self.relation.data[self.relation.app].get("username")

@property
Expand All @@ -926,6 +968,11 @@ def password(self) -> Optional[str]:
if not self.relation.app:
return None

if self.secrets_enabled:
secret = self._get_secret("user")
if secret:
return secret.get("password")

return self.relation.data[self.relation.app].get("password")

@property
Expand All @@ -934,6 +981,11 @@ def tls(self) -> Optional[str]:
if not self.relation.app:
return None

if self.secrets_enabled:
secret = self._get_secret("tls")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (use walrus):

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

if secret:
return secret.get("tls")

return self.relation.data[self.relation.app].get("tls")

@property
Expand All @@ -942,6 +994,11 @@ def tls_ca(self) -> Optional[str]:
if not self.relation.app:
return None

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

return self.relation.data[self.relation.app].get("tls-ca")


Expand Down