Support all Trino Security Access Control modes in Delta lake Connector by mdesmet · Pull Request #11782 · trinodb/trino

mdesmet · 2022-04-04T21:49:14Z

Description

Support Read-only, file and allow all access control modes in delta connector.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Change to the Delta Lake Connector

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Fixes Delta connector doesn't support all Trino Access Control modes #11779

Documentation

( ) 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.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

mdesmet · 2022-04-08T13:01:59Z

This PR is preparation to support Trino views within the Delta Lake connector. We would need to support Access control modes as otherwise the default SYSTEM access control would fail on every view access.

See following comment

findepi · 2022-05-18T13:45:22Z

@kokosing @lukasz-walkiewicz did you have time to look into this?

mdesmet · 2022-05-18T20:42:50Z

I've also added docs based on the existing Iceberg docs.

alexjo2144 · 2022-05-19T20:03:24Z

Are the other SecurityModules used by the Hive connector applicable? legacy and sql-standard?

findepi · 2022-05-19T20:10:01Z

@alexjo2144 good question

legacy is Hive's default because changing the default is hard.
i don't think we want it for Delta, do we?

cc @kokosing @electrum

kokosing · 2022-05-19T20:37:56Z

We don't want legacy.

mdesmet · 2022-05-28T09:55:37Z

Is this good to be merged? This would unblock #11763

findinpath · 2022-07-26T04:05:12Z

@lukasz-walkiewicz CPTAL ?

lukasz-walkiewicz

LGTM 👍

lukasz-walkiewicz · 2022-07-26T11:08:44Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSecurityConfig.java

nit: static import

lukasz-walkiewicz · 2022-07-26T11:08:59Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSecurityConfig.java

nit: description

Gentle reminder

alexjo2144 · 2022-07-26T14:12:27Z

@mdesmet can you resolve the conflicts?

kokosing · 2022-07-27T19:26:25Z

...in/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSecurityConfig.java

I would prefer SYSTEM as default

I based myself on the Iceberg security default:

trino/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSecurityConfig.java

Line 30 in 627e395

private IcebergSecurity securitySystem = IcebergSecurity.ALLOW_ALL;

Wouldn't we want to keep this consistent with Iceberg?

I would change the Iceberg too. From user perspective ALLOW_ALL and and SYSTEM do not differ match, other than ALLOW_ALL pretends that it manages roles (while actaully it has no roles) so it limits system roles use cases.

I think we could even consider removal of ALLOW_ALL.

Hence I would prefer to use SYSTEM here, and then update Iceberg too.

findinpath · 2022-08-22T04:40:40Z

docs/src/main/sphinx/connector/delta-lake.rst

Please add a note which option is used by default.

findinpath · 2022-08-22T04:40:58Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSecurityConfig.java

Gentle reminder

findinpath · 2022-08-22T04:42:33Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakePlugin.java

test-iceberg-plugin-access-control -> test-delta-lake-plugin-access-control

findinpath · 2022-08-22T04:46:05Z

@kokosing CPTAL over this PR ? It has been dormant for a while now.

kokosing · 2022-08-26T10:38:08Z

Either I need to base #13862 on this, on this has to be rebased on top of #13862.

kokosing · 2022-08-26T14:29:02Z

Thanks to #13862 I realized I was wrong. I think you should allow-all as default.

kokosing · 2022-08-26T14:29:20Z

I am very sorry I have mislead you in the first place.

findinpath · 2022-09-01T05:13:27Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadataFactory.java

Unused parameter. Please remove.

findinpath · 2022-09-01T05:14:27Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSecurityConfig.java

We can safely add a static import here. I think it will not be misleading.

mdesmet · 2022-09-06T06:26:26Z

@kokosing: Can you have another look?

kokosing · 2022-09-06T12:06:46Z

Thanks. I am sorry for the delay.

vkorukanti · 2023-04-07T22:45:08Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSecurityModule.java

+        install(new ConnectorAccessControlModule());
+        bindSecurityModule(ALLOW_ALL, new AllowAllSecurityModule());
+        bindSecurityModule(READ_ONLY, new ReadOnlySecurityModule());
+        bindSecurityModule(FILE, new FileBasedAccessControlModule());


@mdesmet @findepi On qn: can Hive's sql-standard be one of the options here in addition the existing access models?

cla-bot bot added the cla-signed label Apr 4, 2022

mdesmet force-pushed the feature/access_controls branch from 4439683 to 9b6b0cc Compare April 4, 2022 22:04

mdesmet mentioned this pull request Apr 4, 2022

Support views in Delta lake Connector #11763

Merged

mdesmet force-pushed the feature/access_controls branch from 9b6b0cc to bf45197 Compare April 5, 2022 07:23

mdesmet requested review from kokosing and lukasz-walkiewicz April 8, 2022 12:59

mdesmet requested a review from findepi May 17, 2022 21:27

findepi requested a review from dain May 18, 2022 13:44

kokosing approved these changes May 18, 2022

View reviewed changes

mdesmet force-pushed the feature/access_controls branch from bf45197 to 1b2d90f Compare May 18, 2022 20:41

github-actions bot added the docs label May 18, 2022

findepi requested review from alexjo2144 and findinpath May 19, 2022 14:04

lukasz-walkiewicz approved these changes Jul 26, 2022

View reviewed changes

mdesmet force-pushed the feature/access_controls branch from 1b2d90f to cb83597 Compare July 27, 2022 13:43

kokosing reviewed Jul 27, 2022

View reviewed changes

findinpath reviewed Aug 22, 2022

View reviewed changes

mdesmet force-pushed the feature/access_controls branch from cb83597 to 0419680 Compare August 25, 2022 12:53

kokosing approved these changes Aug 26, 2022

View reviewed changes

mdesmet force-pushed the feature/access_controls branch from 0419680 to 4b2b708 Compare August 31, 2022 20:17

findinpath reviewed Sep 1, 2022

View reviewed changes

Support all Trino Security Access Control modes in Delta lake Connector

7964760

mdesmet force-pushed the feature/access_controls branch from 4b2b708 to 7964760 Compare September 1, 2022 06:58

kokosing approved these changes Sep 6, 2022

View reviewed changes

kokosing merged commit 95849b0 into trinodb:master Sep 6, 2022

github-actions bot added this to the 395 milestone Sep 6, 2022

colebow mentioned this pull request Sep 6, 2022

Add Trino 395 release notes #13975

Merged

vkorukanti reviewed Apr 7, 2023

View reviewed changes

Conversation

mdesmet commented Apr 4, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

mdesmet commented Apr 8, 2022

Uh oh!

findepi commented May 18, 2022

Uh oh!

mdesmet commented May 18, 2022

Uh oh!

alexjo2144 commented May 19, 2022

Uh oh!

findepi commented May 19, 2022

Uh oh!

kokosing commented May 19, 2022

Uh oh!

mdesmet commented May 28, 2022

Uh oh!

findinpath commented Jul 26, 2022

Uh oh!

lukasz-walkiewicz 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!

alexjo2144 commented Jul 26, 2022

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!

findinpath commented Aug 22, 2022

Uh oh!

kokosing commented Aug 26, 2022

Uh oh!

kokosing commented Aug 26, 2022

Uh oh!

kokosing commented Aug 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdesmet commented Sep 6, 2022

Uh oh!

kokosing commented Sep 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants