Skip to content

Handle changing root user password #10

Open
patricksimonian wants to merge 3 commits intomasterfrom
postinit-on-default-user
Open

Handle changing root user password #10
patricksimonian wants to merge 3 commits intomasterfrom
postinit-on-default-user

Conversation

@patricksimonian
Copy link

Summary

Some services (like aqua) connect to the default user 'postgres' but with a custom password. Although this behaviour can probably be changed I figured it be nice to handle updating the postgres user with a password.

My bash skills aren't sharp. hope the if statement is correct!

psql "$1" -w -c "create user \"${APP_USER}\" WITH LOGIN ENCRYPTED PASSWORD '${APP_PASSWORD}'"

if [ "$APP_USER" = "postgres" ]; then
echo "Updating postgres user with new password $APP_PASSWORD"
Copy link
Member

Choose a reason for hiding this comment

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

Should we ever be printing out the incoming password into the logs? It's good for debugging I suppose, but this could be an unnecessary secret leak that may get caught and saved by log aggregators downstream.

Copy link
Author

Choose a reason for hiding this comment

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

i totally agree. I felt weird but i noticed patroni had always done that with psql "$1" -w -c "create user \"${APP_USER}\" WITH LOGIN ENCRYPTED PASSWORD '${APP_PASSWORD}'

I should have questioned the statusquo instead of following along

Copy link
Author

Choose a reason for hiding this comment

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

Also @jujaga I was thinking the database creation should be in its own control flow statement. Just incase a user was using the default user and password for postgres? You could still create a database?

echo "Creating user ${APP_USER}"
psql "$1" -w -c "create user \"${APP_USER}\" WITH LOGIN ENCRYPTED PASSWORD '${APP_PASSWORD}'"

if [[ "$APP_USER" = "postgres" ]]; then
Copy link

Choose a reason for hiding this comment

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

This is a little different behaviour than most postgres setups. Most have an POSTGRES_ADMIN_PASSWORD as a separate parameter to change the master user password.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree to separate admin and regular user (the user the application uses).

Copy link
Author

Choose a reason for hiding this comment

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

I like that. I'll make the change.

I do still have on issue when trying to run aqua, since it uses the default user by default, it is impossible to run this image without specifying a user.

Perhaps a check for APP_USER != "postgres" before creating the user is necessary, otherwise we print out a statement saying APP_USER is default user - consider changing

Copy link

Choose a reason for hiding this comment

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

Postgres always has the default master account postgres. A default installation will not assign a password, but will only allow localhost traffic to use the account. When you assign the password during initialization of the db it enables network access to the postgres account.
If you want aqua to use the postgres account, you probably just need to set the password for the account.
If you want aqua to use a different account, you'll probably need to setup that user with the correct permissions.

Copy link
Author

Choose a reason for hiding this comment

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

Understood, but this script attempts to create the user postgres and subsequently fails

Copy link

Choose a reason for hiding this comment

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

Then most likely it should fail in a nice way, as the postgres user will always exist and it shouldn't be attempted to be created.

Copy link
Author

@patricksimonian patricksimonian Nov 26, 2021

Choose a reason for hiding this comment

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

yes exactly. Like this script should not attempt to create a postgres user and instead just use it? Does that make sense?

Copy link

Choose a reason for hiding this comment

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

My preference is to keep it similar to other postgres inits. You're not supposed to create a postgres user. It means the person setting the db up is doing something wrong.

Copy link
Member

@jujaga jujaga Nov 26, 2021

Choose a reason for hiding this comment

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

Look into the environment variables PATRONI_SUPERUSER_USERNAME and PATRONI_SUPERUSER_PASSWORD. IIRC they're generally associated with the postgres superuser account. If those environment variables are specified (passed in via secret mapping on dc or similar) Patroni will use that for account management of postgres user and its associated password if this is a fresh new instance of Patroni being stood up.

Ref: https://patroni.readthedocs.io/en/latest/ENVIRONMENT.html#postgresql

Understanding this correctly, we SHOULD NOT MERGE this new if-else logic for handling/updating the postgres superuser account as there's sanctioned, upstream ENV variables that deal with it already.

Here's a gist summary example of how our team is deploying Patroni on OCP4 (via discrete secret object management and insertion to Patroni env): https://gist.github.com/jujaga/d80693a1c4e0fa990a38ef5d1cd7cdfe

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