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.

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

feat: expand the secret ID out to the full URI when only given the ID #1358

Merged
merged 8 commits into from
Sep 10, 2024
35 changes: 26 additions & 9 deletions ops/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import tempfile
import time
import typing
import warnings
import weakref
from abc import ABC, abstractmethod
from pathlib import Path, PurePath
Expand Down Expand Up @@ -295,7 +296,7 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -
raise TypeError('Must provide an id or label, or both')
if id is not None:
# Canonicalize to "secret:<id>" form for consistency in backend calls.
id = Secret._canonicalize_id(id)
id = Secret._canonicalize_id(id, self.uuid)
content = self._backend.secret_get(id=id, label=label)
return Secret(self._backend, id=id, label=label, content=content)

Expand Down Expand Up @@ -1182,8 +1183,17 @@ def __init__(
rotation: Optional[SecretRotate],
rotates: Optional[datetime.datetime],
description: Optional[str] = None,
*,
model_uuid: Optional[str] = None,
):
self.id = Secret._canonicalize_id(id)
if model_uuid is None:
warnings.warn(
'`model_uuid` should always be provided when creating a '
Copy link
Collaborator

@benhoyt benhoyt Sep 8, 2024

Choose a reason for hiding this comment

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

In what cases can this happen? Only if people manually create a SecretInfo object? (Which they shouldn't be doing.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In what cases can this happen? Only if people manually create a Secret object? (Which they shouldn't be doing.)

Manually create a SecretInfo, yes. And yes, I don't know why they would, although we don't tell them not to (as we do with Secret, for example). This is probably overly cautious - I can't find any charm doing this (Scenario does, but we can fix that in advance of a release), but seemed better than just having None work and then in some future version (3.0?) suddenly not work.

'SecretInfo instance, and will be required in the future.',
DeprecationWarning,
stacklevel=2,
)
self.id = Secret._canonicalize_id(id, model_uuid)
self.label = label
self.revision = revision
self.expires = expires
Expand All @@ -1192,7 +1202,9 @@ def __init__(
self.description = description

@classmethod
def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo':
def from_dict(
cls, id: str, d: Dict[str, Any], model_uuid: Optional[str] = None
) -> 'SecretInfo':
"""Create new SecretInfo object from ID and dict parsed from JSON."""
expires = typing.cast(Optional[str], d.get('expiry'))
try:
Expand All @@ -1208,6 +1220,7 @@ def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo':
rotation=rotation,
rotates=timeconv.parse_rfc3339(rotates) if rotates is not None else None,
description=typing.cast(Optional[str], d.get('description')),
model_uuid=model_uuid,
)

def __repr__(self):
Expand Down Expand Up @@ -1249,7 +1262,7 @@ def __init__(
if not (id or label):
raise TypeError('Must provide an id or label, or both')
if id is not None:
id = self._canonicalize_id(id)
id = self._canonicalize_id(id, backend.model_uuid)
self._backend = backend
self._id = id
self._label = label
Expand All @@ -1264,11 +1277,13 @@ def __repr__(self):
return f"<Secret {' '.join(fields)}>"

@staticmethod
def _canonicalize_id(id: str) -> str:
def _canonicalize_id(id: str, model_uuid: Optional[str]) -> str:
"""Return the canonical form of the given secret ID, with the 'secret:' prefix."""
id = id.strip()
if not id.startswith('secret:'):
id = f'secret:{id}' # add the prefix if not there already
# Add the prefix and, if provided, model UUID.
id = f'secret:{id}' if model_uuid is None else f'secret://{model_uuid}/{id}'

return id

@classmethod
Expand Down Expand Up @@ -1308,8 +1323,8 @@ def id(self) -> Optional[str]:
"""Locator ID (URI) for this secret.

This has an unfortunate name for historical reasons, as it's not
really a unique identifier, but the secret's locator URI, which may or
may not include the model UUID (for cross-model secrets).
really a unique identifier, but the secret's locator URI, which will
include the model UUID (for cross-model secrets).

Charms should treat this as an opaque string for looking up secrets
and sharing them via relation data. If a charm-local "name" is needed
Expand Down Expand Up @@ -3604,7 +3619,9 @@ def secret_info_get(
result = self._run_for_secret('secret-info-get', *args, return_output=True, use_json=True)
info_dicts = typing.cast(Dict[str, Any], result)
id = list(info_dicts)[0] # Juju returns dict of {secret_id: {info}}
return SecretInfo.from_dict(id, typing.cast(Dict[str, Any], info_dicts[id]))
return SecretInfo.from_dict(
id, typing.cast(Dict[str, Any], info_dicts[id]), model_uuid=self.model_uuid
)

def secret_set(
self,
Expand Down
28 changes: 24 additions & 4 deletions ops/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import os
import pathlib
import random
import re
import shutil
import signal
import tempfile
Expand Down Expand Up @@ -2701,7 +2702,7 @@ def planned_units(self) -> int:
return len(units) + 1 # Account for this unit.

def _get_secret(self, id: str) -> Optional[_Secret]:
return next((s for s in self._secrets if s.id == id), None)
return next((s for s in self._secrets if self._secret_ids_are_equal(s.id, id)), None)

def _ensure_secret(self, id: str) -> _Secret:
secret = self._get_secret(id)
Expand All @@ -2723,6 +2724,22 @@ def _ensure_secret_id_or_label(self, id: Optional[str], label: Optional[str]):
)
return secret

def _secret_ids_are_equal(self, id1: str, id2: str) -> bool:
secret_re = re.compile(
r'^(?:secret:)?(?://)?(?:(?P<uuid>[a-z0-9-]+)/)?(?P<id>[a-z0-9-]+)$', re.IGNORECASE
)
mo = secret_re.match(id1)
if not mo:
return False
model_uuid1 = mo.group('uuid') or self.model_uuid
id1 = mo.group('id')
mo = secret_re.match(id2)
if not mo:
return False
model_uuid2 = mo.group('uuid') or self.model_uuid
id2 = mo.group('id')
return model_uuid1 == model_uuid2 and id1 == id2

def secret_get(
self,
*,
Expand Down Expand Up @@ -2815,6 +2832,7 @@ def secret_info_get(
rotation=rotation,
rotates=rotates,
description=secret.description,
model_uuid=self.model_uuid,
)

def secret_set(
Expand Down Expand Up @@ -2898,7 +2916,7 @@ def _secret_add(
description=description,
)
self._secrets.append(secret)
return id
return id # Note that this is the 'short' ID, not the canonicalised one.

def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None:
secret = self._ensure_secret(id)
Expand Down Expand Up @@ -2935,9 +2953,11 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None) -> None:
secret.revisions = revisions
else:
# Last revision removed, remove entire secret
self._secrets = [s for s in self._secrets if s.id != id]
self._secrets = [
s for s in self._secrets if not self._secret_ids_are_equal(s.id, id)
]
else:
self._secrets = [s for s in self._secrets if s.id != id]
self._secrets = [s for s in self._secrets if not self._secret_ids_are_equal(s.id, id)]

def open_port(self, protocol: str, port: Optional[int] = None):
self._check_protocol_and_port(protocol, port)
Expand Down
Loading
Loading