[SPARK-30797][SQL] Set tradition user/group/other permission to ACL entries when setting up ACLs in truncate table #27548

viirya · 2020-02-12T09:24:51Z

What changes were proposed in this pull request?

This is a follow-up to the PR #26956. In #26956, the patch proposed to preserve path permission when truncating table. When setting up original ACLs, we need to set user/group/other permission as ACL entries too, otherwise if the path doesn't have default user/group/other ACL entries, ACL API will complain an error Invalid ACL: the user, group and other entries are required..

In short this change makes sure:

Permissions for user/group/other are always kept into ACLs to work with ACL API.
Other custom ACLs are still kept after TRUNCATE TABLE ([SPARK-30312][SQL] Preserve path permission and acl when truncate table #26956 did this).

Why are the changes needed?

Without this fix, TRUNCATE TABLE will get an error when setting up ACLs if there is no default default user/group/other ACL entries.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Update unit test.

Manual test on dev Spark cluster.

Set ACLs for a table path without default user/group/other ACL entries:

hdfs dfs -setfacl --set 'user:liangchi:rwx,user::rwx,group::r--,other::r--' /user/hive/warehouse/test.db/test_truncate_table

hdfs dfs -getfacl /user/hive/warehouse/test.db/test_truncate_table
# file: /user/hive/warehouse/test.db/test_truncate_table
# owner: liangchi
# group: supergroup
user::rwx
user:liangchi:rwx
group::r--
mask::rwx
other::r--

Then run sql("truncate table test.test_truncate_table"), it works by normally truncating the table and preserve ACLs.

viirya · 2020-02-12T09:25:44Z

cc @cloud-fan @dongjoon-hyun This targets both mater and branch-3.0.

cloud-fan

LGTM as a bug fix, but we should really revist the TRUNCATE TABLE implementation to think about how to make it simpler...

SparkQA · 2020-02-12T13:54:34Z

Test build #118288 has finished for PR 27548 at commit b2fa454.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2020-02-12T16:15:24Z

@cloud-fan That's good suggestion. Let's see if we can simplify it in next versions.

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

dongjoon-hyun

+1, LGTM. (Pending Jenkins)
Thank you, @viirya , @HyukjinKwon , @cloud-fan .

dongjoon-hyun · 2020-02-12T22:14:15Z

Oh, @viirya . During deleting my comments, it seems that your reply also is gone. Sorry for that.

viirya · 2020-02-12T22:15:13Z

@dongjoon-hyun It's ok. Thanks for review!

dongjoon-hyun · 2020-02-12T22:26:34Z

I'll merge this since the last commit is only comment-only change due to me.
Merged to master/3.0.

…ntries when setting up ACLs in truncate table ### What changes were proposed in this pull request? This is a follow-up to the PR #26956. In #26956, the patch proposed to preserve path permission when truncating table. When setting up original ACLs, we need to set user/group/other permission as ACL entries too, otherwise if the path doesn't have default user/group/other ACL entries, ACL API will complain an error `Invalid ACL: the user, group and other entries are required.`. In short this change makes sure: 1. Permissions for user/group/other are always kept into ACLs to work with ACL API. 2. Other custom ACLs are still kept after TRUNCATE TABLE (#26956 did this). ### Why are the changes needed? Without this fix, `TRUNCATE TABLE` will get an error when setting up ACLs if there is no default default user/group/other ACL entries. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Update unit test. Manual test on dev Spark cluster. Set ACLs for a table path without default user/group/other ACL entries: ``` hdfs dfs -setfacl --set 'user:liangchi:rwx,user::rwx,group::r--,other::r--' /user/hive/warehouse/test.db/test_truncate_table hdfs dfs -getfacl /user/hive/warehouse/test.db/test_truncate_table # file: /user/hive/warehouse/test.db/test_truncate_table # owner: liangchi # group: supergroup user::rwx user:liangchi:rwx group::r-- mask::rwx other::r-- ``` Then run `sql("truncate table test.test_truncate_table")`, it works by normally truncating the table and preserve ACLs. Closes #27548 from viirya/fix-truncate-table-permission. Lead-authored-by: Liang-Chi Hsieh <liangchi@uber.com> Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 5b76367) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2020-02-12T22:32:53Z

Since this is a security stuff and SPARK-30312 is in branch-2.4, I'll merge this to branch-2.4 together after testing locally.

…ntries when setting up ACLs in truncate table ### What changes were proposed in this pull request? This is a follow-up to the PR #26956. In #26956, the patch proposed to preserve path permission when truncating table. When setting up original ACLs, we need to set user/group/other permission as ACL entries too, otherwise if the path doesn't have default user/group/other ACL entries, ACL API will complain an error `Invalid ACL: the user, group and other entries are required.`. In short this change makes sure: 1. Permissions for user/group/other are always kept into ACLs to work with ACL API. 2. Other custom ACLs are still kept after TRUNCATE TABLE (#26956 did this). ### Why are the changes needed? Without this fix, `TRUNCATE TABLE` will get an error when setting up ACLs if there is no default default user/group/other ACL entries. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Update unit test. Manual test on dev Spark cluster. Set ACLs for a table path without default user/group/other ACL entries: ``` hdfs dfs -setfacl --set 'user:liangchi:rwx,user::rwx,group::r--,other::r--' /user/hive/warehouse/test.db/test_truncate_table hdfs dfs -getfacl /user/hive/warehouse/test.db/test_truncate_table # file: /user/hive/warehouse/test.db/test_truncate_table # owner: liangchi # group: supergroup user::rwx user:liangchi:rwx group::r-- mask::rwx other::r-- ``` Then run `sql("truncate table test.test_truncate_table")`, it works by normally truncating the table and preserve ACLs. Closes #27548 from viirya/fix-truncate-table-permission. Lead-authored-by: Liang-Chi Hsieh <liangchi@uber.com> Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 5b76367) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

viirya · 2020-02-12T23:04:36Z

@dongjoon-hyun Ok. Thanks!

SparkQA · 2020-02-13T01:47:29Z

Test build #118321 has finished for PR 27548 at commit 7568db7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…ntries when setting up ACLs in truncate table ### What changes were proposed in this pull request? This is a follow-up to the PR apache#26956. In apache#26956, the patch proposed to preserve path permission when truncating table. When setting up original ACLs, we need to set user/group/other permission as ACL entries too, otherwise if the path doesn't have default user/group/other ACL entries, ACL API will complain an error `Invalid ACL: the user, group and other entries are required.`. In short this change makes sure: 1. Permissions for user/group/other are always kept into ACLs to work with ACL API. 2. Other custom ACLs are still kept after TRUNCATE TABLE (apache#26956 did this). ### Why are the changes needed? Without this fix, `TRUNCATE TABLE` will get an error when setting up ACLs if there is no default default user/group/other ACL entries. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Update unit test. Manual test on dev Spark cluster. Set ACLs for a table path without default user/group/other ACL entries: ``` hdfs dfs -setfacl --set 'user:liangchi:rwx,user::rwx,group::r--,other::r--' /user/hive/warehouse/test.db/test_truncate_table hdfs dfs -getfacl /user/hive/warehouse/test.db/test_truncate_table # file: /user/hive/warehouse/test.db/test_truncate_table # owner: liangchi # group: supergroup user::rwx user:liangchi:rwx group::r-- mask::rwx other::r-- ``` Then run `sql("truncate table test.test_truncate_table")`, it works by normally truncating the table and preserve ACLs. Closes apache#27548 from viirya/fix-truncate-table-permission. Lead-authored-by: Liang-Chi Hsieh <liangchi@uber.com> Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

viirya added 2 commits February 12, 2020 01:06

Set tradition user/group/other permission to ACL entries.

465ff45

Update test.

b2fa454

cloud-fan approved these changes Feb 12, 2020

View reviewed changes

HyukjinKwon approved these changes Feb 12, 2020

View reviewed changes

dongjoon-hyun reviewed Feb 12, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala Show resolved Hide resolved

dongjoon-hyun reviewed Feb 12, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala Show resolved Hide resolved

Revise comments for review comment.

7568db7

dongjoon-hyun reviewed Feb 12, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala Show resolved Hide resolved

dongjoon-hyun added the SQL label Feb 12, 2020

apache deleted a comment from viirya Feb 12, 2020

dongjoon-hyun approved these changes Feb 12, 2020

View reviewed changes

dongjoon-hyun closed this in 5b76367 Feb 12, 2020

viirya deleted the fix-truncate-table-permission branch December 27, 2023 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-30797][SQL] Set tradition user/group/other permission to ACL entries when setting up ACLs in truncate table #27548

[SPARK-30797][SQL] Set tradition user/group/other permission to ACL entries when setting up ACLs in truncate table #27548

Uh oh!

viirya commented Feb 12, 2020 •

edited

Loading

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

cloud-fan left a comment

Uh oh!

SparkQA commented Feb 12, 2020

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Uh oh!

dongjoon-hyun commented Feb 12, 2020

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

dongjoon-hyun commented Feb 12, 2020

Uh oh!

dongjoon-hyun commented Feb 12, 2020

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

SparkQA commented Feb 13, 2020

Uh oh!

Uh oh!

[SPARK-30797][SQL] Set tradition user/group/other permission to ACL entries when setting up ACLs in truncate table #27548

[SPARK-30797][SQL] Set tradition user/group/other permission to ACL entries when setting up ACLs in truncate table #27548

Uh oh!

Conversation

viirya commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 12, 2020

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Feb 12, 2020

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

dongjoon-hyun commented Feb 12, 2020

Uh oh!

dongjoon-hyun commented Feb 12, 2020

Uh oh!

viirya commented Feb 12, 2020

Uh oh!

SparkQA commented Feb 13, 2020

Uh oh!

Uh oh!

viirya commented Feb 12, 2020 •

edited

Loading