Refactor Azure service principal crawler and fix bug where tenant_id inside secret scope is not detected
#942
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
# Conflicts: # src/databricks/labs/ucx/assessment/azure.py # tests/unit/assessment/test_azure.py
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #942 +/- ##
==========================================
+ Coverage 87.82% 87.88% +0.06%
==========================================
Files 43 43
Lines 5230 5185 -45
Branches 945 929 -16
==========================================
- Hits 4593 4557 -36
+ Misses 434 432 -2
+ Partials 203 196 -7 ☔ View full report in Codecov by Sentry. |
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nfx
requested changes
Feb 13, 2024
| } | ||
| ) | ||
| return spn_list | ||
| return list(generate_service_principals(deduped_service_principals)) |
There was a problem hiding this comment.
if you make a set of AzureServicePrincipalInfo and return those directly instead of working with dictionaries - you don't need a dedupe block - dataclasses already have __hash__.
| storage_account: str | None | ||
|
|
||
|
|
||
| def generate_service_principals(service_principals: list[dict]): |
There was a problem hiding this comment.
we don't need this top-level function ;)
There was a problem hiding this comment.
also cleaned up some nonsensical tests
| @@ -23,161 +23,113 @@ | |||
| @dataclass | |||
| class AzureServicePrincipalInfo: | |||
| # fs.azure.account.oauth2.client.id | |||
| application_id: str | |||
| application_id: str | None | |||
There was a problem hiding this comment.
isn't it broken if all fields are none? we need at least application_id
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nfx
requested changes
Feb 14, 2024
| tenant_id = client_endpoint_list[3] | ||
|
|
||
| # add output to spn list | ||
| spn_list.append( |
There was a problem hiding this comment.
i meant creating the AzureServicePrincipalInfo from here directly, without using intermediate dictionary structure. can you change this place and the other couple of places where we construct this dict?
| assert secret.value is not None | ||
| return base64.b64decode(secret.value).decode("utf-8") | ||
| except NotFound: | ||
| logger.warning(f'removed on the backend: {secret_scope}"/"{secret_key}') |
There was a problem hiding this comment.
Suggested change
| logger.warning(f'removed on the backend: {secret_scope}"/"{secret_key}') | |
| logger.warning(f'removed on the backend: {secret_scope}/{secret_key}') |
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
changed the title
nkvuong
changed the title
tenant_id inside secret scope is not detected
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nkvuong
had a problem deploying
to
account-admin
GitHub Actions
Failure
nfx
added a commit
that referenced
this pull request
Feb 16, 2024
* Added CLI Command `databricks labs ucx principal-prefix-access` ([#949](#949)). * Added a widget with all jobs to track migration progress ([#940](#940)). * Added legacy cluster types to the assessment result ([#932](#932)). * Cleanup of install documentation ([#951](#951), [#947](#947)). * Fixed `WorkspaceConfig` initialization for `DEBUG` notebook ([#934](#934)). * Fixed installer not opening config file during the installation ([#945](#945)). * Fixed groups in config file not considered for group migration job ([#943](#943)). * Fixed bug where `tenant_id` inside secret scope is not detected ([#942](#942)).
Merged
nfx
added a commit
that referenced
this pull request
Feb 16, 2024
* Added CLI Command `databricks labs ucx principal-prefix-access` ([#949](#949)). * Added a widget with all jobs to track migration progress ([#940](#940)). * Added legacy cluster types to the assessment result ([#932](#932)). * Cleanup of install documentation ([#951](#951), [#947](#947)). * Fixed `WorkspaceConfig` initialization for `DEBUG` notebook ([#934](#934)). * Fixed installer not opening config file during the installation ([#945](#945)). * Fixed groups in config file not considered for group migration job ([#943](#943)). * Fixed bug where `tenant_id` inside secret scope is not detected ([#942](#942)).
dmoore247
pushed a commit
that referenced
this pull request
Mar 23, 2024
…` inside secret scope is not detected (#942) ## Changes - Fix tenant_id detection logic In the case where spn endpoint configuration is also inside a secret scope. - Refactoring spn crawler logic to make it more readable ### Linked issues Closes #896 ### Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [x] manually tested - [ ] added unit tests - [ ] added integration tests - [ ] verified on staging environment (screenshot attached)
dmoore247
pushed a commit
that referenced
this pull request
Mar 23, 2024
* Added CLI Command `databricks labs ucx principal-prefix-access` ([#949](#949)). * Added a widget with all jobs to track migration progress ([#940](#940)). * Added legacy cluster types to the assessment result ([#932](#932)). * Cleanup of install documentation ([#951](#951), [#947](#947)). * Fixed `WorkspaceConfig` initialization for `DEBUG` notebook ([#934](#934)). * Fixed installer not opening config file during the installation ([#945](#945)). * Fixed groups in config file not considered for group migration job ([#943](#943)). * Fixed bug where `tenant_id` inside secret scope is not detected ([#942](#942)).
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.
Changes
Linked issues
Closes #896
Tests