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-3654] Peer cluster data stored in a secret #463

Open
wants to merge 15 commits into
base: 2/edge
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
7 changes: 5 additions & 2 deletions lib/charms/opensearch/v0/opensearch_peer_clusters.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
)
from charms.opensearch.v0.opensearch_exceptions import OpenSearchError
from charms.opensearch.v0.opensearch_internal_data import Scope
from charms.opensearch.v0.opensearch_relation_peer_cluster import (
rel_data_from_redacted_str,
)
from ops import BlockedStatus
from shortuuid import ShortUUID

Expand Down Expand Up @@ -522,10 +525,10 @@ def rel_data(self) -> Optional[PeerClusterRelData]:
rel = self._charm.model.get_relation(
PeerClusterOrchestratorRelationName, orchestrators.main_rel_id
)
if not (data := rel.data[rel.app].get("data")):
if not (redacted_dict_str := rel.data[rel.app].get("data")):
skourta marked this conversation as resolved.
Show resolved Hide resolved
return None

return PeerClusterRelData.from_str(data)
return rel_data_from_redacted_str(self._charm, redacted_dict_str)

def _pre_validate_roles_change(self, new_roles: List[str], prev_roles: List[str]):
"""Validate that the config changes of roles are allowed to happen."""
Expand Down
159 changes: 147 additions & 12 deletions lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"""Peer clusters relation related classes for OpenSearch."""
import json
import logging
from hashlib import sha1
from typing import TYPE_CHECKING, Any, Dict, List, MutableMapping, Optional, Union

from charms.opensearch.v0.constants_charm import (
Expand Down Expand Up @@ -64,7 +65,7 @@

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 1
LIBPATCH = 2
skourta marked this conversation as resolved.
Show resolved Hide resolved


class OpenSearchPeerClusterRelation(Object):
Expand Down Expand Up @@ -260,6 +261,13 @@ def refresh_relation_data(
# compute the data that needs to be broadcast to all related clusters (success or error)
skourta marked this conversation as resolved.
Show resolved Hide resolved
rel_data = self._rel_data(deployment_desc, orchestrators)

# if rel_data is an error, prepare to broadcast it to all related clusters
skourta marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(rel_data, PeerClusterRelData):
rel_data_redacted_dict = self._rel_data_redacted_dict(rel_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed to serialize into a dict?


# grant the secrets inside the rel_data to all the related clusters
self._grant_rel_data_secrets(rel_data_redacted_dict, all_relation_ids)

# exit if current cluster should not have been considered a provider
if self._notify_if_wrong_integration(rel_data, all_relation_ids) and event_rel_id:
self.delete_from_rel("trigger", rel_id=event_rel_id)
Expand All @@ -281,9 +289,9 @@ def refresh_relation_data(
orchestrators[f"{cluster_type}_app"] = deployment_desc.app.to_dict()
self.charm.peers_data.put_object(Scope.APP, "orchestrators", orchestrators)

peer_rel_data_key, should_defer = "data", False
should_defer = False
if isinstance(rel_data, PeerClusterRelErrorData):
peer_rel_data_key, should_defer = "error_data", rel_data.should_wait
should_defer = rel_data.should_wait

# save the orchestrators of this fleet
for rel_id in all_relation_ids:
Expand All @@ -299,9 +307,19 @@ def refresh_relation_data(
# there is no error to broadcast - we clear any previously broadcasted error
if isinstance(rel_data, PeerClusterRelData):
self.delete_from_rel("error_data", rel_id=rel_id)

# are we potentially overriding stuff here?
self.put_in_rel(data={peer_rel_data_key: rel_data.to_str()}, rel_id=rel_id)
# we add the hash of the rel_data to only emit a change event
# if the data has actually changed
self.put_in_rel(
data={
"data": json.dumps(rel_data_redacted_dict),
"rel_data_hash": sha1(
json.dumps(rel_data.to_dict(), sort_keys=True).encode()
).hexdigest(),
},
rel_id=rel_id,
)
else:
self.put_in_rel(data={"error_data": rel_data.to_str()}, rel_id=rel_id)

if can_defer and should_defer:
event.defer()
Expand Down Expand Up @@ -519,6 +537,55 @@ def _fetch_local_cm_nodes(self, deployment_desc: DeploymentDescription) -> List[
if node.is_cm_eligible() and node.app.id == deployment_desc.app.id
]

def _rel_data_redacted_dict(self, rel_data: PeerClusterRelData) -> dict[str, Any]:
"""Convert the secret data to a dict for storage in a secret."""
skourta marked this conversation as resolved.
Show resolved Hide resolved
# If we use the values of the secrets then the size of the secret
# would be too large for juju we store the secret ids instead and
# fetch the secrets when needed in the requirer side
skourta marked this conversation as resolved.
Show resolved Hide resolved
secrets = self.charm.secrets
skourta marked this conversation as resolved.
Show resolved Hide resolved

redacted_dict = rel_data.to_dict()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is needed to serialize to a dict, why not work with the object and its attributes directly? I would rather we work with the former.

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 do not want to mutate the object directly. We can either use the dict or copy the object and update the content of the secret to their respective ids.

Copy link
Contributor Author

@skourta skourta Nov 25, 2024

Choose a reason for hiding this comment

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

@Mehdi-Bendriss We cannot directly use the PeerClusterRelData object directly because of the admin_tls field in the credentials which is dict. The secret IDs are str. I can either create a new model for the redacted peer cluster relation data or stick with the dict. WDYT?


redacted_dict["credentials"] = {
"admin-username": AdminUser,
"admin-password": secrets.get_secret_id(Scope.APP, secrets.password_key(AdminUser)),
"admin-password-hash": secrets.get_secret_id(Scope.APP, secrets.hash_key(AdminUser)),
"kibana-password": secrets.get_secret_id(
Scope.APP, secrets.password_key(KibanaserverUser)
),
"kibana-password-hash": secrets.get_secret_id(
Scope.APP, secrets.hash_key(KibanaserverUser)
),
}

if monitor_password := secrets.get_secret_id(Scope.APP, secrets.password_key(COSUser)):
redacted_dict["credentials"]["monitor-password"] = monitor_password
if admin_tls := secrets.get_secret_id(Scope.APP, CertType.APP_ADMIN.val):
redacted_dict["credentials"]["admin-tls"] = admin_tls

if rel_data.credentials.s3:
redacted_dict["credentials"]["s3"] = {
"access-key": secrets.get_secret_id(Scope.APP, "access-key"),
"secret-key": secrets.get_secret_id(Scope.APP, "access-key"),
}

return redacted_dict

def _grant_rel_data_secrets(
self, rel_data_secret_content: dict[str, Any], all_rel_ids: list[int]
):
"""Grant the secrets to all the related apps."""
credentials = rel_data_secret_content["credentials"]
for key, secret_id in credentials.items():
# s3 and admin-username are not secrets
if key == "admin-username":
continue

secret = self.model.get_secret(id=secret_id)
for rel_id in all_rel_ids:
if relation := self.get_rel(rel_id=rel_id):
secret.grant(relation)
skourta marked this conversation as resolved.
Show resolved Hide resolved


class OpenSearchPeerClusterRequirer(OpenSearchPeerClusterRelation):
"""Peer cluster relation requirer class."""
Expand Down Expand Up @@ -573,8 +640,7 @@ def _on_peer_cluster_relation_changed(self, event: RelationChangedEvent): # noq
return

# fetch the success data
data = PeerClusterRelData.from_str(data["data"])

data = rel_data_from_redacted_str(self.charm, data["data"])
# check errors that can only be figured out from the requirer side
if self._error_set_from_requirer(orchestrators, deployment_desc, data, event.relation.id):
return
Expand Down Expand Up @@ -806,11 +872,11 @@ def _cm_nodes(self, orchestrators: PeerClusterOrchestrators) -> List[Node]:
if rel_id == -1:
continue

data = self.get_obj_from_rel(key="data", rel_id=rel_id)
if not data: # not ready yet
redacted_dict_str = self.get_from_rel(key="data", rel_id=rel_id, remote_app=True)
if not redacted_dict_str: # not ready yet
continue

data = PeerClusterRelData.from_dict(data)
data = rel_data_from_redacted_str(self.charm, redacted_dict_str)
skourta marked this conversation as resolved.
Show resolved Hide resolved
cm_nodes = {**cm_nodes, **{node.name: node for node in data.cm_nodes}}

# attempt to have an opensearch reported list of CMs - the response
Expand Down Expand Up @@ -847,7 +913,7 @@ def _error_set_from_providers(

error = None
for rel_id in orchestrator_rel_ids:
data = self.get_obj_from_rel("data", rel_id=rel_id)
data = self.get_from_rel("data", rel_id=rel_id, remote_app=True)
error_data = self.get_obj_from_rel("error_data", rel_id=rel_id)
if not data and not error_data: # relation data still incomplete
return True
Expand Down Expand Up @@ -960,3 +1026,72 @@ def _clear_errors(self, *error_labels: str):
error = self.charm.peers_data.get(Scope.APP, error_label, "")
self.charm.status.clear(error, app=True)
self.charm.peers_data.delete(Scope.APP, error_label)


def rel_data_from_redacted_str(
charm: "OpenSearchBaseCharm", redacted_dict_str: str
) -> PeerClusterRelData | None:
"""Construct the peer cluster rel data from the secret data."""
content = json.loads(redacted_dict_str)
credentials = content["credentials"]

secrets = charm.secrets
skourta marked this conversation as resolved.
Show resolved Hide resolved

admin_password = (
charm.model.get_secret(id=credentials["admin-password"])
.get_content()
.get(secrets.password_key(AdminUser))
)

admin_password_hash = (
charm.model.get_secret(id=credentials["admin-password-hash"])
.get_content()
.get(secrets.hash_key(AdminUser))
)

kibana_password = (
charm.model.get_secret(id=credentials["kibana-password"])
.get_content()
.get(secrets.password_key(KibanaserverUser))
)

kibana_password_hash = (
charm.model.get_secret(id=credentials["kibana-password-hash"])
.get_content()
.get(secrets.hash_key(KibanaserverUser))
)

monitor_password = None
if "monitor-password" in credentials:
monitor_password = (
charm.model.get_secret(id=credentials["monitor-password"])
.get_content()
.get(secrets.password_key(COSUser))
)

admin_tls = None
if "admin-tls" in credentials:
admin_tls = charm.model.get_secret(id=credentials["admin-tls"]).get_content()

s3 = None
if "s3" in credentials:
s3 = S3RelDataCredentials(
access_key=credentials["s3"]["access-key"],
secret_key=credentials["s3"]["secret-key"],
)

return PeerClusterRelData(
cluster_name=content["cluster_name"],
cm_nodes=[Node.from_dict(node) for node in content["cm_nodes"]],
credentials=PeerClusterRelDataCredentials(
admin_username=AdminUser,
admin_password=admin_password,
admin_password_hash=admin_password_hash,
kibana_password=kibana_password,
kibana_password_hash=kibana_password_hash,
monitor_password=monitor_password,
admin_tls=admin_tls,
s3=s3,
),
deployment_desc=DeploymentDescription.from_dict(content["deployment_desc"]),
)
7 changes: 6 additions & 1 deletion lib/charms/opensearch/v0/opensearch_secrets.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 1
LIBPATCH = 2
skourta marked this conversation as resolved.
Show resolved Hide resolved


if TYPE_CHECKING:
Expand Down Expand Up @@ -368,3 +368,8 @@ def delete(self, scope: Scope, key: str) -> None:
self._remove_juju_secret(scope, key)

logging.debug(f"Deleted secret {scope}:{key}")

def get_secret_id(self, scope: Scope, key: str) -> Optional[str]:
"""Get the secret ID from the cache."""
label = self.label(scope, key)
return self._charm.peers_data.get(scope, label)
12 changes: 12 additions & 0 deletions tests/unit/lib/test_opensearch_secrets.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,15 @@ def test_bad_label(self):
@parameterized.expand([Scope.APP, Scope.UNIT])
def test_put_and_get_complex_obj(self, scope):
return

def test_get_secret_id(self):
# add a secret to the store
content = {"secret": "value"}
self.store.put(Scope.APP, "super-secret-key", content)
# get the secret id
secret_id = self.store.get_secret_id(Scope.APP, "super-secret-key")
self.assertIsNotNone(secret_id)
# check the secret content
secret = self.charm.model.get_secret(id=secret_id)
secret_content = secret.get_content()
self.assertDictEqual(secret_content, {"super-secret-key": str(content)})
Loading