Skip to content
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

Jump to bottom

PATRONI_LOG_* environment variables are ignored. #718

Open
mhaley-tignis opened this issue Apr 14, 2022 · 7 comments · May be fixed by #719
Open

PATRONI_LOG_* environment variables are ignored. #718

mhaley-tignis opened this issue Apr 14, 2022 · 7 comments · May be fixed by #719

Comments

@mhaley-tignis
Copy link

It seems that most environment variables are removed before starting the patroni process. I was attempting to configure patroni to log to a directory (instead of stdout) by using the PATRONI_LOG_DIR environment variable, but the runit/patroni/run script seems to be working against me here:

# Only small subset of environment variables is allowed. We don't want accidentally disclose sensitive information
for E in $(printenv -0 | tr '\n' ' ' | sed 's/\x00/\n/g' | grep -vE '^(KUBERNETES_(SERVICE|PORT|ROLE)[_=]|((POD_(IP|NAMESPACE))|HOSTNAME|PATH|PGHOME|LC_ALL|ENABLE_PG_MON)=)' | sed 's/=.*//g'); do
    unset $E
done

ref: https://github.com/zalando/spilo/blob/master/postgres-appliance/runit/patroni/run#L24-L26

Would it be possible to add |(PATRONI_LOG_.*=) to the regex to allow PATRONI_LOG_* variables to be passed to the process?
@CyberDem0n
Copy link
Contributor

You can always inject part of Patroni configuration via SPILO_CONFIGURATION:

{"log":{"level":"DEBUG", ...}}

@mhaley-tignis
Copy link
Author

I am using the postgres kubernetes operator to manage the database. Unfortunately the operator does not provide a way to add custom config to SPILO_CONFIGURATION. We could update the operator, do you think that is a better route to go? I think that changing both project would be the most flexible. Is there a reason we don't want to allow PATRONI_* variables to be passed to patroni?

@CyberDem0n
Copy link
Contributor

Is there a reason we don't want to allow PATRONI_* variables to be passed to patroni?

Yes, these variables may have some sensitive data that will be visible to Postgres. Exactly this is written in the comment to lines you are referring at:

spilo/postgres-appliance/runit/patroni/run

Line 23 in 823f146

# Only small subset of environment variables is allowed. We don't want accidentally disclose sensitive information

@mhaley-tignis
Copy link
Author

I see, does patroni not strip out PATRONI_* environment variables itself before starting postgres?

ref zalando/patroni#1224
ref: https://github.com/zalando/patroni/blob/master/patroni/postgresql/postmaster.py#L215-L216

So shouldn't it be safe to be able to configure patroni using env vars (as is documented) and let it worry about removing them before starting postgres?

@mhaley-tignis mhaley-tignis linked a pull request Apr 14, 2022 that will close this issue
Open
@mhaley-tignis
Copy link
Author

Made a quick PR here (#719) @CyberDem0n, let me know what you think.

@mhaley-tignis mhaley-tignis mentioned this issue Apr 18, 2022
Open
@mhaley-tignis
Copy link
Author

@CyberDem0n any additional thoughts here? Are we unable to rely on patroni process to protect itself?
If not, an alternative solution is here zalando/postgres-operator#1857. Either one solves my needs.

@mhaley-tignis mhaley-tignis mentioned this issue Jul 22, 2022
Open
@holooloo
Copy link

any news to decrease log verbosity?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow passing PATRONI_LOG_* environment variables to patroni process. mhaley-tignis/spilo
3 participants
@CyberDem0n @holooloo @mhaley-tignis