Skip to content
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 hive 3 missing privileges and principal #4307

Closed
wants to merge 1 commit into from

Conversation

jiegzhan
Copy link

@jiegzhan jiegzhan commented Jul 1, 2020

I was testing kolosing's PR: #4254, then I encountered these errors below:

Query 20200701_000714_00003_5si9e failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported privilege name: CREATE
Query 20200701_005028_00003_x42iu failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported privilege name: DROP
Query 20200701_011500_00002_6tdhc failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported privilege name: INDEX
Query 20200701_061040_00004_6vhcm failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported privilege name: LOCK
Query 20200701_063758_00004_aiyms failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported privilege name: WRITE
Query 20200701_204738_00001_jq7w9 failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported privilege name: READ


Query 20200701_070112_00004_jz78b failed: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Unsupported principal type: GROUP

I made changes on top of kokosing's PR and tested with my hive-3-metastore-service (hadoop 3.2.1, hive 3.1.2), now I am able to run presto queries.

Please merge kokosing's PR first. This PR added extra privileges and principal on top of his PR. Thanks a lot. @electrum

@cla-bot
Copy link

cla-bot bot commented Jul 1, 2020

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

INDEX,
LOCK,
WRITE,
GROUP,
Copy link
Member

Choose a reason for hiding this comment

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

what is GROUP privilege?

@@ -15,5 +15,5 @@

public enum PrincipalType
{
USER, ROLE
USER, ROLE, GROUP
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
USER, ROLE, GROUP
USER,
ROLE,
GROUP,
/**/

@martint I think we were here someday. See #2940

CREATE,
DROP,
INDEX,
LOCK,
WRITE,
GROUP,
READ,
/**/
Copy link
Member

Choose a reason for hiding this comment

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

Can you please squash commits?

Copy link
Author

Choose a reason for hiding this comment

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

Squashed. Thanks.

@findepi
Copy link
Member

findepi commented Jul 2, 2020

For the privileges that have no meaning in Presto, is there a point in defining them?
User will be able to GRANT them (good), but they will expect them to take effect and they can be surprised they do not (bad).

Since we effectively ignore them, we could ignore them early, when mapping from thrift responses to Presto permissions model.

@kokosing
Copy link
Member

kokosing commented Jul 2, 2020

User will be able to GRANT them

User won't be able to grant them as it is only HivePrivilege. So we will be able to read that from Hive. We might need to add them to the engine, but I don't think we want to.

they will expect them to take effect

If we hit that with properly configured (sql-standard?) access control then it means that we have a bug (aka missing feature) IMO.

@cla-bot
Copy link

cla-bot bot commented Jul 2, 2020

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

@colebow
Copy link
Member

colebow commented Oct 19, 2022

👋 @jiegzhan - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

@colebow colebow closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants