Skip to content
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

fix: Secret.set_info and Secret.set_content can be called in the same hook #1373

Conversation

tonyandrewmeyer
Copy link
Contributor

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:

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 #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

ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the fix, however, it seems to me we should do it in class Model, rather than in _ModelBackend. They're supposed to be to 1:1 wrappers around the Juju hook tools. Might be good to discuss during daily.

ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
test/test_model.py Outdated Show resolved Hide resolved
Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/test_model.py Outdated Show resolved Hide resolved
@jameinel
Copy link
Member

also, what is the behavior if you:

secret-set $ID foo=bar
secret-set $ID baz=bing

I think the behavior is that content is replaced, and not additive.

As demonstrated with:

$ juju exec --unit ubuntu/0 "secret-set secret:cr87vfcuth6s6ib4phog foo=bar; secret-set secret:cr87vfcuth6s6ib4phog baz=bing"
$ juju show-secret cr87vfcuth6s6ib4phog --reveal
cr87vfcuth6s6ib4phog:
  revision: 52
  owner: ubuntu-lite
  label: foo
  created: 2024-08-29T14:03:42Z
  updated: 2024-09-17T23:20:21Z
  content:
    baz: bing

Note that this is different from relation-set. Which instead uses:

Setting an empty string causes the setting to be removed. Duplicate settings
are not allowed.

But there if you do:

$ juju exec --unit dummy-source/0 "relation-set -r :0 foo=bar; relation-set -r :0 baz=bing"
$ juju exec --unit dummy-source/0 "relation-get -r :0 - dummy-source/0"
baz: bing
egress-subnets: 10.190.220.126/32
foo: bar
ingress-address: 10.190.220.126
private-address: 10.190.220.126

I don't think it is predictable that secret-set behaves differently than relation-set, but I'm not sure if that ship has sailed.

@jameinel
Copy link
Member

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 secret-set $ID --label=a; secret-set $ID --description=blah does what you would expect:

$ juju exec --unit ubuntu-lite/0 "secret-set secret:cr87vfcuth6s6ib4phog --label='foo'; secret-set secret:cr87vfcuth6s6ib4phog --description='some text'"
$ juju show-secret cr87vfcuth6s6ib4phog --reveal
cr87vfcuth6s6ib4phog:
  revision: 52
  owner: ubuntu-lite
  description: some text
  label: foo
  created: 2024-08-29T14:03:42Z
  updated: 2024-09-17T23:27:38Z
  content:
    baz: bing

The content hasn't been updated, but the label and description are changed.

@tonyandrewmeyer
Copy link
Contributor Author

Though in my testing, calling secret-set $ID --label=a; secret-set $ID --description=blah does what you would expect:

$ juju exec --unit ubuntu-lite/0 "secret-set secret:cr87vfcuth6s6ib4phog --label='foo'; secret-set secret:cr87vfcuth6s6ib4phog --description='some text'"
$ juju show-secret cr87vfcuth6s6ib4phog --reveal
cr87vfcuth6s6ib4phog:
  revision: 52
  owner: ubuntu-lite
  description: some text
  label: foo
  created: 2024-08-29T14:03:42Z
  updated: 2024-09-17T23:27:38Z
  content:
    baz: bing

The content hasn't been updated, but the label and description are changed.

Isn't that because label is special? For example (this is Juju 3.6b1 with LXD, if that matters):

$ juju exec --unit secret-info-and-content-set/5 "secret-set secret:crl1juhtvhl39c5lur90 --expire=3600s; secret-set secret:crl1juhtvhl39c5lur90 --description='some text'"
$ juju show-secret crl1juhtvhl39c5lur90 --reveal
crl1juhtvhl39c5lur90:
  revision: 1
  owner: secret-info-and-content-set/5
  description: some text
  label: my-secret
  created: 2024-09-18T00:08:27Z
  updated: 2024-09-18T00:09:53Z
  content:
    foo: bar

No expiry, but the description is set. And in the other order:

$ juju exec --unit secret-info-and-content-set/5 "secret-set secret:crl1juhtvhl39c5lur90 --description='some other text'; secret-set secret:crl1juhtvhl39c5lur90 --expire=1800s;"
$ juju show-secret crl1juhtvhl39c5lur90 --reveal
crl1juhtvhl39c5lur90:
  revision: 1
  expires: 2024-09-18T00:41:10Z
  owner: secret-info-and-content-set/5
  description: some text
  label: my-secret
  created: 2024-09-18T00:08:27Z
  updated: 2024-09-18T00:11:10Z
  content:
    foo: bar

The description hasn't changed, but the expiry is set. Compared to the single call where both update:

$ juju exec --unit secret-info-and-content-set/5 "secret-set secret:crl1juhtvhl39c5lur90 --description='some other text still' --expire=900s;"
$ juju show-secret crl1juhtvhl39c5lur90 --reveal
crl1juhtvhl39c5lur90:
  revision: 1
  expires: 2024-09-18T00:28:10Z
  owner: secret-info-and-content-set/5
  description: some other text still
  label: my-secret
  created: 2024-09-18T00:08:27Z
  updated: 2024-09-18T00:13:10Z
  content:
    foo: bar

@jameinel
Copy link
Member

No expiry, but the description is set. And in the other order:

sigh... that seems quite inconsistent. Do we have a bug on Juju about this?
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.

@tonyandrewmeyer
Copy link
Contributor Author

I like the fix, however, it seems to me we should do it in class Model, rather than in _ModelBackend.

The catch with that is that Model doesn't do secret-set calls, Secret does. Secret does not currently have a link to a Model object, only to a _ModelBackend. If we want this done in Model, then that would mean adjusting Secret to get access to a Model object (which would also mean adjusting Unit and Application, which create Secret objects).

We can't do it in individual Secret objects because you could have two Secret objects that are the same underlying secret and they should also presumably not clobber each other. I guess it could be done at the Secret class level?

They're supposed to be to 1:1 wrappers around the Juju hook tools.

This is a pretty convincing argument.

Might be good to discuss during daily.

Sure :). Also about whether we should be caching content when setting and John's comments :).

@tonyandrewmeyer
Copy link
Contributor Author

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 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 :).

@jameinel
Copy link
Member

jameinel commented Sep 18, 2024 via email

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 18, 2024

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.

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Sep 18, 2024

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 run. Harness is much trickier, because there isn't really a defined way of saying "this is a new Juju hook execution". We could reset each time you make a new Harness object (or each time the charm is instantiated, which is roughly the same) but it does increase the behaviour that is unexpectedly different from running against an actual Juju. I'm concerned that this would end up strangely leaking secret info/metadata between tests, although maybe the answer is to just make a new Harness every time you want a new Juju hook execution.

I'll have a look at the alternative of passing a cache down to Application and Unit and through those to Secret.

tonyandrewmeyer added a commit that referenced this pull request Sep 18, 2024
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.
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks! Can we make sure to super-tox this?

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 18, 2024

I'll have a look at the alternative of passing a cache down to Application and Unit and through those to Secret.

Sorry, I jumped the gun on that review. Thanks!

@tonyandrewmeyer
Copy link
Contributor Author

I'll have a look at the alternative of passing a cache down to Application and Unit and through those to Secret.

There was less wiring here than I anticipated, because I forgot we already have _ModelCache, and it seems reasonable to me to store the cache there. I think this version is much better.

Can we make sure to super-tox this?

Yes, will do. I didn't with the first version because a _ModelBackend change wouldn't change anything for unit tests, but this change potentially does.

@tonyandrewmeyer tonyandrewmeyer force-pushed the set-info-and-content-in-same-hook-1288 branch from c9be2ed to a179166 Compare September 18, 2024 08:27
@tonyandrewmeyer
Copy link
Contributor Author

Can we make sure to super-tox this?

Yes, will do. I didn't with the first version because a _ModelBackend change wouldn't change anything for unit tests, but this change potentially does.

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 Model is now doing what Harness already did, the test behaviour isn't going to change.

All of the set_info use I can find is in the tls-certificates lib, and that does do both set_content and set_info together (although perhaps not in an active code path, based on the comment from #1288?). As far as I can tell, the existing code will not work correctly, and this change will make it work correctly, but I haven't found any tests around it (and if there were tests, then you'd think that someone would have noticed this already).

@tonyandrewmeyer
Copy link
Contributor Author

I opened a Juju ticket where the Juju team can decide if they want to keep the existing secret-set behaviour or improve it to match the new ops behaviour.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall -- couple of potential minor issues.

ops/model.py Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a request for change, rather a critical question about juju agent behaviour

test/test_model.py Outdated Show resolved Hide resolved
test/test_model.py Outdated Show resolved Hide resolved
ops/model.py Show resolved Hide resolved
Also get rid of mock.ANY - the tests are no longer consistent with the others, but it's not worth the bother.
@tonyandrewmeyer tonyandrewmeyer merged commit 7dec772 into canonical:main Sep 25, 2024
29 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the set-info-and-content-in-same-hook-1288 branch September 25, 2024 06:50
tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this pull request Oct 4, 2024
…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.
tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this pull request Oct 4, 2024
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot call Secret.set_info and Secret.set_content in the same hook
4 participants