-
Notifications
You must be signed in to change notification settings - Fork 10
[DPE-7379] Requested entity name #239
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
Conversation
31f17de
to
b75d73d
Compare
b75d73d
to
69b2e2d
Compare
@@ -1 +1 @@ | |||
ops >= 2.0.0 | |||
ops >= 2.0.0, < 3.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops 3 doesn't doesn't work on focal.
@@ -1926,13 +1926,27 @@ def __init__( | |||
extra_group_roles: Optional[str] = None, | |||
entity_type: Optional[str] = None, | |||
entity_permissions: Optional[str] = None, | |||
requested_entity_secret: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass a secret URI directly. Simpler but the user needs to manage grants and clean up the secret after relation.
requested_entity_name: Optional[str] = None, | ||
requested_entity_password: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the entity name and a password, to create a temporary secret from inside the lib.
): | ||
try: | ||
secret = self.framework.model.get_secret(id=secret_uri) | ||
secret.remove_all_revisions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove the relation data, so that the secret isn't recreated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work overall, just a very few questions.
Could you add some unit tests to the PR on top of the integration testing so that we can ensure easily the safeguards you added (not mixing the secret uri and requested username + password) ?
) and not self.secrets_enabled: | ||
raise SecretsUnavailableError("Secrets unavailable on current Juju version") | ||
|
||
if self.requested_entity_secret and self.requested_entity_name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add the case where requested_entity_secret
and requested_entity_password
to prevent that case as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks for entity secret and entity password conflict and for entity password without entity name added in 09f95f2
# Create helper secret if needed | ||
if ( | ||
self.relation_data.requested_entity_name | ||
and "requested-entity-secret" not in event_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency of checks I would rather use and not self.relation_data.requested_entity_secrets
.
Or there's a difference semantically in this case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in fc5f510. My concern was if the secret will be recreated, but it doesn't seem to happen when scaling data-int on k8s.
self.relation_data.requested_entity_name: str( | ||
self.relation_data.requested_entity_password | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm fully happy about the formatting here.
If I understand correctly, this means the format is <username>: <password | None >
?
What about this other option ?
username: <username>
password: <password>
I'm just a bit afraid of the None
encoded as a string in this case whereas the other way we could just not add the password field ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern here is v1 compatibility, my understanding is that we should be able to set multiple entities in v1 which should be easier with a mapping (one key, one entity). With explicit name
and password
we have to decide how to expand on this. Multiple secrets would be too much complexity IMHO, so we'll most probably need to somehow serialise the name/pass pairs, either by setting lists of names and passwords matching by index, eg:
names: ["username1", "rolename1", "username2"]
passwords: ["pass1" null, null]
Possibly passing the type here as well (type: ["user", "role", "user"]
).
Or having a nested mapping inside the predefined secret key, eg:
requested-entities: {"username1": { "password": "pass1", "type": "user"}, "rolename1" {"password": null, "type": "role"}}
Since this is still a WIP, I'm not opposed to changing the format, so the question is which would be the most appropriate way to make this v1 compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rechecked the specification and the agreed format is:
entity-name: <username>
password: <password>
Updated to that format in 3f7d614.
self.relation_data.requested_entity_password | ||
) | ||
}, | ||
label=f"{self.model.uuid}-{event.relation.id}-requested-entity", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to globally unique label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Charm interfaces PR: canonical/charm-relation-interfaces#294 |
Had to pin ops dependency for the s3 test charm as well. |
Implement a way to request entity name by passing a secret.
Draft implementation PRs: