[DPE-8395] Remove old revision of secret

[marceloneppel]

Issue

The charm does not remove the old revisions of a secret after updating it.

Solution

Implement the handler for the secret-remove event. juju/juju#20794 is taken into account.

Implement the removal of the charm secrets' old revision in the upgrade logic. It's possible that not all the old revisions will be removed in an environment due to juju/juju#20782. For example, if the latest revision of a secret is 10 and there is an old one with the revision number equal to 1, that old revision won't be removed because its number is the prefix of the latest one.

Fixes #1180.

Checklist

• I have added or updated any relevant documentation.
• I have cleaned any remaining cloud resources from my accounts.

[codecov]

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.73%. Comparing base (d6154c2) to head (fec93f2).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/charm.py	40.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   75.76%   75.73%   -0.04%     
==========================================
  Files          16       16              
  Lines        4163     4170       +7     
  Branches      629      629              
==========================================
+ Hits         3154     3158       +4     
- Misses        789      792       +3     
  Partials      220      220              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jameinel]

When I was testing an implementation like this with my own charms, I ran into an issue with juju 3.6.9. We have a bugfix prepared that should make this implementation ok, but we might want to be defensive in how we write our charm.
juju/juju#20796 is the bug fix and
juju/juju#20794 is the issue.

Specifically, the issue is that older versions of juju, when a secret is fully-removed, will trigger secret-remove with the old revisions of the secret. However, that secret no longer exists, so that will cause the hook to fail, because it tries to delete something that doesn't exist.

I do see that you have some code to try and handle some of that:

    def remove(self, label: str) -> None:
        """Remove a secret from the cache."""
        if secret := self.get(label):
            try:
                secret.remove()
                self._secrets.pop(label)
            except (SecretsUnavailableError, KeyError):
                pass
            else:
                return
        logging.debug("Non-existing Juju Secret was attempted to be removed %s", label)

However, in my testing, because Juju tries to make the removal of a secret revision transaction consistent, it doesn't actually do anything until the hook completes, at which point, the charm has no way of resolving it (hence the above bug).

The fix that I did in my test charm was:

val=`secret-get $JUJU_SECRET_ID`
if [ -n "$val" ] ; then
  secret-remove --revision $JUJU_SECRET_REVISION $JUJU_SECRET_ID;
else
  juju-log -l WARNING secret $JUJU_SECRET_ID already deleted;
fi

I'm not a huge fan of the workaround. (Not least of which, it means that your application now becomes an observer of the charm content, so it will also get secret-changed events. I suppose you might be able to use --peek? I'm not sure if that sets you as an observer.)

[marceloneppel]

Thank you so much, @jameinel, for your comments in this PR. I'm now handling the issue from your above comment through the check added on a6c36bb.

[jameinel]

This also won't work.
I tested this with just manually running hooks, and juju "queues up" a single revision to be removed. So if you just do:

juju exec --unit u/0 -- secret-remove --revision 1 $secret; secret-remove --revision 2 $secret

Then juju will only remove revision 2.

[jameinel]

I filed a bug about this behavior:
juju/juju#20805
and a PR to fix it:
juju/juju#20806

I don't know how you would work around this fact for existing versions of juju, as without the PR, you can only delete 1 revision per hook event.

[marceloneppel]

I noticed a different behaviour between the approach you commented about and the one that uses /usr/bin/juju-exec u/0 -- secret-remove --revision 1 $secret; /usr/bin/juju-exec u/0 -- secret-remove --revision 2 $secret from inside the unit SSH or ops hook context.

The latter approach "queues up" all the revisions to be removed.

[marceloneppel]

I removed this upgrade logic on fec93f2 after syncing with @taurus-forever and agreeing that it's safer not to have it in the charm.

[marceloneppel]

@taurus-forever, I removed the upgrade logic on fec93f2 and tested the remaining workaround in the secret-remove event handler in the following Juju versions.

• 3.6.11 (built from commit 37e3e7fec0dd8f287e55f03b4055bad2a927697e - coming from fix: make secret-remove hook command idempotent juju/juju#20796 - mentioned on [DPE-8395] Remove old revision of secret #1195 (comment))
• 3.6.9
• 3.5.7
• 3.4.6
• 3.3.7
• 3.2.4
• 3.1.10

It's working on all of them (the revisions are correctly removed, and no error happens when a secret is removed entirely, and the charm runs the secret-remove event handler).

[taurus-forever]

@dragomirp let's merge this to deploy PS6 and see the real production behavior in the test model for 14/edge. Tnx!

Also, @jameinel is not reachable till the end of the week, so assuming his comments have been addressed in full in the latest commits. John, please share your ACK/NACK once you are reachable.

For the history: due to the stable release time pressure and the list of Juju issues with secrets removal, we are NOT going to remove ALL old secrets on charm refresh (at least for now), as the code there was risky longterm. Therefor this PR removes the PREVIOUS revision only (once new secret revision has been applied). It will stop secrets grow.

However, manually cleanup will be necessary from Juju controller for all old revision. It will be necessary once only.

[taurus-forever]

Fixed for 14/edge (merged) and porting to PG16 VM and PG K8s 14+16 (and PGB).

P.S. See the last messages:
#1195 (comment)
#1195 (review)