[DPE-10184] fix mtls certificate for cross-model-relations#266
[DPE-10184] fix mtls certificate for cross-model-relations#266reneradoi wants to merge 22 commits into
Conversation
…`mtls-cert` from requirer secrets in cross-model-relations
…s-model relations
… updating a request in case the `encryption-secret` was added
…r updating a request in case the `encryption-secret` was added
… in cross-model relations
… data when fetching
291b50e to
dfadf62
Compare
…is highly error-prone
…is highly error-prone
| try: | ||
| remote_model = relation.remote_model.uuid | ||
| except (FileNotFoundError, ModelError, RuntimeError): | ||
| # access to remote model added in Juju 3.6.2, fails with 2.9 |
There was a problem hiding this comment.
What is the error raised in case of Juju 2.9 ?
Maybe we could have a log line for that specific case?
There was a problem hiding this comment.
I've added a log line.
According to ops, this raises a ModelError. But I've also seen instances of FileNotFoundError and RuntimeError in integration test runs, that's why I added those.
|
|
||
| # in cross-model relations, generate an encryption key and share it with the requirer as a secret | ||
| event_data = {} | ||
| secret_label = f"{self.model.uuid}-{event.relation.id}-encryption-secret" |
There was a problem hiding this comment.
What is the need for the model uuid in the secret label ?
There was a problem hiding this comment.
Mostly consistency with another instance of creating a "helper" secret here.
| series: | ||
| - focal |
There was a problem hiding this comment.
Isn't this for juju 2.9 purposes?
There was a problem hiding this comment.
Well, interesting one: I think this is a mistake (or leftover from the past). Even when deployed on jammy juju container, the container image was always focal, which resulted in issues when using the default Python of the container. It took quite some time to figure out why integration tests started to fail since the last commit and this repo and my PR.
Building for different bases is defined here, which stays untouched.
There was a problem hiding this comment.
What's the reason of all this cleanup ?
Where those tests testing nothing?
There was a problem hiding this comment.
Yes, these assertions where useless. The actual check happens afterwards, in the assertion of the relation data to be present (or not). Asserting log messages is error prone and should be avoided in general, I believe. Especially if there are random wait times (3s) for a message to be present. In these cases, it caused test flakeyness and CI instability, and as it doesn't bring value, I decided to remove.
imanenami
left a comment
There was a problem hiding this comment.
Thanks for the effort, looks good to me 👍
| LIBPATCH = 59 | ||
|
|
||
| PYDEPS = ["ops>=2.0.0"] | ||
| PYDEPS = ["ops>=2.0.0", "cryptography"] |
There was a problem hiding this comment.
I would prefer if cryptography is inline import with a runtime error instead of a hard dependency. It's a pretty big library a lot of products don't necessarily need and latest versions already dropped support for Python versions on some of our supported platforms (focal with 3.8).
This PR fixes an issue when providing the mTLS certificate in a cross-model relation.
Due to juju/juju#21248 (comment), granting a Juju secret from the requirer side of a cross-model relations fails with
sharing consumer secrets across a cross model relation not supported. The proposed workaround from DA264 is to place themtls-certinto regular relation data instead of a secret, if the relation is a cross-model relation.The scope of this PR is to fix this issue for
data-interfacesv0. The required fixes for v1 will be provided in a separate PR to https://github.com/canonical/data-platform-charmlibs (see canonical/data-platform-charmlibs#16).Changes:
cryptographyfor usingFernetCROSS_MODEL_RELATION_CONSUMER_SECRETS(currently only includesmtls-certCROSS_MODEL_RELATION_CONSUMER_SECRETSfor requirer-side secrets in cross-model relationsrelation-createdevent inProviderEventHandlersclass: in cross-model relations, generate the encryption key, store it in a secret, grant the secret to the requirer and add the secret id to relation data_update_relation_data_without_secrets: read the encryption key from the secretencryption-secretand encrypt keys defined inCROSS_MODEL_RELATION_CONSUMER_SECRETS; in case of failures, values will be left blank_fetch_relation_data_without_secrets: read the encryption key from the secretencryption-secretand decrypt keys defined inCROSS_MODEL_RELATION_CONSUMER_SECRETSrelation-changedevents inEtcdRequirerEventHandlersandKafkaRequirerEventHandlersclasses: resend a previous request if the encryption secret was added to relation data from provider side, to ensure data is sent correctlyTesting:
Functionality was tested manually and with integration test coverage on charmed-etcd (see this draft PR, example CI run here). Integration test coverage for this repo can be added after changes where merged in this etcd PR (because a provider charm with mtls support is needed)
cryptographyto deps for all test charmss3-apptest charmcheck_logswhere possible (only if action results are ensured otherwise) to avoid test flakyness