Secret ownership fixes

[PietroPasotti]

• uniform Secret.owner = app | Secret.granted = app
• validate ownership on write paths and grantedness on read paths

[PietroPasotti]

todo: figure out if 'owner=app' means something different in terms of who can read and what non-leaders can read.

[benhoyt]

See also canonical/operator#1067. Juju is changing how secret-get works (for secrets owners to work more like secret observers with "refresh" and so on). We're updating ops to that approach (ahead of the Juju change).

[PietroPasotti]

@benhoyt added a switch to toggle that behaviour if juju_version<3.2
@tonyandrewmeyer not sure how to generalize this and future-proof for upcoming juju changes. Maybe in time it'd pay off to have separate _MockModelBackend classes per juju (major) versions?

[tonyandrewmeyer]

Maybe in time it'd pay off to have separate _MockModelBackend classes per juju (major) versions?

The problem I see with that is that it seems like Juju isn't really semver? If it was, then the "owner always peeks" removal would be going into 4.0 and not any 3.x. But if I understand correctly, it's not even being introduced in a minor version but in multiple patch ones. It's before my time, but it looks like open-port on k8s was added in a patch not minor, and adding a Pebble version with exec context was added in a patch not minor.

I do think it would be great to have something better than having lots of if simulated_juju_version complexity scattered around. Not sure yet what that would be.

[PietroPasotti]

Maybe in time it'd pay off to have separate _MockModelBackend classes per juju (major) versions?

The problem I see with that is that it seems like Juju isn't really semver? If it was, then the "owner always peeks" removal would be going into 4.0 and not any 3.x. But if I understand correctly, it's not even being introduced in a minor version but in multiple patch ones. It's before my time, but it looks like open-port on k8s was added in a patch not minor, and adding a Pebble version with exec context was added in a patch not minor.

I do think it would be great to have something better than having lots of if simulated_juju_version complexity scattered around. Not sure yet what that would be.

Let's keep it in the back of our heads, something will come up eventually

[PietroPasotti]

@benhoyt @tonyandrewmeyer I refactored the access control logic based on my latest findings

It should be correct now.

[PietroPasotti]

One more thing:
this work has revealed that we can simplify considerably the Secret data model: granted can go.
First I realized that there was no difference in view/manage permissions depending on whether the (remote) owner has granted the secret to our app or our unit, since secret-grant [app] is equivalent to: for unit in app.units: secret-grant [unit].

Then I realized we can strip that whole concept completely: it doesn't make sense to put a Secret in State.secrets if we're not entitled to know it exists (i.e. if we don't own it nor we've been granted access to it). So the mere presence of a Secret in State.secrets tells us we're entitled to view it, either because our unit owns it, or our app owns it, or we've been granted it.
If we hadn't been granted it, it wouldn't be part of our State.secrets: the exhaustive list of secrets we are aware of.

This is a BC change so far as the API is concerned, I added a deprecation notice for Secret.granted.
However, some tests may break since the idea used to be: put a Secret in State.secrets and make it 'not granted', and now the behaviour will be different.

@tonyandrewmeyer major vbump?

[tonyandrewmeyer]

Then I realized we can strip that whole concept completely: it doesn't make sense to put a Secret in State.secrets if we're not entitled to know it exists (i.e. if we don't own it nor we've been granted access to it). So the mere presence of a Secret in State.secrets tells us we're entitled to view it, either because our unit owns it, or our app owns it, or we've been granted it. If we hadn't been granted it, it wouldn't be part of our State.secrets: the exhaustive list of secrets we are aware of.

👍

This is a BC change so far as the API is concerned, I added a deprecation notice for Secret.granted. However, some tests may break since the idea used to be: put a Secret in State.secrets and make it 'not granted', and now the behaviour will be different.

@tonyandrewmeyer major vbump?

Imo, yes.