-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat(db_engine): Implement user impersonation support for StarRocks #28110
feat(db_engine): Implement user impersonation support for StarRocks #28110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Woellchen for the change. Would you mind adding unit/integration tests to help ensure we don't have future regressions? |
Thanks @john-bodley! Sure, I will look into it :) |
Looks like the pre-commit hooks also need to be run to make the linters happy. |
51b878d
to
b94ea18
Compare
e0d6b38
to
8cf48bf
Compare
Hey @rusackas and @john-bodley, I have tried to address your comments. Could you please another look at it? :) |
Running CI... fingers crossed! |
@Woellchen can you rebase the PR to restart CI? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we need to rebase the PR to ensure tests are running ok on the other dbs due to the sig changes. Please ping me for a new review once done.
8cf48bf
to
25619a2
Compare
Hey @villebro! I have rebased the branch and verified that everything still works as expected :) Thank you |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #28110 +/- ##
===========================================
+ Coverage 60.48% 83.65% +23.16%
===========================================
Files 1931 529 -1402
Lines 76236 38312 -37924
Branches 8568 0 -8568
===========================================
- Hits 46114 32051 -14063
+ Misses 28017 6261 -21756
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
25619a2
to
4147eeb
Compare
I had missed one usage for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the patience and apologies for the loooong review process, excited to see this finally get merged! 🚀
No problem, glad we merged it! Thank you too! |
SUMMARY
This PR adds user impersonation support for the StarRocks database by leveraging the native
EXECUTE AS {user} WITH NO REVERT
user management query.TESTING INSTRUCTIONS
SELECT CURRENT_USER();
and see that it shows your current Superset userSELECT CURRENT_USER();
again and see that it changed back to the user of the StarRocks connectionADDITIONAL INFORMATION