-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add support for pgpass #666
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Aimilios Tsouvelekakis <[email protected]>
@wangxiaoying Adding a bunch of new code to support specific postgres functionality into the
|
@@ -247,6 +247,66 @@ def read_sql( | |||
) -> pl.DataFrame: ... | |||
|
|||
|
|||
def get_passfile_content(path: pathlib.Path) -> dict: | |||
# host:port:db_name:user_name:password | |||
with open(path) as f: |
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.
How about we do something different? the content we need from pgpass is the password only the restof the elements are used for retrieving it. So:
for line in f:
contents = line.read().split(':')
if len(contents) != 5:
raise Exception('Pgpass content should follow: host:port:db_name:user_name:password')
else:
if ((contents['host'] == host or contents['host'] == '*') and
(contents['port'] == port or contents['port'] == '*') and
(contents['database'] == database or contents['database'] == '*') and
(contents['user'] == user or contents['user'] == '*')):
return contents['password']
So in this way we do also multiple lines and we return the first line matched.
def get_pgpass(conn) -> dict: | ||
# check param or (PGPASS env or DEFAULT) | ||
# DEFAULT = linux or windows | ||
passfile = '%APPDATA%\postgresql\pgpass.conf' if sys.platform == 'windows' else os.path.expanduser( |
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.
Instead of sys.platform maybe we can use os.name == 'nt'
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 it better?
|
||
passfile_path = pathlib.Path(passfile) | ||
|
||
if sys.platform != 'windows': |
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.
Same here: os.name != 'nt'
# We rewrite the netloc from scratch here e.g. | ||
# netloc = contents['username'] or o.username + ':' + contents['password'] or o.password + ... | ||
parsed_conn = urllib.parse.urlparse(conn) | ||
return str(parsed_conn._replace(**contents)) |
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.
We can do the following:
database = parsed_conn.path[1:] or os.environ.get('PGDATABASE', os.getenv('USER'))
host = parsed_conn.hostname or os.environ.get('PGHOST', 'localhost')
port = parsed_conn.port or os.environ.get('PGPORT', '5432')
user = parsed_conn.username or os.environ.get('PGUSER', os.getenv('USER'))
password = parsed_conn.password
We parse 2 times the URI so maybe we need to rethink that maybe we call pgpass from inside reconstruct? That would allow something like that also:
query_params = parse_qs(parsed_con.query)
passfile = query_params.get('passfile', [None])[0] or os.environ.get('PGPASSFILE')
default_pgpass_path = os.path.expanduser('~/.pgpass') if os.name != 'nt' else os.path.join(os.getenv('APPDATA', ''), 'postgresql', 'pgpass.conf')
# Determine the password precedence
if not password:
if 'PGPASSWORD' in os.environ:
password = os.environ['PGPASSWORD']
else:
pgpass_path = passfile or default_pgpass_path
password = get_passfile_content(pgpass_path, host or '*', str(port or '*'), database or '*', user or '*')
And with all these information we can get the new connection string.
|
||
def run_per_database(conn): | ||
# Todo rename to something better. | ||
if 'postgresql' in conn: |
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.
Or postgres is also valid: https://www.postgresql.org/docs/current/libpq-connect.html
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.
Then we ought to change it to postgres
Thanks for the PR and review @surister @aimtsou !
I think moving the logic into an independent file would be great! |
I just pushed a commit to fix the error that cause the fail of the CI. Please feel free to rebase on that one. |
We follow the spec in https://www.postgresql.org/docs/current/libpq-pgpass.html,
Order of passfile path is:
postgresql://localhost:5432?passfile=/home/someuser/secret/.pgpass
~/.pgpass
in *nix,%APPDATA%\postgresql\pgpass.conf
in windows.0600
or we raise an exception.host:port:db_name:user_name:password
, so*:*:*:*:password
is valid, justpassword
is not.pgpassfile has precedence over the given connection string.
Fixes: #567