Skip to content

Make user property optional in JDBC#11350

Merged
kokosing merged 1 commit intotrinodb:masterfrom
lukasz-walkiewicz:optional-user
Mar 30, 2022
Merged

Make user property optional in JDBC#11350
kokosing merged 1 commit intotrinodb:masterfrom
lukasz-walkiewicz:optional-user

Conversation

@lukasz-walkiewicz
Copy link
Copy Markdown
Member

@lukasz-walkiewicz lukasz-walkiewicz commented Mar 7, 2022

Description

user property is only required in two scenarios:

  • no authentication - in this case the server will return a well-formed
    error,
  • basic authentication - the driver will validate the configuration and
    return an error if only password was provided.

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# JDBC Driver
* Remove `user` property requirement

Fixes: #9669

@cla-bot cla-bot bot added the cla-signed label Mar 7, 2022
@findepi findepi added the jdbc Relates to Trino JDBC driver label Mar 7, 2022
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Can we remove things like the below from product tests that are using KRB or other authentications methods and use user mapping instead there?

    #   1) It should belong to the "admin" role in hive
    #   2) It should have the required privileges (such as SELECT) on Hive tables (such as hive.default.nation)
    jdbc_user: hdfs
  

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 7, 2022

--user option in case of CLI and user property in case of JDBC is only required when no or basic authentication is being used. In the first case server will return a well-formed error

I'd strongly prefer to avoid breaking change like this when no authentication is used.
It should actually be easy to do.

(if i were reviewing this PR, it would be 'must have' change; i am currently not reviewing this)

Copy link
Copy Markdown
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

It’s good to make it optional, but we shouldn’t make it a breaking change. Let’s allow setting it to empty to indicate that it shouldn’t be sent.

@electrum
Copy link
Copy Markdown
Member

electrum commented Mar 8, 2022

JDBC and CLI are different and the behavior needs to be considered separately. Can you create separate pull requests?

@lukasz-walkiewicz lukasz-walkiewicz changed the title Make user property optional in JDBC and CLI Make user property optional in JDBC Mar 9, 2022
@lukasz-walkiewicz
Copy link
Copy Markdown
Member Author

lukasz-walkiewicz commented Mar 9, 2022

JDBC and CLI are different and the behavior needs to be considered separately. Can you create separate pull requests?

I've removed CLI related changes from this PR. I'll create a seperate PR.

@lukasz-walkiewicz
Copy link
Copy Markdown
Member Author

Can we remove things like the below from product tests that are using KRB or other authentications methods and use user mapping instead there?

    #   1) It should belong to the "admin" role in hive
    #   2) It should have the required privileges (such as SELECT) on Hive tables (such as hive.default.nation)
    jdbc_user: hdfs
  

I removed it but this cause a lot of product test failures. I tried to replace it with user mapping but it didn't work: it was using jdbc_user from testing/trino-product-tests/src/main/resources/tempto-configuration.yaml. Also, it's not possible to simply delete jdbc_user at the moment, Tempto requires it.

@kokosing
Copy link
Copy Markdown
Member

@electrum Do you want to take a look into this? There are no more breaking changes, so I would like to merge it.

@lukasz-walkiewicz
Copy link
Copy Markdown
Member Author

CI hit: #6315

`user` property is only required in two scenarios:
* no authentication - in this case the server will return a well-formed
  error,
* basic authentication - the driver will validate the configuration and
  return an error if only password was provided.
@lukasz-walkiewicz
Copy link
Copy Markdown
Member Author

Any updates on this @electrum?

@leniartek
Copy link
Copy Markdown

@lukasz-walkiewicz @kokosing please merge as this was positively reviewed 21 days ago.

@kokosing kokosing merged commit a51b35c into trinodb:master Mar 30, 2022
@kokosing
Copy link
Copy Markdown
Member

Merged, thanks!

@github-actions github-actions bot added this to the 376 milestone Mar 30, 2022
@lukasz-walkiewicz lukasz-walkiewicz deleted the optional-user branch March 30, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs jdbc Relates to Trino JDBC driver

Development

Successfully merging this pull request may close these issues.

JDBC driver should not expect a user name when a JWT is passed via accessToken name-value pair

5 participants