Skip to content

[MISC] Conditional password #604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 31, 2024
Merged

[MISC] Conditional password #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98cfde5
Conditional password
dragomirp Aug 30, 2024
0aafbec
Revert dependency bump
dragomirp Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions src/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ def is_creating_backup(self) -> bool:
for member in r.json()["members"]
)

def is_replication_healthy(self, raft_encryption: bool = False) -> bool:
def is_replication_healthy(self) -> bool:
"""Return whether the replication is healthy."""
try:
for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3)):
Expand All @@ -427,13 +427,6 @@ def is_replication_healthy(self, raft_encryption: bool = False) -> bool:
member_status = requests.get(
f"{url}/{endpoint}", verify=self.verify, auth=self._patroni_auth
)
# If raft is getting encrypted some of the calls will fail
if member_status.status_code == 503 and raft_encryption:
logger.warning(
"Failed replication check for %s during raft encryption"
% members_ip
)
continue
if member_status.status_code != 200:
logger.debug(
"Failed replication check for %s with code %d"
Expand Down
10 changes: 1 addition & 9 deletions src/upgrade.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,6 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None:

self.charm._setup_exporter()
self.charm.backup.start_stop_pgbackrest_service()
raft_encryption = (
int(
json.loads(self.peer_relation.data[self.charm.app].get("dependencies", "{}"))
.get("charm", {})
.get("version", 0)
)
< 3
)
Comment on lines -161 to -168
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the 503 errors were API related not RAFT related.


try:
self.charm.unit.set_workload_version(
Expand All @@ -185,7 +177,7 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None:
not self.charm._patroni.member_started
or self.charm.unit.name.replace("/", "-")
not in self.charm._patroni.cluster_members
or not self.charm._patroni.is_replication_healthy(raft_encryption)
or not self.charm._patroni.is_replication_healthy()
):
logger.debug(
"Instance not yet back in the cluster."
Expand Down
2 changes: 2 additions & 0 deletions templates/patroni.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ log:
restapi:
listen: '{{ self_ip }}:8008'
connect_address: '{{ self_ip }}:8008'
{%- if patroni_password %}
authentication:
username: patroni
password: {{ patroni_password }}
{%- endif %}
Comment on lines +24 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the leader elected executes after the template is rendered we'll be stuck with None pass and will be unable to reload via the REST API

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen in wild? What is our answer there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but when we add rotation (DPE-5270) we should switch to SIGHUPing the service instead of reloading via REST.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic question: At which point during charm execution the patroni template is rendered?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 @lucasgameiroborges good to have this answer in https://charmhub.io/postgresql-k8s/docs/e-flowchart-charm for VM charm :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic question: At which point during charm execution the patroni template is rendered?

I don't think there's a set order, especially during charm's bootstrap.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might ask @marceloneppel once hes back, might be good to have this clear and documented somewhere (maybe on the diagrams!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the issue might be happening at

postgresql-operator/src/upgrade.py

Line 149 in 5f546f1

self.charm.update_config()
, when a non-leader unit is the first one to be updated.

The changes from this PR LGTM.

{%- if enable_tls %}
cafile: {{ conf_path }}/ca.pem
certfile: {{ conf_path }}/cert.pem
Expand Down
5 changes: 0 additions & 5 deletions tests/unit/test_cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,11 +235,6 @@ def test_is_replication_healthy(peers_ips, patroni):
]
assert not patroni.is_replication_healthy()

# Test ignoring errors in case of raft encryption.
_get.side_effect = None
_get.return_value.status_code = 503
assert patroni.is_replication_healthy(True)


def test_is_member_isolated(peers_ips, patroni):
with (
Expand Down
Loading