Skip to content

Conversation

@cdbartholomew
Copy link
Contributor

Fixes #8091

Motivation

In order to use authentication in Presto (and Pulsar SQL), the password authenticators plugin must be present in the distribution. If you configure password authentication and the plugin is not present, the server will not start.

Modifications

This update pulls in the password authenticators plugin from the PrestoSQL build and packages as part of the presto distribution used by the pulsar sql-worker run command.

Verifying this change

I have built a Docker image with this change and confirmed that file-based password authentication works with this change. Since the plugin comes from the Presto build, I don't think we need any extra tests for it. Adding an integration test would be difficult because configuring authentication in Presto requires HTTPS to also be set up.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): yes. Adds a new dependency on the Presto password plugin.
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

The plugin can be configured by modifying the files in the /etc/conf/presto directory by following the PulsarSQL (now Trino) documentation. For example, this link explains has to configure file-based password authentication.

@zzzming
Copy link
Contributor

zzzming commented Feb 1, 2021

LGTM

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

I am not sure we need to add this.

password authentication is already supported in the latest image. We have enabled and use it in the helm chart: https://github.com/streamnative/charts/blob/master/charts/pulsar/templates/presto/presto-coordinator-configmap.yaml#L258

@cdbartholomew
Copy link
Contributor Author

Adding the configuration will just cause the server to fail to start, like this:

2021-02-02T00:54:38.791Z	ERROR	main	io.prestosql.server.Server	Password authenticator 'file' is not registered
java.lang.IllegalStateException: Password authenticator 'file' is not registered

To get it to work, you to do item 2, listed in issue #8091: "include the plugin https://mvnrepository.com/artifact/io.prestosql/presto-password-authenticators in the pulsar image and put in the presto plugin folder."

If you don't believe me, you are welcome to try it.

@sijie
Copy link
Member

sijie commented Feb 2, 2021

@cdbartholomew okay. Can you add an integration test to verify this work?

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

rzana added a commit to instructure/pulsar that referenced this pull request Aug 24, 2021
rzana added a commit to instructure/pulsar that referenced this pull request Aug 24, 2021
rzana added a commit to instructure/pulsar that referenced this pull request Aug 24, 2021
rzana added a commit to instructure/pulsar that referenced this pull request Aug 24, 2021
@codelipenghui
Copy link
Contributor

The pr had no activity for 30 days, mark with Stale label.

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

@cdbartholomew:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@miton18
Copy link
Contributor

miton18 commented Nov 30, 2022

Today, we hit this problem, sadly, it was opened on 2021 and not yet fixed :-/

cc @sijie

@tisonkun
Copy link
Member

Closed as stale and conflict.

@miton18 I'm active in reviewing Pulsar SQL related issues/PRs. Also, in the master branch, we upgrade from PrestoSQL to Trino version #16683. If it's still relevant to your use case, you can either open a new issue to describe the case and reproduce or rebase and resubmit this patch (ping me as a reviewer :))

@tisonkun tisonkun closed this Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable authentication for presto sql

7 participants