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

[DPE-4416] URI exists while re-creating secret with modified label #170

Merged
merged 6 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 22 additions & 2 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,30 @@ jobs:
juju-snap-channel: "3.1/stable"
libjuju-version: "3.2.2"
include:
- {tox-environments: integration-upgrades,
- {tox-environments: integration-interfaces-upgrade-from-version-1-earlier,
juju-version:
{juju-snap-channel: "3.1/stable",
juju-bootstrap-option: "3.1.6",
juju-bootstrap-option: "3.1.7",
libjuju-version: "3.2.2"}}
- {tox-environments: integration-interfaces-upgrade-from-version-2-earlier,
juju-version:
{juju-snap-channel: "3.1/stable",
juju-bootstrap-option: "3.1.7",
libjuju-version: "3.2.2"}}
- {tox-environments: integration-interfaces-upgrade-from-version-3-earlier,
juju-version:
{juju-snap-channel: "3.1/stable",
juju-bootstrap-option: "3.1.7",
libjuju-version: "3.2.2"}}
- {tox-environments: integration-interfaces-upgrade-from-version-4-earlier,
juju-version:
{juju-snap-channel: "3.1/stable",
juju-bootstrap-option: "3.1.7",
libjuju-version: "3.2.2"}}
- {tox-environments: integration-upgrades-databag-to-secrets,
juju-version:
{juju-snap-channel: "3.1/stable",
juju-bootstrap-option: "3.1.7",
libjuju-version: "3.2.2"}}
- {tox-environments: integration-secrets,
juju-version:
Expand Down
10 changes: 5 additions & 5 deletions lib/charms/data_platform_libs/v0/data_interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent):

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 35
LIBPATCH = 36

PYDEPS = ["ops>=2.0.0"]

Expand Down Expand Up @@ -642,16 +642,16 @@ def _move_to_new_label_if_needed(self):
return

# Create a new secret with the new label
old_meta = self._secret_meta
content = self._secret_meta.get_content()
self._secret_uri = 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.

Underlying logic: Normally this helper field is to hold the URI of a secret (in case the secret was newly created, or fetched by URI).

At this point, we are switching the self (CachedSecret) object to point to a new secret object (that's associated with the new label).

Thus we have to "unlink" the object from the old URI. (Otherwise new secret creation is blocked: as the error in https://warthogs.atlassian.net/browse/DPE-4416 was indicating so.)


# I wish we could just check if we are the owners of the secret...
try:
self._secret_meta = self.add_secret(content, label=self.label)
except ModelError as err:
if "this unit is not the leader" not in str(err):
raise
old_meta.remove_all_revisions()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this in a long term, but I rather leave "garbage" around for now, and bring this code back after having verified that under no circumstances we may remove this data pre-mature.

See issue on this matter #171

self.current_label = 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.

This field is used to indicate when an upgrade was performed, where a secret label change may have happened.

We detect dynamically if a secret with an old label may be "hanging around", from an old version of the charm. In order to ensure smooth upgrades, we are leaving the secret associated with the old label, as long as only read operations are performed on the secret.
We hold the value of the outdated label in the CachedSecret.current_label field (while CachedSecret.label is always pointing to the label that is to be used in a long term.

When the above code is executed, we are at the point of the first write operation impacting this secret.
I.e. move to the new label (by having created a brand-new secret, that's recognized by the new label).
We need to clean the current_label field, as we have no link to the old (labelled) secret anymore.


def set_content(self, content: Dict[str, str]) -> None:
"""Setting cached secret content."""
Expand Down Expand Up @@ -1586,7 +1586,7 @@ def _register_secret_to_relation(
"""
label = self._generate_secret_label(relation_name, relation_id, group)

# Fetchin the Secret's meta information ensuring that it's locally getting registered with
# Fetching the Secret's meta information ensuring that it's locally getting registered with
CachedSecret(self._model, self.component, label, secret_id).meta

def _register_secrets_to_relation(self, relation: Relation, params_name_list: List[str]):
Expand Down Expand Up @@ -2309,7 +2309,7 @@ def _secrets(self) -> dict:
return self._cached_secrets

def _get_secret(self, group) -> Optional[Dict[str, str]]:
"""Retrieveing secrets."""
"""Retrieving secrets."""
if not self.app:
return
if not self._secrets.get(group):
Expand Down
2 changes: 2 additions & 0 deletions tests/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Copyright 2023 Canonical Ltd.
# See LICENSE file for licensing details.
37 changes: 37 additions & 0 deletions tests/integration/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import shutil
from datetime import datetime
from pathlib import Path
from subprocess import check_call, check_output

import pytest
from pytest_operator.plugin import OpsTest
Expand Down Expand Up @@ -153,3 +154,39 @@ async def without_errors(ops_test: OpsTest, request):
continue
if logitems[2] == "ERROR":
assert any(white in line for white in whitelist)


@pytest.fixture(scope="session")
def fetch_old_versions():
"""Fetching the previous 4 versions of the lib for upgrade tests."""
cwd = os.getcwd()
src_path = "lib/charms/data_platform_libs/v0/data_interfaces.py"
data_path = f"{cwd}/tests/integration/data/data_interfaces.py"
tmp_path = "./tmp_repo_checkout"

os.mkdir(tmp_path)
os.chdir(tmp_path)
check_call("git clone https://github.com/canonical/data-platform-libs.git", shell=True)
os.chdir("data-platform-libs")
last_commits = check_output(
"git show --pretty=format:'%h' --no-patch -15", shell=True, universal_newlines=True
).split()

versions = []
for commit in last_commits:
check_call(f"git checkout {commit}", shell=True)
version = check_output(
"grep LIBPATCH lib/charms/data_platform_libs/v0/data_interfaces.py | cut -d ' ' -f 3",
shell=True,
universal_newlines=True,
)
version = version.strip()
if version not in versions:
shutil.copyfile(src_path, f"{data_path}.v{version}")
versions.append(version)

if len(versions) == 4:
break

os.chdir(cwd)
shutil.rmtree(tmp_path)
17 changes: 17 additions & 0 deletions tests/integration/database-charm/actions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,23 @@ set-peer-relation-field:
type: string
description: Value of the field to set

set-peer-relation-field-multiple:
description: Set fields from the second-database relation multiple times
params:
component:
type: string
description: app/unit
field:
type: string
description: Relation field
value:
type: string
description: Value of the field to set
count:
type: integer
description: Number of iterations
default: 3

set-peer-secret:
description: Set fields from the second-database relation
params:
Expand Down
31 changes: 31 additions & 0 deletions tests/integration/database-charm/src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ def __init__(self, *args):
self.framework.observe(
self.on.set_peer_relation_field_action, self._on_set_peer_relation_field
)
self.framework.observe(
self.on.set_peer_relation_field_multiple_action,
self._on_set_peer_relation_field_multiple,
)
self.framework.observe(self.on.set_peer_secret_action, self._on_set_peer_secret)
self.framework.observe(
self.on.delete_peer_relation_field_action, self._on_delete_peer_relation_field
Expand Down Expand Up @@ -341,6 +345,33 @@ def _on_set_peer_relation_field(self, event: ActionEvent):
relation.id, {event.params["field"]: event.params["value"]}
)

def _on_set_peer_relation_field_multiple(self, event: ActionEvent):
"""Set requested relation field."""
component = event.params["component"]
count = event.params["count"]

# Charms should be compatible with old vesrions, to simulate rolling upgrade
for cnt in range(count):
value = event.params["value"] + f"{cnt}"
if DATA_INTERFACES_VERSION <= 17:
relation = self.model.get_relation(PEER)
if component == "app":
relation.data[self.app][event.params["field"]] = value
else:
relation.data[self.unit][event.params["field"]] = value
return

if component == "app":
relation = self.peer_relation_app.relations[0]
self.peer_relation_app.update_relation_data(
relation.id, {event.params["field"]: value}
)
else:
relation = self.peer_relation_unit.relations[0]
self.peer_relation_unit.update_relation_data(
relation.id, {event.params["field"]: value}
)

def _on_set_peer_secret(self, event: ActionEvent):
"""Set requested relation field."""
component = event.params["component"]
Expand Down
Loading
Loading