feat: secret owners do not auto-peek, and can use refresh

[tonyandrewmeyer]

Update the testing harness to reflect an upcoming Juju change regarding secrets: the special-casing for secret-get is going to be removed, so that owners have the same behaviour as everyone else - they get the tracked revision of the secret, and can call peek to get the latest content, or refresh to get the latest content and update the tracking pointer.

Also extends the Harness tests for getting secrets.

Fixes #1062

[tonyandrewmeyer]

@benhoyt it seems like we don't have any tests asserting the previous behaviour (nothing fails when it's removed, and I couldn't manually find any). Do you think we should add tests for the new behaviour, e.g. where the owner uses peek=False or refresh=True?

[tonyandrewmeyer]

@wallyworld just to confirm: the secret owner always has permission to do a secret-get, right? ie. in the testing harness, we're checking that a grant exists for the relevant secret+relation, but skipping that for the owner. Does that sound right to you?

[benhoyt]

@benhoyt it seems like we don't have any tests asserting the previous behaviour (nothing fails when it's removed, and I couldn't manually find any). Do you think we should add tests for the new behaviour, e.g. where the owner uses peek=False or refresh=True?

Because this is now the same for owners and observers, I don't think we need any owner-specific tests for it. (I'm presuming the peek and refresh code paths are tested in general.)

[wallyworld]

@wallyworld just to confirm: the secret owner always has permission to do a secret-get, right? ie. in the testing harness, we're checking that a grant exists for the relevant secret+relation, but skipping that for the owner. Does that sound right to you?

When a secret is created, the owner gets "manage" permission. So they can read the value, as well as grant access etc

[tonyandrewmeyer]

Because this is now the same for owners and observers, I don't think we need any owner-specific tests for it. (I'm presuming the peek and refresh code paths are tested in general.)

We have tests that check that get_secret works when the app/unit has been granted access, and that it fails when no grant exists.

We don't have a test that checks the 'owner has manage permission' case. We also seem to only have tests that provide the ID, so don't check getting by label or updating the label (not even using get_info() - I can't find any harness tests for that?). We don't seem to test the peek or refresh paths.

So I think the conclusion here is that a few extra tests would be a good idea, even if they aren't specifically about this change.

[benhoyt]

Looks good to me. Thanks for the additional tests.

[PietroPasotti]

what juju version will this be?

[benhoyt]

@wallyworld Can you remind us what Juju version(s) the secrets owner fix will be going into?

@tonyandrewmeyer I wonder, maybe we should have a JujuVersion property to allow testing for this (and simply to record the version clearly).

[tonyandrewmeyer]

@tonyandrewmeyer I wonder, maybe we should have a JujuVersion property to allow testing for this (and simply to record the version clearly).

I wondered about this in the original MM thread too. It seems like Juju is treating this as a bug (at least in the way that it's being changed in existing versions), and I don't think we would want to deliberately model behaviour of bugs in specific Juju versions. I don't think we want JujuVersion to end up being a Juju changelog, or to have Charmers running large matrices of tests with different simulated Juju versions.

Otoh, it doesn't seem like a bug to me, and JujuVersion already has checks that go to the patch level.

The default JujuVersion of 0.0.0 isn't ideal here, though. That means the default behaviour would be the old (flawed/buggy) approach, and you'd need to explicitly set the Juju version to get the new (good/fixed) behaviour. Perhaps that's more of an argument for resolving #1037, though.

Overall, I'm -0 on adding a JujuVersion check and switching the behaviour based on that. If we're confident that at least some Chamers would write charms that change their behaviour to handle both Juju versions and have tests to verify that, that'd move me to at least +0.

[benhoyt]

Overall, I'm -0 on adding a JujuVersion check and switching the behaviour based on that

Yeah, you've convinced me -- let's leave that out.

[benhoyt]

Excellent, thanks for this