fix: use cached leadership in security logging, renew lease on success by cristiangirlea · Pull Request #2778 · canonical/operator

cristiangirlea · 2026-09-29T05:55:26Z

Reporting a failed hook command as a security event called is_leader(), which could run the is-leader hook command while handling the original failure. If is-leader itself failed with an authorisation error and no leadership status was cached, this recursed until a RecursionError.

The security event now uses the leadership status that ops already has cached, through a new _cached_leadership() helper that is_leader() shares, so the lease rule is written once. This also covers failures in juju_log(). When there is no valid cached status, the event says "(leadership unknown)" rather than reading the same as a known non-leader.

is_leader() also stored the lease time before running is-leader, so a failed refresh kept the previous value with a fresh timestamp. The time is still taken before the query, and is now stored only once it succeeds.

No exception message changes: str(e) and args[0] of ModelError are still Juju's stderr.

Pass the existing leadership state in to the security logging, rather than running is-leader while reporting a failed hook command. Only store the refreshed leadership lease time when is-leader succeeds.

james-garner-canonical

Thanks, this looks good to me.

james-garner-canonical · 2026-09-29T06:11:58Z

        if self._is_recursive.get():
            # Either `juju-log` hook command failed or there's a bug in ops.
            return
+        is_leader = None


Suggested change

is_leader = None

# Save known leadership status at time of calling the hook command,

# for use in security event logging if the hook command fails.

is_leader = None

james-garner-canonical · 2026-09-29T06:16:07Z

            return
        base_cmd = os.path.basename(cmd)
-        leadership = ' (as leader)' if self.is_leader() else ''
+        leadership = ' (as leader)' if is_leader else ''


I think this is a decent behaviour change -- we don't call is-leader on every authorization related error, which is a small win for performance, and arguably for correctness too on two fronts: 1) we avoid potentially failing in error handling on another hook command call, and 2) it's sensible to report the known leadership status at the time we called the hook command rather than afterwards.

Ops will have checked if we were the leader on events where we expect that to matter (so it will be cached in that case), and leadership doesn't matter for other permission errors (e.g. querying a secret or relation ID that we don't have access to).

WDYT @tonyandrewmeyer?

If the lease has expired (or is_leader() wasn't called in this hook), is_leader is None and the log reads exactly the same as for a known non-leader. Since this is the security audit log, I think it would be better to distinguish those, maybe with something like (leadership unknown) for None?

Added (leadership unknown) when there is no valid cached status.

james-garner-canonical · 2026-09-29T06:22:52Z

+        query = mock.Mock(side_effect=is_leader)
+        monkeypatch.setattr(hookcmds, 'is_leader', query)
+        assert backend.is_leader() is True
+        assert backend._leader_check_time == 100


Suggested change

assert backend._leader_check_time == 100

assert query.call_count == 1

assert backend._leader_check_time == 100

assert backend._leader_check_time != now

james-garner-canonical · 2026-09-29T06:23:09Z

+        assert backend._leader_check_time == 100
+        now = 130
+        assert backend.is_leader() is True
+        assert query.call_count == 1


Suggested change

assert query.call_count == 1

assert query.call_count == 1

assert backend._leader_check_time == 100

assert backend._leader_check_time != now

james-garner-canonical · 2026-09-29T06:26:42Z

+        now = 131
+        assert backend.is_leader() is True
+        assert query.call_count == 2
+        assert backend._leader_check_time == 131


Suggested change

assert backend._leader_check_time == 131

assert backend._leader_check_time == 131

assert backend._leader_check_time != now

james-garner-canonical · 2026-09-29T06:27:19Z

+        'cached,age,as_leader',
+        [
+            (True, 0, True),
+            (True, 30, True),


This is quite funny on our part really isn't it, it's just asking for a leadership change out from under you ...

tonyandrewmeyer · 2026-09-30T08:58:00Z

            return
        base_cmd = os.path.basename(cmd)
-        leadership = ' (as leader)' if self.is_leader() else ''
+        leadership = ' (as leader)' if is_leader else ''


If the lease has expired (or is_leader() wasn't called in this hook), is_leader is None and the log reads exactly the same as for a known non-leader. Since this is the security audit log, I think it would be better to distinguish those, maybe with something like (leadership unknown) for None?

Move the lease check into a _cached_leadership() helper shared by is_leader() and _check_for_security_event(), so the two can't drift and juju_log() failures are covered too. Report "(leadership unknown)" in the security event when there is no valid cached status, and add a test for the is-leader authz failure that used to recurse.

cristiangirlea · 2026-09-30T09:29:16Z

Thanks Tony, all addressed in a53b128. One note on the history: my first cut of this PR had a shared helper like _cached_leadership(), but I trimmed it down to strictly the two changes you and James listed, so the helper went with it. Now that you've asked for it, it's back, and it also fixed the recursion case you spotted.

tonyandrewmeyer

Thanks! Could you please update the PR body? It gets used as the body of the squash commit, so it's important that it's up-to-date and accurate.

cristiangirlea · 2026-10-01T08:45:08Z

Updated the PR title and body to match the final change.

fix: pass known leadership to security logging, renew lease on success

f1e6f93

Pass the existing leadership state in to the security logging, rather than running is-leader while reporting a failed hook command. Only store the refreshed leadership lease time when is-leader succeeds.

cristiangirlea mentioned this pull request Sep 29, 2026

fix: include Juju operation context in model errors #2749

Closed

james-garner-canonical requested review from james-garner-canonical and tonyandrewmeyer and removed request for james-garner-canonical September 29, 2026 05:57

james-garner-canonical approved these changes Sep 29, 2026

View reviewed changes

chore: apply review suggestions

6874e71

tonyandrewmeyer reviewed Sep 30, 2026

View reviewed changes

cristiangirlea and others added 2 commits September 30, 2026 12:08

Merge branch 'main' into fix/leadership-cache-security-log

c4c7018

tonyandrewmeyer approved these changes Sep 30, 2026

View reviewed changes

Merge branch 'main' into fix/leadership-cache-security-log

fb649b5

cristiangirlea changed the title ~~fix: pass known leadership to security logging, renew lease on success~~ fix: use cached leadership in security logging, renew lease on success Oct 1, 2026

Merge branch 'main' into fix/leadership-cache-security-log

132f9b5

tonyandrewmeyer merged commit da64263 into canonical:main Oct 5, 2026
60 checks passed

This was referenced Oct 5, 2026

chore: update changelog and versions for 3.8.4 release tonyandrewmeyer/operator#60

Merged

chore: update changelog and versions for 3.9.0 release #2806

Merged

	assert backend._leader_check_time == 131
	assert backend._leader_check_time == 131
	assert backend._leader_check_time != now

Conversation

cristiangirlea commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-garner-canonical left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristiangirlea commented Sep 30, 2026

Uh oh!

tonyandrewmeyer left a comment

Choose a reason for hiding this comment

Uh oh!

cristiangirlea commented Oct 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cristiangirlea commented Sep 29, 2026 •

edited

Loading