Skip to content

feat(iceberg): add namespace permissions #13414

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.

Sign up for GitHub

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

Jump to bottom
Merged
chakru-r merged 21 commits into from
Jun 2, 2025
Merged

feat(iceberg): add namespace permissions #13414

chakru-r merged 21 commits into from
Jun 2, 2025

Conversation

@ghost
Copy link

@ghost ghost commented May 5, 2025

No description provided.

@github-actions github-actions bot added docs Issues and Improvements to docs product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment community-contribution PR or Issue raised by member(s) of DataHub Community labels May 5, 2025
@ghost ghost force-pushed the iceberg-namespace-permissions branch from d4f850e to 2ed08e4 Compare May 5, 2025 14:20
@codecov
Copy link

codecov bot commented May 5, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.21739% with 57 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...src/app/permissions/policy/PolicyPrivilegeForm.tsx 56.12% 43 Missing ⚠️
...catalog/rest/secure/IcebergTableApiController.java 82.75% 3 Missing and 2 partials ⚠️
.../catalog/rest/secure/IcebergViewApiController.java 82.14% 3 Missing and 2 partials ⚠️
...log/rest/secure/IcebergNamespaceApiController.java 87.50% 1 Missing and 1 partial ⚠️
...ava/com/datahub/authorization/EntityFieldType.java 0.00% 1 Missing ⚠️
...catalog/rest/secure/AbstractIcebergController.java 91.66% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@ghost ghost force-pushed the iceberg-namespace-permissions branch from 9edf2b8 to 239a4b9 Compare May 8, 2025 07:31
@ghost ghost force-pushed the iceberg-namespace-permissions branch from 239a4b9 to e9e7a30 Compare May 8, 2025 14:08
@ghost ghost marked this pull request as ready for review May 13, 2025 07:47
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label May 13, 2025
chakru-r
chakru-r approved these changes May 13, 2025
View reviewed changes
Copy link
Collaborator

@chakru-r chakru-r left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Two optimizations that could be done if feasible.

I did not review the front end code, someone with that expertise should review it

...g/src/main/java/io/datahubproject/iceberg/catalog/rest/secure/IcebergTableApiController.java Outdated
warehouse,
renameTableRequest.destination().namespace(),
DataOperation.MANAGE_TABLES,
false);
Copy link
Collaborator

@chakru-r chakru-r May 13, 2025

Choose a reason for hiding this comment

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

Suggestion -- an exception that reports both privileges missing may be more useful if the user intends to fix it by adding the missing privileges. This may cause them to see one error at a time, the the second one will show up only after the first privilege is fixed.

Copy link
Author

@ghost ghost May 19, 2025

Choose a reason for hiding this comment

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

@chakru-r incorporated this, could you please review?

@datahub-cyborg datahub-cyborg bot added merge-pending-ci A PR that has passed review and should be merged once CI is green. and removed needs-review Label for PRs that need review from a maintainer. labels May 13, 2025
@ghost ghost force-pushed the iceberg-namespace-permissions branch from 07ad6c9 to 508cb46 Compare May 16, 2025 05:53
@ghost ghost force-pushed the iceberg-namespace-permissions branch from 5f268c0 to 0ec0cdd Compare May 20, 2025 14:55
@ghost ghost force-pushed the iceberg-namespace-permissions branch from fc5766d to 0ec0cdd Compare May 21, 2025 09:07
@ghost ghost force-pushed the iceberg-namespace-permissions branch from 933be93 to 63fa3b9 Compare May 21, 2025 10:48
ksrinath and others added 20 commits May 21, 2025 16:20
feat(iceberg): add namespace permissions
fa30390
refine create-namespace permission resolution
8159bd7
add, harden unit tests
2c7882e
@yoonhyejin
add UI for container-selection for policy
86dbce4
@yoonhyejin
add Containers field to policy details
a13b3da
@yoonhyejin
fix lint
8ece675
modify container selection UI desc, handle namespace-not-found in nam…
05e9d24 
…espace auth flow
add more unit tests, verify number of network calls by container reso…
4457971 
…lver
update doc
fbe4d0b
frontend lint
cab0206
doc lint fix
b1a06e9
add more unit tests
6cffe88
@yoonhyejin
add tsx tests
a048bbd
doc update
45ed650
doc update
cb0afe4
report missing privileges for both source & destination for rename op…
ae28a3e 
…erations
sharpen controller unit test boundaries
71e20ff
add unit tests for tableApiController, misc. changes
490b506
add unit tests for viewApiController
b207446
more unit tests, minor code improvement in rename
6a8000d
@ghost ghost force-pushed the iceberg-namespace-permissions branch from 63fa3b9 to 6a8000d Compare May 21, 2025 10:50
@ghost ghost force-pushed the iceberg-namespace-permissions branch from a6f473e to 6a8000d Compare May 21, 2025 11:45
chriscollins3456
chriscollins3456 reviewed May 28, 2025
View reviewed changes
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

the UI work here looks solid, no real concerns about that.

however, I want to make sure we've run this by the whole team since this is a pretty big change and could lead to some performance concerns.

also, right now we're just checking the direct container on the entity itself, but I think we should probably be consistent with domains and check parent containers of containers to see if a user has permission to do something

@chakru-r chakru-r merged commit ee5d1e5 into datahub-project:master Jun 2, 2025
40 of 44 checks passed
kartikey-visa pushed a commit to kartikey-visa/datahub that referenced this pull request Jul 23, 2025
@yoonhyejin @kartikey-visa
feat(iceberg): add namespace permissions (datahub-project#13414)
2ff0b62 
Co-authored-by: Hyejin Yoon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

community-contribution PR or Issue raised by member(s) of DataHub Community devops PR or Issue related to DataHub backend & deployment docs Issues and Improvements to docs merge-pending-ci A PR that has passed review and should be merged once CI is green. product PR or Issue related to the DataHub UI/UX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chriscollins3456 @david-leifker @chakru-r @yoonhyejin