fix(drill): specify an SA URL parm of impersonation_target for drill+sadrill
#19252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…et". Sqlalchemy-drill is being updated to support impersonation with the drill+sadrill driver, where previously it did not. The way that callers should specify impersonation matches that for the drill+jdbc driver in that a SA URL parameter of impersonation_target should be set to the username of the user to be impersonated, while the stadard SA username and password should be those of the proxy user.
Codecov Report
@@ Coverage Diff @@
## master #19252 +/- ##
==========================================
- Coverage 66.76% 66.63% -0.13%
==========================================
Files 1670 1673 +3
Lines 64398 63977 -421
Branches 6499 6499
==========================================
- Hits 42993 42633 -360
+ Misses 19722 19661 -61
Partials 1683 1683
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Bump? |
villebro
reviewed
Mar 31, 2022
| assert url.username == username | ||
| assert url.query["impersonation_target"] == username |
There was a problem hiding this comment.
nit: bonus points for adding impersonation tests for drill+odbc, drill+jdbc and drill+foobar.
There was a problem hiding this comment.
The first two cases are already higher up in test_drill.py. I've now added the drill+foobar case.
villebro
reviewed
Mar 31, 2022
Comment on lines
86
to
91
| try: | ||
| DrillEngineSpec.modify_url_for_impersonation(url, True, username) | ||
| except Exception as e: | ||
| exception_type = type(e) | ||
|
|
||
| assert exception_type == SupersetDBAPIProgrammingError |
There was a problem hiding this comment.
nit: this is how it's usually done with pytest:
Suggested change
| try: | |
| DrillEngineSpec.modify_url_for_impersonation(url, True, username) | |
| except Exception as e: | |
| exception_type = type(e) | |
| assert exception_type == SupersetDBAPIProgrammingError | |
| with pytest.raises(SupersetDBAPIProgrammingError): | |
| DrillEngineSpec.modify_url_for_impersonation(url, True, username) |
There was a problem hiding this comment.
Thanks! I'll push again and watch the CI output, it looks like there might be some problem with my use of the SupersetDBAPIProgrammingError type in this module.
There was a problem hiding this comment.
Oh - you may need to move the exception import into the test function
villebro
approved these changes
Mar 31, 2022
villebro
pushed a commit
that referenced
this pull request
Apr 3, 2022
…l+sadrill (#19252) * Update drill+sadrill to specify an SA URL parm of "impersonation_target". Sqlalchemy-drill is being updated to support impersonation with the drill+sadrill driver, where previously it did not. The way that callers should specify impersonation matches that for the drill+jdbc driver in that a SA URL parameter of impersonation_target should be set to the username of the user to be impersonated, while the stadard SA username and password should be those of the proxy user. * Remove lint. * Address review comments. * Use idiomatic pytest to test for a raised exception. * Fix import statement order in drill.py. (cherry picked from commit 85e330e)
villebro
pushed a commit
that referenced
this pull request
Apr 4, 2022
…l+sadrill (#19252) * Update drill+sadrill to specify an SA URL parm of "impersonation_target". Sqlalchemy-drill is being updated to support impersonation with the drill+sadrill driver, where previously it did not. The way that callers should specify impersonation matches that for the drill+jdbc driver in that a SA URL parameter of impersonation_target should be set to the username of the user to be impersonated, while the stadard SA username and password should be those of the proxy user. * Remove lint. * Address review comments. * Use idiomatic pytest to test for a raised exception. * Fix import statement order in drill.py. (cherry picked from commit 85e330e)
philipher29
pushed a commit
to ValtechMobility/superset
that referenced
this pull request
Jun 9, 2022
…l+sadrill (apache#19252) * Update drill+sadrill to specify an SA URL parm of "impersonation_target". Sqlalchemy-drill is being updated to support impersonation with the drill+sadrill driver, where previously it did not. The way that callers should specify impersonation matches that for the drill+jdbc driver in that a SA URL parameter of impersonation_target should be set to the username of the user to be impersonated, while the stadard SA username and password should be those of the proxy user. * Remove lint. * Address review comments. * Use idiomatic pytest to test for a raised exception. * Fix import statement order in drill.py.
mistercrunch
added
🍒 1.5.0
🍒 1.5.1
🍒 1.5.2
🍒 1.5.3
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Sqlalchemy-drill has been updated to support impersonation with the
drill+sadrill driver, where previously it did not. The way that callers
should specify impersonation matches that for the drill+jdbc driver in that
a SA URL parameter of impersonation_target should be set to the username
of the user to be impersonated, while the standard SA username and password
should be those of the proxy user.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
Configure a new Apache Drill connection using the drill+sadrill driver from
sqlalchemy-drill. Enable impersonation and trigger the execution of a Drill
query. In the Drill query profile history, confirm that the query was executed
as the impersonated user.
ADDITIONAL INFORMATION