Don't show unsupported AUTHORIZATION ROLE in Hive schema by ebyhr · Pull Request #14285 · trinodb/trino

ebyhr · 2022-09-26T07:53:00Z

Description

Don't show unsupported AUTHORIZATION ROLE property in SHOW CREATE SCHEMA result when the access control doesn't support roles in Hive connector.
Fixes #8817

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Hive
* Don't show unsupported `AUTHORIZATION ROLE` property in `SHOW CREATE SCHEMA` result when the access control doesn't support roles. ({issue}`8817`)

findepi · 2022-09-26T08:38:37Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/AccessControlMetadata.java

"supportsRoles" (plural)?

also, this should be used in default implementations of all other role-related methods.

plugin/trino-hive/src/test/resources/io/trino/plugin/hive/security.json

kokosing

skimmed

kokosing · 2022-09-26T20:56:49Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/AccessControlMetadata.java

I don't like it. Maybe we should have AccessControlMetadata to be bound as Optional?

kokosing · 2022-09-26T20:58:00Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

I think it exposes some issue in the engine. If engine controls ownership elsewhere it should not even ask connector for the owner.

It seems it didn't expose engine issues as CI is green. io.trino.metadata.MetadataManager#getSchemaOwner doesn't call this method when the security management is SYSTEM. Is there other logic I should confirm?

I think I get it now, it is about using allow-all so you now don't return the owner of the schema in such case.

I am not sure if it is the way to go. Support of roles and ownership could be seen independent from each other. I mean returning role as owner would seem odd, but returning an user sounds ok.

Should we move this method implementation to AccessControlMetadata?

What about owner for table and view?

Moved to AccessControlMetadata. SHOW CREATE table and view don't show the owner if my understanding is correct.

I know, but it is more about the logical ownership. The thing that schema owner is used today should define the actual design decision.

ebyhr · 2022-10-05T09:22:24Z

Just rebased on upstream.

dain · 2022-10-07T18:12:20Z

Have we considered going the opposite direction and instead adding support for setting the owner directly during create schema?

findepi · 2022-10-24T07:49:43Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/AccessControlMetadata.java

+     */
+    default Optional<HivePrincipal> getSchemaOwner(ConnectorSession session, String schemaName)
+    {
+        return Optional.empty();


Should this throw in the default implementation?
Looks like all other methods do throw by default.

hashhar · 2022-11-07T21:01:49Z

Ci seems to fail. PTAL https://github.com/trinodb/trino/actions/runs/3410817080/jobs/5674773042#step:4:348

Error:  io.trino.plugin.iceberg.catalog.glue.TestSharedGlueMetastore.testShowSchemas  Time elapsed: 0.919 s  <<< FAILURE!
java.lang.AssertionError: 
expected [CREATE SCHEMA hive.test_shared_schema_14x52kn5jo
AUTHORIZATION ROLE public
WITH (
   location = '/tmp/TrinoTest16557917362932507789/iceberg_data'
)] but found [CREATE SCHEMA hive.test_shared_schema_14x52kn5jo
WITH (
   location = '/tmp/TrinoTest16557917362932507789/iceberg_data'
)]
	at org.testng.Assert.fail(Assert.java:94)
	at org.testng.Assert.failNotEquals(Assert.java:513)
	at org.testng.Assert.assertEqualsImpl(Assert.java:135)
	at org.testng.Assert.assertEquals(Assert.java:116)
	at org.testng.Assert.assertEquals(Assert.java:190)
	at org.testng.Assert.assertEquals(Assert.java:200)
	at io.trino.plugin.iceberg.BaseSharedMetastoreTest.testShowSchemas(BaseSharedMetastoreTest.java:121)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

and

Error:  io.trino.plugin.deltalake.TestDeltaLakeSharedGlueMetastoreWithTableRedirections.testShowSchemas  Time elapsed: 2.98 s  <<< FAILURE!
java.lang.AssertionError: 
expected [CREATE SCHEMA hive_with_redirections.test_shared_schema_1oiyqgnkkd
AUTHORIZATION ROLE public
WITH (
   location = '/tmp/TrinoTest1321593021550281499/delta_lake_data'
)] but found [CREATE SCHEMA hive_with_redirections.test_shared_schema_1oiyqgnkkd
WITH (
   location = '/tmp/TrinoTest1321593021550281499/delta_lake_data'
)]
	at org.testng.Assert.fail(Assert.java:94)
	at org.testng.Assert.failNotEquals(Assert.java:513)
	at org.testng.Assert.assertEqualsImpl(Assert.java:135)
	at org.testng.Assert.assertEquals(Assert.java:116)
	at org.testng.Assert.assertEquals(Assert.java:190)
	at org.testng.Assert.assertEquals(Assert.java:200)
	at io.trino.plugin.deltalake.BaseDeltaLakeSharedMetastoreWithTableRedirectionsTest.testShowSchemas(BaseDeltaLakeSharedMetastoreWithTableRedirectionsTest.java:85)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

ebyhr · 2022-11-07T22:33:41Z

@hashhar Thanks for catching that. Sent #14940

cla-bot bot added the cla-signed label Sep 26, 2022

github-actions bot added the tests:hive label Sep 26, 2022

findepi reviewed Sep 26, 2022

View reviewed changes

findepi requested review from dain, kokosing and lukasz-walkiewicz September 26, 2022 08:39

ebyhr force-pushed the ebi/hive-show-create-schema-authorization-160b7067f1 branch 2 times, most recently from c1038f5 to 0afe70e Compare September 26, 2022 11:33

kokosing reviewed Sep 26, 2022

View reviewed changes

ebyhr force-pushed the ebi/hive-show-create-schema-authorization-160b7067f1 branch from 0afe70e to eb179b0 Compare September 27, 2022 22:36

Don't show unsupported AUTHORIZATION ROLE in Hive schema

36b53f4

ebyhr force-pushed the ebi/hive-show-create-schema-authorization-160b7067f1 branch from eb179b0 to 36b53f4 Compare October 5, 2022 09:22

findepi reviewed Oct 24, 2022

View reviewed changes

kokosing approved these changes Nov 7, 2022

View reviewed changes

ebyhr merged commit 442e31c into trinodb:master Nov 7, 2022

ebyhr deleted the ebi/hive-show-create-schema-authorization-160b7067f1 branch November 7, 2022 12:54

ebyhr mentioned this pull request Nov 7, 2022

Release notes for 403 #14878

Closed

github-actions bot added this to the 403 milestone Nov 7, 2022

ebyhr mentioned this pull request Nov 7, 2022

Fix broken TestSharedGlueMetastore.getExpectedHiveCreateSchema #14940

Merged

colebow mentioned this pull request Nov 9, 2022

Add Trino 403 release notes #14977

Merged

Conversation

ebyhr commented Sep 26, 2022

Description

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Oct 5, 2022

Uh oh!

dain commented Oct 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar commented Nov 7, 2022

Uh oh!

ebyhr commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants