Skip to content

Conversation

marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Aug 25, 2022

Issue

  • Database charms should enable the human operator to rotate any system user (users that are need for the correct operation of the charm and/or the workload, like a superuser, a replication user, etc.) password.

Solution

  • Create an action to set system users passwords and propagate that password to all the other units.

Context

  • This was already done in a very similar way in Password rotation postgresql-k8s-operator#26.
  • Users created through relations (legacy or new relations) are not covered by this mechanism. Those users will have the password rotated after a sequence of a broken relation and a new relation being established.
  • When updating a password using the set-password action, that password need to be update in the Patroni configuration (which also needs to be reloaded). The configuration is reloaded in the leader unit first and later on the other units (through relation changed event; that event already handles Patroni configuration changes and its reload process). There is no downtime in that processes.
  • The new integration test from tests/integration/test_password_rotation.py rotate the two system users password and checks that they are correctly updated in all the units (which is checked after restarting Patroni; it would trigger an connection error in the Patroni process if the password is not updated).

Testing

  • Unit and integration tests were updated based in the new actions.
  • Additional unit and integration tests were added to validate the password rotation mechanism.

Release Notes

  • Add password rotation mechanism for all system users (users used by the charm/workload and that were not created through relations).

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2022

Codecov Report

Merging #25 (d99bcaf) into main (ceeba02) will increase coverage by 1.61%.
The diff coverage is 77.41%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   60.11%   61.73%   +1.61%     
==========================================
  Files           6        6              
  Lines         692      763      +71     
  Branches      103      118      +15     
==========================================
+ Hits          416      471      +55     
- Misses        256      264       +8     
- Partials       20       28       +8     
Impacted Files Coverage Δ
src/cluster.py 54.81% <50.00%> (+0.33%) ⬆️
src/charm.py 55.85% <78.04%> (+3.67%) ⬆️
src/constants.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@marceloneppel marceloneppel changed the base branch from main to rework-secrets August 26, 2022 13:18
Base automatically changed from rework-secrets to main September 1, 2022 17:19
Copy link
Contributor

@shayancanonical shayancanonical left a comment

Choose a reason for hiding this comment

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

lgtm!!

try:
with self._connect_to_database() as connection, connection.cursor() as cursor:
cursor.execute(
sql.SQL("ALTER USER {} WITH ENCRYPTED PASSWORD '" + password + "';").format(
Copy link
Contributor

Choose a reason for hiding this comment

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

minor suggestion (feel free to ignore): is it possible to use fstrings instead of .format?

Copy link
Member Author

Choose a reason for hiding this comment

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

.format is needed to correctly escape and handle invalid characters in the user name. But I can improve it to user a Placeholder object from psycopg2 to avoid using the plus signal to concat the password. I will do it in the library in the k8s repository along with other improvements and then update it here.

Thanks for the questions and the suggestion Shayan!

Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

Looks good!

Co-authored-by: Raúl Zamora Martínez <76525382+zmraul@users.noreply.github.com>
@marceloneppel marceloneppel merged commit 14bed53 into main Sep 5, 2022
@marceloneppel marceloneppel deleted the password-rotation branch September 5, 2022 17:49
BON4 pushed a commit to BON4/postgresql-operator that referenced this pull request Apr 23, 2024
* Change charm database user

* Fix unit tests

* Fix integration test call

* Fix user name in library

* Fix user

* Add default postgres user creation

* Change action name

* Rework secrets management

* Add password rotation logic

* Add user to the action parameters

* Add pytest marks

* Fix action description

* Fix method docstring

* Fix pytest mark

* Update src/charm.py

Co-authored-by: Raúl Zamora Martínez <76525382+zmraul@users.noreply.github.com>

Co-authored-by: Raúl Zamora Martínez <76525382+zmraul@users.noreply.github.com>
github-actions bot added a commit to canonical/test-runners-2-github-x64-postgresql-operator that referenced this pull request May 18, 2024
github-actions bot added a commit to canonical/test-runners-2-is-x64-postgresql-operator that referenced this pull request May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants