Skip to content

Allow Trino connections with password#15

Merged
erezsh merged 9 commits intoerezsh:masterfrom
jkleinkauff:trino-password
Jul 12, 2024
Merged

Allow Trino connections with password#15
erezsh merged 9 commits intoerezsh:masterfrom
jkleinkauff:trino-password

Conversation

@jkleinkauff
Copy link
Copy Markdown
Contributor

@jkleinkauff jkleinkauff commented Jul 12, 2024

fix #14

  • Most of the files are not mandatory. Those are just instructions and files to spun a Trino cluster using password file authentication.
  • We don't need to bump the Trino version. As I was working those files I though we can bump it.

New options in server config

If using jdk < 22, new versions of Trino will complain about

dd-trino | ERROR: Trino requires -XX:+UnlockDiagnosticVMOptions -XX:G1NumCollectionsKeepPinned=10000000 on Java versions lower than 22.0.2 due to JDK-8329528
https://bugs.openjdk.org/browse/JDK-8329528

-XX:+UnlockDiagnosticVMOptions

Unlocks additional diagnostic options for the JVM.
Here's more info trinodb/trino#12251

-XX:G1NumCollectionsKeepPinned

More info trinodb/trino#21999


To allow Trino connections with password, I believe two routes would work.

Check for "auth" parameter like we have in presto.py

https://github.com/erezsh/sqeleton/blob/01ef6c7ee742aee754a8ea628fc3b6f0ec2d5655/sqeleton/databases/presto.py#L173C1-L174C95

Check for password

if kw.get("password"):
    kw["auth"] = trino.auth.BasicAuthentication(
        kw.pop("user"), kw.pop("password")
    )

The second method would work for sqlalchemy like conn strings and also when using db_conf dict with user and password.

@erezsh I may be missing something, let me know what you think and I can change it to check for auth instead. Thank you so much let me work on that one.

@erezsh
Copy link
Copy Markdown
Owner

erezsh commented Jul 12, 2024

Thanks for the PR! It looks pretty good. I especially like the README you added with the extra info.

A few points:

  • The tests are failing because they can't connect. What URL should we use for trino now?

  • If there are explanations you can add for the added lines (like why you changed jvm.config), it would be welcome.

  • The code can be a bit simpler if you use cert = kw.pop("cert", None) and then test if cert is None.

@jkleinkauff
Copy link
Copy Markdown
Contributor Author

jkleinkauff commented Jul 12, 2024

@erezsh thank you so much for the feedback.

The last commit will pass the tests. Maybe we can let the tests run against the same cluster as before and then, if the user wants a cluster with password, he could follow the instructions on the readme file?

The tests could run against the same cluster as before. Or the password authentication could be the default settings for Trino cluster, then we test TRINO_URI with localhost:8081 (without auth) and localhost:8443 (with password). There is an option that allow us to access http even with password enabled. (insecure but just for the tests). If we go for the later I need to edit de ci to build the certs before starting the server, or we can let "default" certs in git. Not secure but it is only for the tests.

@erezsh erezsh merged commit 0abd60d into erezsh:master Jul 12, 2024
@erezsh
Copy link
Copy Markdown
Owner

erezsh commented Jul 12, 2024

Thank you for contributing!

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.

Trino - Allow login using BasicAuthentication

2 participants