From a4be54d21fe0cc923c1545e66fb5dd92b95664e2 Mon Sep 17 00:00:00 2001 From: Alexey Kondratov Date: Mon, 23 Jan 2023 17:46:22 +0100 Subject: [PATCH] [compute_ctl] Stop updating roles on each compute start (#3391) I noticed that `compute_ctl` updates all roles on each start, search for rows like > - web_access:[FILTERED] -> update in the compute startup log. It happens since we had an adhoc hack for md5 hashes comparison, which doesn't work with scram hashes stored in the `pg_authid`. It doesn't really hurt, as nothing changes, but we just run >= 2 extra queries on each start, so fix it. --- compute_tools/src/params.rs | 8 +++++++- compute_tools/src/spec.rs | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/compute_tools/src/params.rs b/compute_tools/src/params.rs index 925a2f8ef3..0ce01ff478 100644 --- a/compute_tools/src/params.rs +++ b/compute_tools/src/params.rs @@ -1,3 +1,9 @@ pub const DEFAULT_LOG_LEVEL: &str = "info"; -pub const DEFAULT_CONNSTRING: &str = "host=localhost user=postgres"; +// From Postgres docs: +// To ease transition from the md5 method to the newer SCRAM method, if md5 is specified +// as a method in pg_hba.conf but the user's password on the server is encrypted for SCRAM +// (see below), then SCRAM-based authentication will automatically be chosen instead. +// https://www.postgresql.org/docs/15/auth-password.html +// +// So it's safe to set md5 here, as `control-plane` anyway uses SCRAM for all roles. pub const PG_HBA_ALL_MD5: &str = "host\tall\t\tall\t\t0.0.0.0/0\t\tmd5"; diff --git a/compute_tools/src/spec.rs b/compute_tools/src/spec.rs index 97cd623052..4c7cca545c 100644 --- a/compute_tools/src/spec.rs +++ b/compute_tools/src/spec.rs @@ -152,8 +152,20 @@ pub fn handle_roles(spec: &ComputeSpec, client: &mut Client) -> Result<()> { { RoleAction::Update } else if let Some(pg_pwd) = &r.encrypted_password { - // Check whether password changed or not (trim 'md5:' prefix first) - if pg_pwd[3..] != *role.encrypted_password.as_ref().unwrap() { + // Check whether password changed or not (trim 'md5' prefix first if any) + // + // This is a backward compatibility hack, which comes from the times when we were using + // md5 for everyone and hashes were stored in the console db without md5 prefix. So when + // role comes from the control-plane (json spec) `Role.encrypted_password` doesn't have md5 prefix, + // but when role comes from Postgres (`get_existing_roles` / `existing_roles`) it has this prefix. + // Here is the only place so far where we compare hashes, so it seems to be the best candidate + // to place this compatibility layer. + let pg_pwd = if let Some(stripped) = pg_pwd.strip_prefix("md5") { + stripped + } else { + pg_pwd + }; + if pg_pwd != *role.encrypted_password.as_ref().unwrap() { RoleAction::Update } else { RoleAction::None