feat(config): add peer-state config/user hash + tls unit accessors#181
Draft
marceloneppel wants to merge 12 commits into
Draft
feat(config): add peer-state config/user hash + tls unit accessors#181marceloneppel wants to merge 12 commits into
marceloneppel wants to merge 12 commits into
Conversation
Introduce the low-level building blocks that the operator-certificate TLS flow composes on top of, so they can be reviewed on their own, ahead of the manager, events handler and charm wiring that consume them. This covers the peer-relation databag accessors for CA rotation (current-ca/old-ca), the client-facing database-address, and the K8s-shaped peer address set that omits the ip key for parity with the pre-migration K8s charm; the workload file-ownership/mode primitives (user/group and the substrate-specific tls_file_mode, 0o600 on VM and 0o400 on K8s) plus user/group forwarding through write_text so TLS material is chowned correctly on both substrates; a per-substrate tls path; the TLS relation-name constants; and the unit-test harness fixture the later branches' TLS tests depend on. The change is purely additive: the existing charm still constructs unchanged and the current unit suite is unaffected, which keeps this branch a safe, self-contained foundation for the stack. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The TLS stack had overridden the K8s patroni_conf path to /etc/patroni, a vestige of an earlier TLS-hardening lineage. #172's Patroni port renders patroni.yaml at patroni_conf, so it must stay the data storage root (/var/lib/pg/data); the override made a consuming charm run 'patroni /etc/patroni/patroni.yaml' against a config rendered elsewhere. TLS writes its .pem files to the separate 'tls' path (also the data dir) and does not read patroni_conf, so this revert is TLS-safe. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Layer the certificate-SAN and common-name policy onto CharmState, on top of the raw peer-databag accessors from the previous branch, so the substrate-specific certificate identity is reviewable as a unit with its own tests before any manager or handler consumes it. K8s must regain the parity the migration had dropped: common_hosts has to advertise the primary/replicas Service FQDNs and the resolved pod FQDN, and the operator-cert common name has to be the endpoints FQDN (wildcarded past the 64-char CN limit) rather than the VM-style host/address; the peer SAN set must exclude the ip key the original K8s charm never emitted. VM behaviour is left host/address-derived as before. The CharmState charm parameter is also widened to ops.CharmBase so the state object no longer depends on the concrete charm type. These accessors are additive and only read state, so the existing charm keeps constructing unchanged. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…cstring The 'migration had switched this to VM-style ...; restore the endpoints-FQDN CN for K8s parity' sentence narrates a completed regression fix and is redundant with the preceding 'Matches the original K8s charm' line. The format, wildcard rule, and parity rationale already carry the load-bearing context; the migration history belongs in the original commit message, not the docstring. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Wire the operator-certificate TLSManager and TLS events handler into the lib charm and bump the library version. The manager fetches operator cert/key live from the tls_certificates V4 requirers (constructor-injected by the handler); only the peer CA is tracked in state for rotation. Unit tests for this layer land in the stacked tests PR. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Cover TLSManager live-fetch getters, CA rotation, push, and the TLS events handler (certificate_available / relation_broken / defer paths). Stacked on the manager+events PR whose code these exercise. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The 4/4 test docstrings pointed at postgresql-operator/src/relations/tls.py file/line positions and a '16/edge secrets, kept' track label — both stale-prone the moment that repo refactors or the track shifts, with no test catching the drift. Replace with conceptual references to the pre-port charm's defer guards and the state-tracked rotation, which survive refactors; git blame recovers the exact position when needed. Also soften test_relation_broken_client_wired's docstring: it asserts only that removal doesn't crash and the live getter reports absent, not that the event routes to _on_certificate_available (the peer variant verifies routing via the CA side-effect; the client route has no state side-effect to diff on). No test logic changes; suite stays 56/56 green on both substrates. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…ests Lift the requirer-return-shape fakes (FakePrivateKey + fake_assigned) out of the two TLS test files into a shared tests/unit/tls_helpers.py, and add a patch_crypto conftest fixture that stubs generate_private_key/generate_ca with sentinels. The incidental set_leader paths were running real RSA keygen purely to populate internal-ca; the dedicated generate_* tests keep their own sentinels to assert call args, so this only touches paths that need the CA present, not its content. Suite runtime drops 4.2s -> 1.9s. Three tests sat below the patroni/config bar and are tightened, none tautological after the change (each mutation-confirmed to fail when the pinned behavior breaks): - test_tls_manager_wired_to_handler_requirers (was constructor_injects_requirers): drop the two asserts that only verified `self.x = x` plumbing; keep the real wiring asserts (charm injects the handler's requirers into the manager). - test_peer_relation_changed_emits_refresh (was refresh_event_defined_and_wired): drop the hasattr tautology + assertion-less relation_changed smoke; patch TLS.refresh_tls_certificates_event and assert emit() was called once. - test_relation_broken_client_routes_to_live_push (was relation_broken_client_wired): the client route has no CA side-effect, so the old test only proved it didn't crash; now set leader + seed a peer CA, remove the client relation, and assert the push was reached while peer CA rotation stayed untouched — proving the route is the client path, not the peer path. No source or other test changes; the two pre-existing test_patroni_manager.py daemon_reload failures are unrelated (charmlibs.systemd symbol not resolving in this env) and untouched here. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The previous round left test_tls_manager_wired_to_handler_requirers asserting only
`manager.{client,peer}_certificate is tls.{client,peer}_certificate` — the exact two
checks test_handler_is_wired in the events file already makes (plus the not-None
construction asserts). The charm-wiring concern has one home (the events file, since
wiring is a charm __init__ concern); the manager-side copy added nothing. Delete it.
generate_internal_peer_cert had two raise TlsError guards — "No CA key content" and
"No CA cert content" — exercised by no test (the stores_material test seeds both
secrets). Add pytest.raises coverage for each: the CA-key guard raises before any
crypto runs (no leader, no secrets); the CA-cert guard runs after PrivateKey.from_string,
so it stubs that and drops only the CA cert. Both mutation-confirmed (removing the guard
makes each test fail with DID-NOT-RAISE).
Also give test_generate_internal_peer_ca_stores_secrets the patch_crypto fixture and drop
its now-redundant inner generate_* patches (patch_crypto stubs the same symbols with the
same sentinels for both the set_leader side-effect and the explicit call) — it had been
running real RSA during set_leader, then overwriting it with sentinels.
No source changes; suite is 58 TLS tests on both substrates.
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Each dropped test was a second layer over a code path another test already pins; keeping both is the two-layers-that-sort-of-do smell. None of the five guarded a behavior the surviving suite doesn't. - test_rotate_peer_ca_sets_current: first-set + old-ca-None. Covered by no_rotation_when_unchanged's first call (sets current, leaves old None) and the else-branch remove_secret in clears_stale_old_ca_on_reenable. The rotation state machine stays fully covered (rotate, no-op, clear, re-enable-clear). - test_get_client_tls_files_none_when_absent: the absent getter path. Covered by test_certificate_available_pushes_on_empty, which asserts get_client_tls_files == (None, None, None) on an empty requirer. - test_client_and_peer_requesters_have_distinct_common_names: the CN values live in test_tls_state.py (three tests); requester construction is in test_handler_is_wired. - test_peer_certificate_available_defers_when_internal_ca_absent and test_peer_certificate_available_defers_on_workload_file_error: the defer guards live in the shared _push_tls_files. The client variant proves each guard; the peer handler's routing through _push_tls_files is proven by test_peer_certificate_available_rotates_ca_and_pushes. The two surviving defer tests now note they cover the peer path too (shared guard). Suite: 58 -> 48 TLS tests on both substrates. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Scaffolds the config-subsystem port into the library: a later change persists the applied config/user hashes and the last-rendered TLS flag to peer relation data so the update_config manager can detect drift without re-deriving it from live workload state. Add the plain-databag accessors now so that change only wires them up. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
The
update_configmigration needs the config/user hash and the TLS flag to round-trip through the peer databag, as they do in both charms. This first step adds those accessors so later steps in the stack can read and persist them.Solution
Add plain-databag accessors on the peer-relation state, mirroring the existing
ip/is_connectivity_enabledpattern:PostgreSQLPeer.config_hash/user_hash(str | None) andPostgreSQLApplication.user_hash.PostgreSQLPeer.tls(bool) — getter reads"enabled", setter writes"enabled"/"".First PR in the update_config stack (base
tls-4-tests, #175).Checklist