Propagate requested entity secret for OpenSearch#267
Conversation
akram09
requested review from
Gu1nness,
dragomirp,
imanenami,
reneradoi and
sinclert-canonical
There was a problem hiding this comment.
Giving a +1 just to help the OpenSearch team get this unblocked.
Please consider: I have no context whether these fields make sense for OpenSearch, the main reviewers should be @akram09 (as OpenSeach main dev) and @dragomirp (who introduced this functionality to begin with).
reneradoi
left a comment
reneradoi
left a comment
There was a problem hiding this comment.
Hi Serhii! Thank you for bringing this PR! I want to point you to an issue related to secrets sharing in cross-model relations that should be fixed, to avoid unpleasant deploy-time issues. Please see the comment below for details. Happy to provide more context or information if required, please let me know (you can ping me in the Opensearch Mattermost channel).
Furthermore I see that quite a few integration tests are failing currently. This should also be investigated.
ping @akram09 for heads-up
| extra_group_roles, | ||
| entity_type, | ||
| entity_permissions, | ||
| None, |
There was a problem hiding this comment.
Better pass keyword arguments instead of just None to make it easier to read/understand.
There was a problem hiding this comment.
Sharing a secret directly most probably won't be supported in v1, so it is OK to skip, but it is better to use keywords as requested.
| secret = self.charm.app.add_secret( | ||
| content, label=f"{self.model.uuid}-{event.relation.id}-requested-entity" | ||
| ) | ||
| secret.grant(event.relation) |
There was a problem hiding this comment.
This will fail in cross-model relations, due to a constraint in Juju: sharing consumer secrets across a cross model relation not supported. For details, see DA264.
Cross-model relations could be more relevant for Opensearch than they are for the current users of this feature, therefor I would recommend to take this into consideration before going forward with this PR.
With #266 a solution was added for the mTLS certificate that could be adapted for the requested_entity_password, too: In case of cross-model relations, store the sensitive information in encrypted form as part of the relation data itself and not as a secret.
There was a problem hiding this comment.
For context, when I implemented this originally for Postgresql I followed the same approach as mTLS. Since mTLS itself switched, it makes sense to port this as well. I lack the context to say if it is a strict requirement for Opensearch or not.
dragomirp
left a comment
dragomirp
left a comment
There was a problem hiding this comment.
LGTM in principle. It needs to be decided if switching to encrypted credentials instead of consumer secret is a hard requirement for Opensearch.
4 participants
This PR adds support of
requested-entity-secret(requested-entities-secretconfig field) for OpenSearch connections, which is required for bringing LDAP integration according to DA-265.Before:
After: