[DPE-3654] Peer cluster data stored in a secret #463
Conversation
skourta
force-pushed
the
DPE-3654-peer-cluster-secret
branch
from
376e526 to
52d9410
Compare
There was a problem hiding this comment.
Thanks Smail.
Having the rel data as a whole as part of the secrets makes us lose valuable information from the relation.
I would rather:
- we keep the relation flow as it was and just replace the plaintext values of the passwords etc.. by their secret labels in the relation data.
- grant access to all these secrets to the consumers
- consumer is able to fetch the content from these labels
- when a secret changes, we'll have to trigger a rel changed event for the consumers to see it (or do we if the secrets are already granted?)
There was a problem hiding this comment.
Hey Smail, thanks for tackling this topic! I think it needs some adjustments as we are currently mixing using the opensearch_secrets.py class and accessing secrets directly through the model, especially in the _grant_rel_data_secrets() method. This should be avoided, if possible, and methods missing the in opensearch_secrets class should be added there. This is to make sure we have a clean interface with the secrets, and don't have to adjust multiple places in the code of the ops-implementation of secrets changes.
There was a problem hiding this comment.
I am approving it, but if we have to do any changes on this PR still, @skourta, can you add the TODO notes as described? Also, we should make sure the backup tests are passing in our CI, as they depend on the secret exchanges.
| ): | ||
| redacted_dict["credentials"]["s3"] = { | ||
| "access-key": self.secrets.get_secret_id(Scope.APP, "s3-access-key"), | ||
| "secret-key": self.secrets.get_secret_id(Scope.APP, "s3-secret-key"), |
There was a problem hiding this comment.
Likewise, this secret should be caught straight from the s3 relation, once it supports granting secrets around.
There was a problem hiding this comment.
and it should be only one s3 secret, not 2 like we currently do
| # 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 | ||
| redacted_dict = rel_data.to_dict() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
| @@ -260,6 +262,13 @@ def refresh_relation_data( | |||
| # compute the data that needs to be broadcast to all related clusters (success or error) | |||
| rel_data = self._rel_data(deployment_desc, orchestrators) | |||
|
|
|||
| # if rel_data is an error, prepare to broadcast it to all related clusters | |||
| if isinstance(rel_data, PeerClusterRelData): | |||
| rel_data_redacted_dict = self._rel_data_redacted_dict(rel_data) | |||
There was a problem hiding this comment.
why is it needed to serialize into a dict?
| ): | ||
| redacted_dict["credentials"]["s3"] = { | ||
| "access-key": self.secrets.get_secret_id(Scope.APP, "s3-access-key"), | ||
| "secret-key": self.secrets.get_secret_id(Scope.APP, "s3-secret-key"), |
There was a problem hiding this comment.
and it should be only one s3 secret, not 2 like we currently do
Issue
The data of the peer cluster relation was stored in the app data bag in text format including passwords and sensitive data.
Solution
Move the data of the peer cluster relation to a juju secret