fix: Secret.set_info and Secret.set_content can be called in the same hook #1373
Conversation
There was a problem hiding this comment.
FWIW, if people feel like the charm hook:
secret-set $ID --label=foo
secret-set $ID --description="my description"
Should also preserve the information, then this really should be a Juju fix, and not the ops framework caching the information to pass it in a second time.
We might want to have ops try the caching thing to make it easier to support multiple versions of Juju, but we should be pushing to have the functionality changed in Juju not just in ops.
|
also, what is the behavior if you: I think the behavior is that content is replaced, and not additive. As demonstrated with: Note that this is different from But there if you do: I don't think it is predictable that |
|
Partly the reason why secret-set has to behave differently is that we don't want to cache the known existing values to do the merge operation because of their sensitivit. So we push that onto the charm author. It could be better to find a way to keep doing that for labels, etc. Though in my testing, calling The content hasn't been updated, but the label and description are changed. |
Isn't that because No expiry, but the description is set. And in the other order: The description hasn't changed, but the expiry is set. Compared to the single call where both update: |
sigh... that seems quite inconsistent. Do we have a bug on Juju about this? |
The catch with that is that We can't do it in individual
This is a pretty convincing argument.
Sure :). Also about whether we should be caching content when setting and John's comments :). |
No. We (Charm-Tech) did have a talk about this issue broadly with other people in the Juju team (it would either have been at the Charm-Tech/Juju APAC sync or Charm-Tech/Ian sync, I can't remember which). The conclusion there was that I didn't realise that
It's cached somewhere though, right? Because if I do a Even if the caching is in a transaction of sorts in the secret backend, doesn't that still make it available to Juju?
I can open a bug for this and let the Juju'ers hammer out whether it should happen :). |
|
On Tue, Sep 17, 2024 at 9:15 PM Tony Meyer ***@***.***> wrote:
Do we have a bug on Juju about this?
No. We (Charm-Tech) did have a talk about this issue broadly with other
people in the Juju team (it would either have been at the Charm-Tech/Juju
APAC sync or Charm-Tech/Ian sync, I can't remember which). The conclusion
there was that secret-set behaves consistently within itself (each call
replaces any previous call in the same hook execution), even though it's
different from relation-set (as you described), and the problem arises
from ops splitting secret-set up into two separate methods, one for the
metadata and one for the content. So we felt like this should be fixed in
ops, because ops was at fault for the limitation of not being able to set
both metadata and content in the same hook.
I didn't realise that label was different at the time - I only found that
out while testing things while working on this PR.
Partly the reason why secret-set has to behave differently is that we
don't want to cache the known existing values to do the merge operation
because of their sensitivity.
It's cached somewhere though, right? Because if I do a secret-set and
then a secret-get --peek and then crash the hook I get the new content
back in the peek but it's not saved. Is the secret backend handling this
rather than Juju? Like Juju does a transaction with the secret backend that
then isn't committed/is rolled back if handling the hook fails?
Even if the caching is in a transaction of sorts in the secret backend,
doesn't that still make it available to Juju? secret-get --peek works
somehow :).
I think it is cached only in the memory of the unit agent, which is why it
rolls back if you crash the hook.
John
=:->
… I understand why we don't want to merge secret content, but for everything
else I'd rather have multiple calls actually not replace valid values with
empty values.
I can open a bug for this and let the Juju'ers hammer out whether it
should happen :).
—
Reply to this email directly, view it on GitHub
<#1373 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7MW4Z525LMTTGDJSY3ZXDH2FAVCNFSM6AAAAABOKR3KJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJXGI4TENBYG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Decision about structure: avoid doing this in ModelBackend and use Secret/Model instead. Tony will try the dict at the class level, if that doesn't seem great, then explicitly wire the secret_cache dict through Model/Application/Unit/Secret. |
The implementation, including ops tests themselves, is ok (commit). For charm unit tests, Scenario can just reset the cache for each I'll have a look at the alternative of passing a cache down to |
No functional changes (including to the tests) - this just adjusts the
imports so that the names from `unittest.mock` are imported into a
`mock` namespace, rather than all into the module namespace.
Primarily, this ensures that `typing.Any` and `unittest.mock.ANY` are
clearly distinguished by more than case, but it's also generally tidier
("Namespaces are one honking great idea").
See discussion in #1373.
Sorry, I jumped the gun on that review. Thanks! |
There was less wiring here than I anticipated, because I forgot we already have
Yes, will do. I didn't with the first version because a |
tonyandrewmeyer
force-pushed
the
set-info-and-content-in-same-hook-1288
branch
from
c9be2ed
to
a179166
Compare
There are no new failures. That's to be expected, I think, because the Harness behaviour was already what we are changing this to, so as long as All of the |
|
I opened a Juju ticket where the Juju team can decide if they want to keep the existing |
Also get rid of mock.ANY - the tests are no longer consistent with the others, but it's not worth the bother.
…ical#1379) No functional changes (including to the tests) - this just adjusts the imports so that the names from `unittest.mock` are imported into a `mock` namespace, rather than all into the module namespace. Primarily, this ensures that `typing.Any` and `unittest.mock.ANY` are clearly distinguished by more than case, but it's also generally tidier ("Namespaces are one honking great idea"). See discussion in canonical#1373.
… hook (canonical#1373) Each call to `secret-set` replaces the values in any previous `secret-set` call in the same hook. Since both `Secret.set_content` and `Secret.set_info` use `secret-set`, this means that doing both in the same hook would only retain the change from whichever was done last. This PR changes the model backend to cache the secret metadata (from `set_info`) and add it into any subsequent `secret-set` calls. Although the metadata is only available to the secret owner, it does not seem so private that it is unsafe to keep it in memory for the duration of the hook execution. There is an additional behaviour change here: previously calling `set_info` twice in the same hook would 'undo' setting metadata. For example: ```python secret.set_info(label="foo") secret.set_info(description="bar") # With main, the secret will end up with a description and no label # With the PR branch, the secret will end up with a description and a label ``` I believe this is a bug fix also, because it seems likely that a charmer would expect both values to be set with code such as the above, and the documentation states: > Once attributes are set, they cannot be unset. For the secret content, I believe we should not cache this in the charm process's memory, so this PR only sets a sentinel that the content has been set, and if there is a subsequent `set_info` call, the content is retrieved via a `secret-get` call (I believe this is from the in-memory cache in the unit agent, but, in any case, it's the un-committed value in an existing location, so does not increase the secret content exposure). There is an example charm in canonical#1288 that can be used to verify the behaviour in a real Juju model. No updates to Harness because it already has the behaviour that we're changing to. Fixes canonical#1288
Each call to
secret-setreplaces the values in any previoussecret-setcall in the same hook. Since bothSecret.set_contentandSecret.set_infousesecret-set, this means that doing both in the same hook would only retain the change from whichever was done last.This PR changes the model backend to cache the secret metadata (from
set_info) and add it into any subsequentsecret-setcalls. Although the metadata is only available to the secret owner, it does not seem so private that it is unsafe to keep it in memory for the duration of the hook execution.There is an additional behaviour change here: previously calling
set_infotwice in the same hook would 'undo' setting metadata. For example:I believe this is a bug fix also, because it seems likely that a charmer would expect both values to be set with code such as the above, and the documentation states:
For the secret content, I believe we should not cache this in the charm process's memory, so this PR only sets a sentinel that the content has been set, and if there is a subsequent
set_infocall, the content is retrieved via asecret-getcall (I believe this is from the in-memory cache in the unit agent, but, in any case, it's the un-committed value in an existing location, so does not increase the secret content exposure).There is an example charm in #1288 that can be used to verify the behaviour in a real Juju model.
No updates to Harness because it already has the behaviour that we're changing to.
Fixes #1288