-
Notifications
You must be signed in to change notification settings - Fork 22
[DPE-5269] Add passwords to Syncobj and Patroni #596
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.
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
==========================================
- Coverage 70.89% 70.85% -0.04%
==========================================
Files 12 12
Lines 3030 3043 +13
Branches 536 538 +2
==========================================
+ Hits 2148 2156 +8
- Misses 768 771 +3
- Partials 114 116 +2 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Mykola Marzhan <[email protected]>
@@ -27,7 +27,7 @@ get-password: | |||
username: | |||
type: string | |||
description: The username, the default value 'operator'. | |||
Possible values - operator, replication, rewind. | |||
Possible values - operator, replication, rewind, patroni. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users and tests need to be able to get at least the patroni password.
for key in ( | ||
USER_PASSWORD_KEY, | ||
REPLICATION_PASSWORD_KEY, | ||
REWIND_PASSWORD_KEY, | ||
MONITORING_PASSWORD_KEY, | ||
RAFT_PASSWORD_KEY, | ||
PATRONI_PASSWORD_KEY, | ||
): | ||
if self.get_secret(APP_SCOPE, key) is None: | ||
self.set_secret(APP_SCOPE, key, new_password()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop to reduce McCabe.
@@ -164,6 +174,7 @@ def cluster_members(self) -> set: | |||
f"{self._patroni_url}/{PATRONI_CLUSTER_STATUS_ENDPOINT}", | |||
verify=self.verify, | |||
timeout=API_REQUEST_TIMEOUT, | |||
auth=self._patroni_auth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GETs are usually open, but adding for consistency.
if self.charm.unit.is_leader(): | ||
return self.charm.model.app.add_secret(content=shared_content, label=SECRET_LABEL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started hitting this on follower units
if self.charm.unit.is_leader() and self.charm._peers: | ||
for key in ( | ||
RAFT_PASSWORD_KEY, | ||
PATRONI_PASSWORD_KEY, | ||
): | ||
if self.charm.get_secret(APP_SCOPE, key) is None: | ||
self.charm.set_secret(APP_SCOPE, key, new_password()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct place for generating the passes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's ok to put it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for the quick fix!
Let's wait @marceloneppel before merging.
P.S. we need to backport Patroni part to K8s charm :-(
if self.charm.unit.is_leader() and self.charm._peers: | ||
for key in ( | ||
RAFT_PASSWORD_KEY, | ||
PATRONI_PASSWORD_KEY, | ||
): | ||
if self.charm.get_secret(APP_SCOPE, key) is None: | ||
self.charm.set_secret(APP_SCOPE, key, new_password()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @dragomirp!
if self.charm.unit.is_leader() and self.charm._peers: | ||
for key in ( | ||
RAFT_PASSWORD_KEY, | ||
PATRONI_PASSWORD_KEY, | ||
): | ||
if self.charm.get_secret(APP_SCOPE, key) is None: | ||
self.charm.set_secret(APP_SCOPE, key, new_password()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's ok to put it here.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still executing to have warnings in the log
raft_encryption = ( | ||
int( | ||
json.loads(self.peer_relation.data[self.charm.app].get("dependencies", "{}")) | ||
.get("charm", {}) | ||
.get("version", 0) | ||
) | ||
< 3 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for versions that don't encrypt RAFT. Seems to be only an issue for 1 (current 14/stable
) but adding both previous version just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you!
Closes #27