Conversation
Based on canonical#840 with rework for the new API design and simplifications.
Also simplify hook command funcs (move logic and error handling up).
Sneak in a Pyright update to Python 3.8 as I'm starting to use variable type annotations (as opposed to "# type: T" comments).
Also fix a few bugs: - secret_info_get revision - secret_grant and secret_revoke app name
| """ | ||
| return self._label | ||
|
|
||
| def get_content(self, *, refresh: bool = False) -> Dict[str, str]: |
There was a problem hiding this comment.
As a developer I'd rather be using secret.content and secret.info instead of calling secret.get_content() and secret.get_info(). And since refresh is discretionary to the user I believe it's fine to run something like
secret.refresh()
print(secret.content)
There was a problem hiding this comment.
Since the revision concept seems core the the idea of secrets, I wouldn't abstract it too much to the user.
There was a problem hiding this comment.
The very prescriptive "@property should not be used on things which do dynamic lookups" mantra probably applies to the design here, but I definitely agree that a plain property would look nicer.
It's harder, though. I'm not sure whether or not "individual" parts of .info or .content could be set anyway, which would mean a somewhat weird behavior where some attributes must be set as complete objects even if they can be accessed with granularity. It doesn't look like it.
I think we have to pick our poison without adding an entirely new class or guidewires which protect setting some parts but not others. For example, you could do secret_id = s.info.id. But you could not do the converse. For consistency, we probably to pick one or the other, and get_...(), while it feels less Pythonic in some ways, is also very clear about the fact that you're not going to be able to in reverse through a @property to set the value.
There was a problem hiding this comment.
revision is only visible to the owner of a secret. consumers of a secret cannot see the 'revision' attribute, their only option is to stick with the current content or 'refresh' to new content, leaving the old behind. (they cannot, for example, ask for 'exactly revision 10 of the content). Hence why it is pretty hidden in the consumer objects.
There was a problem hiding this comment.
Definitely. I think the discussion is far more about "what feels like a good API from OF" than how it works from Juju itself, though.
That is, foo.get_bar().some_property just doesn't feel Pythonic. My point in "picking our poison" is that, regardless of the visibility of any given attribute, it would be just as jarring to make get_content() into:
@property
def content(...)Which would make it feel natural to be able to say secret.content.foo = bar, and that won't do what users expect, so it's down to picking whether get_content().revision is an ok tradeoff (it probably is) or doing ~50 lines of additional work to SecretInfo to make all of the attributes into _attribute with @property getters which do not have setters, so foo = secret.content.bar works but secret.content.foo = bar does not. That's just throwing a different kind of exception, even if it's more Pythonic to write.
get_content().... is fine IMO.
There was a problem hiding this comment.
Thanks for the comments, folks. Yeah, I think there are enough problem with properties here, given that some properties are not settable, some are owner and some consumer, etc. So I'm going to stick with what we have. Not perfect, but clear about what you can set and when hook tool calls are made.
| YEARLY = 'yearly' | ||
|
|
||
|
|
||
| class SecretInfo: |
There was a problem hiding this comment.
Why do we need a class for SecretInfo? Couldn't we simply call secret.id or secret.revision? it seems cleaner than secret.get_info().id
There was a problem hiding this comment.
It gives an easy factory with from_dict, if nothing else, but agreed that many of the properties could be added to Secret more transparently.
There was a problem hiding this comment.
It also separates out "what is visible to Owners" vs "what is visible to Consumers".
There was a problem hiding this comment.
Yeah, the reason John gives is the main one: most of the items in SecretInfo are only available to secret owners.
|
|
||
| def grant(self, relation: 'Relation', *, unit: Optional[Unit] = None): | ||
| """Grant read access to this secret. | ||
|
|
There was a problem hiding this comment.
I am not a huge fan of the fact that the user has to manually pass the secret id over the relation data even though it's been granted. It feels redundant and mistake prone. Here we can use the example from the database test charm:
secret.grant(event.relation)
event.relation.data[self.app]["db_password_id"] = secret.id
Secrets could have their own standard formatting in relation data. Example: {secrets: [{'id': '1234', 'label': "db-password"}]}. When you run secret.grant(relation), it gets added to the relation data, when you run secret.revoke(relation)` it gets removed.
Here the problem I see is every charm developer having to reinvent the wheel in terms managing the relation content with secret id's.
Any thoughts?
There was a problem hiding this comment.
I agree this is interesting, and may have been worth discussing when the secrets spec was being finalized. However, for better or worse this ship has sailed (and John's points below are helpful too).
There was a problem hiding this comment.
I would worry, as always when "just automagically put stuff in relation data" (especially as nested objects) about the sheer amount of churn and events which would get fired off from a change like that, especially with a number of Juju workers the same as GOMAXPROCS.
Every secret event being added to relation data as a default more or less means that every unit in that relation will get an event, and when the foo_relation_changed hook fires, how much logic it performs and how much time it takes is more or less unknowable to Juju/OF, which could lead to an "event storm" pretty easily.
There's a more or less middle ground here would could be split out into a separate issue on OF (and, realistically, probably closed, but at least there's a place for discussion about it) to extend the signature here into:
def grant(self, relation: 'Relation', *, unit: Optional[Unit] = None, databag_key: Optional[str] = None):Then, at the bottom of the method, to:
if databag_key:
if unit:
relation[unit][databag_key] = secret.id
else:
relation[relation.app][databag_key] = secret_idI probably don't think OF should be handling this kind of operation. It's trivial enough to implement, and OF doesn't deal with handling actual charm data. Sure, it's asking charm authors to re-invent the wheel, but it's also true that my charms and your charms and Ben's charms and John's charms and so on don't have the same mental model of what's in a relation databag, the keys each of us pick for coherency will not be the same, there's no real expectation of arbitrary re-use of secrets with predictive names between my charm and yours, etc.
Personally, if I were picking an "idealized" workflow for secrets and relations as an addition to the spec, it would be a way to avoid the relation databag at all, and simply have the containeragent fire an event directly at the unit/app which was just granted access anyway.
|
...
+
I am not a huge fan of the fact that the user has to manually pass the
secret id over the relation data even though it's been granted. It feels
redundant and mistake prone. Here we can use the example from the database
test charm:
secret.grant(event.relation)
event.relation.data[self.app]["db_password_id"] = secret.id
Secrets could have their own standard formatting in relation data.
Example: {secrets: [{'id': '1234', 'label': "db-password"}]}. When you
run secret.grant(relation), it gets added to the relation data, when you
run secret.revoke(relation)` it gets removed.
Here the problem I see is every charm developer having to reinvent the
wheel in terms managing the relation content with secret id's.
Any thoughts?
My concerns for that is that there is usually context around *what* the
secret is. And you can certainly end up with multiple secrets granted to
you, and which secret should you use for what purpose. (Is this secret a db
password, or a TLS certificate for the socket connection, or...)
Automatically populating content means you don't actually have an
opportunity to give the context.
If the design had been "secret-grant ID TARGET PURPOSE" or something along
those lines, it would be ok, but it was intentionally designed that the
context would be transferred as part of the relation data instead.
John
=:->
…------------------------------
In ops/model.py
<#861 (comment)>:
> + return self._id
+
+ @Property
+ def label(self) -> Optional[str]:
+ """Label used to reference this secret locally.
+
+ This label is locally unique, that is, Juju will ensure that the
+ entity (the owner or consumer) only has one secret with this label at
+ once.
+
+ This will be None if you obtained the secret using
+ :meth:`Model.get_secret` with an ID but no label.
+ """
+ return self._label
+
+ def get_content(self, *, refresh: bool = False) -> Dict[str, str]:
Since the revision concept seems core the the idea of secrets, I wouldn't
abstract it too much to the user.
—
Reply to this email directly, view it on GitHub
<#861 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7M357T5LO563OVNGB3WM4VKJANCNFSM6AAAAAASM4K7LQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
It's true that a charm could have access to multiple secrets granted from other charms. And for this reason, labels and descriptions should also be part of what is passed over the relation data so that it's easy to parse. Do we have usecases where this approach wouldn't work? If charm X is related to charm Y and Z and both provide secrets to X. It's possible (and probable) that both will place secret id's differently in the relation data and charm X has to know about the specialities of both. Making handling secrets standard across charms would reduce complexity and probably avoid bugs. |
| label = secret.label | ||
| self.charm.on.secret_remove.emit(secret_id, label, revision) | ||
|
|
||
| def trigger_secret_expiration(self, secret_id: str, revision: int, *, |
There was a problem hiding this comment.
The method names that trigger events don't follow the same conventions. For example container_pebble_ready triggers pebble ready and trigger_secret_expiration triggers secret expiration. I understand we probably don't want to break container_pebble_ready here but it'd be great to standardize over a convention (ex. trigger_x means that x event is triggered).
There was a problem hiding this comment.
Yeah, good spotting. I do like the trigger_ prefix here, and think that container_pebble_ready should probably have been trigger_pebble_ready or similar (it takes the container name as a parameter). Yeah, I don't think we should change container_pebble_ready, but I'll keep in mind to standardize on trigger_x from now on.
ops/testing.py
Outdated
| self._backend._calls.clear() | ||
| return calls | ||
|
|
||
| def add_model_secret(self, app_or_unit: AppUnitOrName, content: Dict[str, str]) -> str: |
There was a problem hiding this comment.
I know you detailed the reason why the model in the method name but the fact that a same method name is present in 2 classes that are used in different context isn't a problem in my opinion.
There was a problem hiding this comment.
It's not a problem as such, and the previous iteration of this PR had it named add_secret, but we had a comment explaining why this was not the same as Application.add_secret (implying "be careful"), so I think the distinct name is best just to avoid confusion.
| """ | ||
| owner_name = _get_app_or_unit_name(app_or_unit) | ||
| model.Secret._validate_content(content) | ||
| return self._backend._secret_add(content, owner_name) |
There was a problem hiding this comment.
I feel like a secret object could be returned to the user instead of having them manipulate the secret ID.
def test_1():
secret = self.harness.add_secret(owner="remote-app-name", content={"a", "b"})
secret.grant(receiver="app-under-test-name")
secret.revoke()This being said, I understand this situation is not specific to secrets and that relations follow the same approach. But I hold the same opinion for relations (it would also be cleaner to access relation data using relation.data instead of get_relation_data(id=123))
There was a problem hiding this comment.
Yeah, I played with this while writing it, but I decided against it for two reasons:
- As you've noted, other similar harness calls like
get_relation_datadon't do this. - It's confusing to have a
testing.Secretobject that's somewhat similar to themodel.Secretclass, but also different enough to be confusing (and the methods have slightly different parameters/types).
So overall I think this is a reasonable approach that fits the current methods.
| YEARLY = 'yearly' | ||
|
|
||
|
|
||
| class SecretInfo: |
There was a problem hiding this comment.
It gives an easy factory with from_dict, if nothing else, but agreed that many of the properties could be added to Secret more transparently.
| return id | ||
|
|
||
| @classmethod | ||
| def _validate_content(cls, content: Optional[Dict[str, str]]): |
There was a problem hiding this comment.
This doesn't feel right. I'm not sure that a method to validate should be throwing exceptions this way. I would expect that a _is_valid_secret or _validate_secret_content or _validate_content method would return a bool, and that we'd log whatever the problems are. Particularly given the use case here.
It's definitely one of those times where I wish that Python had a stronger typing story, but as a user, I would hope for some way via the API to ask "is this dict able to converted into a secret?" before calling a method and being forced to catch ValueError|TypeError.
If would be one thing if this returned a materialized object, but the usage pattern seems to be "dict -> _validate_content -> no exception -> constructor".
Pointedly, this is not even invoked from the constructor itself, and probably should be. Otherwise, there's not necessarily much to stop users from just s = Secret(....) and getting back an invalid, pretty unusable Secret object.
Whether or not we add something like:
@classmethod
def is_valid(cls, content: Optional[Dict[str, str]]) -> bool:
try:
cls._validate_content(cls, content)
return True
except (ValueError, TypeError) as e:
logger.debug("Invalid secret: {!r}", e.msg)
return FalseWe should at least also invoke _validate_content from the constructor here for a guardrail.
There was a problem hiding this comment.
I kinda disagree here that this is an issue. For a start, if _validate_content (or whatever we call it) returns a bool, you can't give good error messages when you do raise.
I don't think we need a way for the user to ask is this a valid secret. You can just add it and the OF will check for you and you'll get an appropriate TypeError or ValueError if it's no good. That said, we can always add a user-facing function later if there's a compelling use case.
I don't think it needs to be invoked from the constructor, as "This class should not be instantiated directly" as the docstring says. The Secret class is difficult to instantiate directly anyway, as the first argument is backend, which the user doesn't really have access to (backend implementations and accessors are underscore-private). So the user won't be calling Secret() directly, and every place we call it with content we validate the content first. We could do it in the constructor as well, but that would duplicate effort.
There was a problem hiding this comment.
I think this is putting way too much faith into users reading the docs and/or using an editor with LSP hinting.
Even if we don't make a public method which allows users to validate "is this a valid dict to turn into a secret" which returns a bool (and logs messages along with returning False if it's not), this sort of validation should absolutely be invoked from the constructor.
It is not hard to instantiate directly. Python doesn't really enforce this, the constructor doesn't check, and it's trivial for users to just pass None, even if they only do it once, to say nothing of the fact that even __foo is still accessible from people writing code. As a user of OF, I don't want yet another place in my code where I need to potentially catch exceptions which OF should be handling (and re-raising as a more specific exception, like ModelError or whatever), and I definitely do not want to do it for a case like this.
Frankly, the only reason it's duplicating effort to put it into the constructor is because OF itself explicitly calls it before add_secret, and in set_content, this gets called, but there also, it checks the dict against an arbitrary, private, class-level regexp _key_re = re.compile(r'^([a-z](?:-?[a-z0-9]){2,})$'), and if it fails, OF throws a ValueError or TypeError. It's very, very possible for a relatively new user of Secrets to run into multiple exceptions (trying to pass a non-str as a value, trying a disallowed key format, etc) before actually encountering a usable value.
It's just a bad user experience. It is extremely safe, and easy, to both move the validation into the constructor itself instead of calling it from OF code before constructing an object (and logically identical). It's also easy to throw a InvalidSecretError with a message which includes the complete allowed format before users hit one or the other, and "the docs are clear" and/or "the Juju docs specify the format" probably aren't discoverable enough when it's this simple to make it better.
There was a problem hiding this comment.
Hmm ... I don't mind improving the experience here, but two things:
- By itself this doesn't make it good, but there's a precedent for this, for example
RelationandRelationDataand other model classes say the same thing, "This class should not be instantiated directly", but don't prevent you from instantiating it with a dummy backend and/or invalid data attributes if you really want (Python's "we're all consenting adults" philosophy). - In practice, it seems to me that if you were blindly trying to call
Secret()to instantiate a secret object, you'd at least have to get the args right, and your editor would show you that "backend" is a required argument. So you'd be guided to think "What do I need to supply for 'backend'? I'll read the docstring ... oh, I see, I should be calling add_secret or similar."
Has the issue you're describing been (or will this be) a problem in practice? Have we run into times where people are actually trying to instantiate (say) a Relation directly, with an explicit None backend, and things going wrong?
If we consider improving this we should at least know whether it's a real problem today, and then consider it on the larger scale of also improving the situation for the existing classes like Relation (in a separate PR).
There was a problem hiding this comment.
Oh, one other thing: it's actually logically different to call it in the constructor, because currently for example in add_secret we want to call it before calling the hook tool, whereas the Secret(...) call is only called after that. So if we added it to the constructor you'd still need it in both places.
There was a problem hiding this comment.
- I mean, yes, it's Python, so you can do a lot of things. Just because it may be one way doesn't mean that it still can't be as safeguarded as possible, though. It only appears to be used in a few places.
[Application|Unit].add_secret(in both cases moving to the constructor is an easy step and logically identical).Secret.set_content(in which case the validation could raise a more explicit exception with a clear message). It's already past the constructor here.- My suggestion that "we could call it from the constructor" doesn't necessarily mean "move the code to the constructor". Just to call the method from inside the constructor explicitly rather than before it in
[Application|Unit].add_secret, which is more or less -2 lines (in eachadd_secret) and +1 (in__init__). It's pretty small and adds a lot of "training wheels"
- In practice, yes, sort-of. The assumption that users actually read and/or pay attention to what their editor suggests in dynamically-typed languages is generous, in my experience, but even so, it would be easy for OF users to run face-first into this when trying to
set_contentif they violated an invisible regexp (where the docstring is not nearly as clear as the messages in the exceptions raised by_validate_content) in any case.
Yes, we have run into people trying to instantiate, say, a Relation directly, albeit mostly people who are new to charming and/or OF.
More directly, we (and I) have been impacted by needing to add an annoyingly high amount of exception guarding in charm code from behavior changes in OF (in particular, relation_get suddenly being able to return None). Open source is a democracy/consensus model, and sometimes pain points are deemed to be too much, and sometimes they're acceptable consequences. I mean, you don't necessarily have to implement the suggestions here. I'm not always right, but I always have an opinion ;)
My opinion in this case is that the amount of work needed to "bubble wrap" things a little bit and provide more meaningful exceptions is small in terms of the potential time saved for charm authors, but "considered, and I'm not going to, they can read the docs" is ok, too. It's your call, ultimately.
In practice, _key_re requires _validate_content to be a @classmethod to access it, but it's never accessed by anything else, and could just as easily be a top-level constant if we really wanted to compile it only once, or a method-level variable anyway, in which case _validate_content could also be @staticmethod anyway.
The desire to call it prior to the hook is true, but the constructor for Secret has id as optional anyway. Again, in practice, there's not a practical difference between instantiating Secret first and setting Secret._id after the hook tool versus the converse except for your preference, and that doing the inverse adds input validation to the constructor. Your choice, in the end.
There was a problem hiding this comment.
Regarding "logically identical", maybe I'm missing what you're saying here, but moving the _validate_content step to the constructor wouldn't work for the two add_secret methods, because you need to validate before calling the hook tool. I could create the Secret object at the top of those functions, but like you suggest, you'd have to poke the id in afterwards, so it's awkward. And for get_secret it would work, but mean we're unnecessary validating content we just received from Juju.
More directly, we (and I) have been impacted by needing to add an annoyingly high amount of exception guarding in charm code from behavior changes in OF (in particular, relation_get suddenly being able to return None).
This is a fair point in general. I'm hoping to be very careful with backwards compatibility as I shepherd ops. Even for the upcoming 2.0 release, which is a major version bump and can be backwards-incompatible, we're planning to be extremely careful about changing stuff, and almost all charms should continue to work as is. The main "breakage" is not API at all, but bumping the minimum Python version up to 3.8.
My opinion in this case is that the amount of work needed to "bubble wrap" things a little bit and provide more meaningful exceptions is small in terms of the potential time saved for charm authors, but "considered, and I'm not going to, they can read the docs" is ok, too. It's your call, ultimately.
It's a fair point of view. My take is that it's a bit of a false sense of security, and if people are just blindly providing dummy values for things like backend (not to mention ignoring docs), they need to up their coding game. :-)
Another counterpoint is that you can only go so far with more and more checks before the implementation (and even sometimes the usage) is unwieldy. In future we're planning to lean in harder on static type checking. But in the meantime we're striving for a happy medium of a few runtime checks as appropriate.
All that said, I've considered your point of view, but decided not to make this specific change now.
| """ | ||
| return self._label | ||
|
|
||
| def get_content(self, *, refresh: bool = False) -> Dict[str, str]: |
There was a problem hiding this comment.
The very prescriptive "@property should not be used on things which do dynamic lookups" mantra probably applies to the design here, but I definitely agree that a plain property would look nicer.
It's harder, though. I'm not sure whether or not "individual" parts of .info or .content could be set anyway, which would mean a somewhat weird behavior where some attributes must be set as complete objects even if they can be accessed with granularity. It doesn't look like it.
I think we have to pick our poison without adding an entirely new class or guidewires which protect setting some parts but not others. For example, you could do secret_id = s.info.id. But you could not do the converse. For consistency, we probably to pick one or the other, and get_...(), while it feels less Pythonic in some ways, is also very clear about the fact that you're not going to be able to in reverse through a @property to set the value.
|
|
||
| def grant(self, relation: 'Relation', *, unit: Optional[Unit] = None): | ||
| """Grant read access to this secret. | ||
|
|
There was a problem hiding this comment.
I would worry, as always when "just automagically put stuff in relation data" (especially as nested objects) about the sheer amount of churn and events which would get fired off from a change like that, especially with a number of Juju workers the same as GOMAXPROCS.
Every secret event being added to relation data as a default more or less means that every unit in that relation will get an event, and when the foo_relation_changed hook fires, how much logic it performs and how much time it takes is more or less unknowable to Juju/OF, which could lead to an "event storm" pretty easily.
There's a more or less middle ground here would could be split out into a separate issue on OF (and, realistically, probably closed, but at least there's a place for discussion about it) to extend the signature here into:
def grant(self, relation: 'Relation', *, unit: Optional[Unit] = None, databag_key: Optional[str] = None):Then, at the bottom of the method, to:
if databag_key:
if unit:
relation[unit][databag_key] = secret.id
else:
relation[relation.app][databag_key] = secret_idI probably don't think OF should be handling this kind of operation. It's trivial enough to implement, and OF doesn't deal with handling actual charm data. Sure, it's asking charm authors to re-invent the wheel, but it's also true that my charms and your charms and Ben's charms and John's charms and so on don't have the same mental model of what's in a relation databag, the keys each of us pick for coherency will not be the same, there's no real expectation of arbitrary re-use of secrets with predictive names between my charm and yours, etc.
Personally, if I were picking an "idealized" workflow for secrets and relations as an addition to the spec, it would be a way to avoid the relation databag at all, and simply have the containeragent fire an event directly at the unit/app which was just granted access anyway.
The issue for labels and description is that the label is my reference for this object not my reference for you to understand this object. And description is meant to be the human readable message as to why this content exists, which again is not a good means of communication from application to application. I agree that we could do a way of naming secrets in a uniform fashion as you grant them to the other side of a relation. But we have relation keys that handle 90% of that already, in exchange for occasionally needing to be aware of the secret's id. |
jameinel
left a comment
jameinel
left a comment
There was a problem hiding this comment.
I think the rest of the polish that we might wan to apply can certainly happen iteratively. I think overall this looks quite good.
|
|
||
| def __init__(self, handle: 'Handle', id: str, label: Optional[str], revision: int): | ||
| super().__init__(handle, id, label) | ||
| self._revision = revision |
There was a problem hiding this comment.
Since the next three all just add _revision, should they have a common "SecretOwnerEvent" or "SecretRevisionEvent" ?
There was a problem hiding this comment.
I actually had SecretOwnerEvent previously, but it's actually not all owner events (SecretRotateEvent doesn't have a revision). I tried adding a _SecretEventWithRevision just now, and it works, but doesn't show up nicely in the docs: I don't want that class part of the public API, but then the docs generator doesn't show it at all and you can't see that those two subclasses have a .revision. Hmmm, going to punt for now (can always tweak later).
Starting an already-started service now succeeds (start is idempotent). So just remove this comment.
|
|
||
| cannot perform the following tasks: | ||
| - Start service "test" (service "test" was previously started) | ||
| """ |
There was a problem hiding this comment.
Drive by fix: Starting an already-started service now succeeds (start is idempotent). So just remove this comment.
6 participants
This PR implements the
opswrapper for Juju secrets (3.0.2+). See also the secrets spec and the API sketch.The PR includes the functions for charm use (in
ops/model.py), the test harness for secrets (inops/testing.py), and units tests for each of those parts.QA steps
I've written two test charms, database (the secret owner) and webapp (the secret consumer) that exercise the code. To deploy locally:
Documentation changes
Pietro's documentation additions (I plan to update these for latest API design and harness in Jan 2023).