Fix for secret scope apply task always raises ValueError by andrascsillag-db · Pull Request #683 · databrickslabs/ucx

andrascsillag-db · 2023-12-07T13:51:37Z

Summary:

Unfortunately, both the list_acls and get_acl methods seem to be very inconsistent for checking whether a permission is applied to a given secret scope. For this reason, this PR introduces a workaround where if the permissions don't match, the acl is being applied again. From my experience of trying multiple workarounds, this was the only one that proved to be consistent.

Fixes #627

Changes:

Added extra safety checks for inflight permission check logic in secrets.py
Added integration tests in test_secrets.py

Tests:

Introduced integration tests for secrets which were missing before.

codecov · 2023-12-07T13:54:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2d982be) 81.37% compared to head (a50d916) 81.38%.

❗ Current head a50d916 differs from pull request most recent head 8eac083. Consider uploading reports for the commit 8eac083 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
+ Coverage   81.37%   81.38%   +0.01%     
==========================================
  Files          37       37              
  Lines        3828     3831       +3     
  Branches      712      712              
==========================================
+ Hits         3115     3118       +3     
  Misses        555      555              
  Partials      158      158

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nfx

Split the integration test

### Summary: Unfortunately, both the `list_acls` and `get_acl` methods seem to be very inconsistent for checking whether a permission is applied to a given secret scope. For this reason, this PR introduces a workaround where if the permissions don't match, the acl is being applied again. From my experience of trying multiple workarounds, this was the only one that proved to be consistent. **Fixes #627** ### Changes: - Added extra safety checks for inflight permission check logic in secrets.py - Added integration tests in test_secrets.py ### Tests: Introduced integration tests for secrets which were missing before.

* Added `databricks labs ucx create-table-mapping` and `databricks labs ucx manual-workspace-info` commands for CLI ([#682](#682)). * Added `databricks labs ucx installations` command ([#679](#679)). * Added `databricks labs ucx skip --schema ... --table ...` command to mark table/schema for skipping in the table migration process ([#680](#680)). * Added `workspace_group_name` and `account_group_name` to `make_ucx_group` fixture ([#664](#664)). * Added capturing `ANY FILE` and `ANONYMOUS FUNCTION` grants ([#653](#653)). * Added cluster override and handle case of write protected DBFS ([#610](#610)). * Added cluster policy selector in the installer ([#655](#655)). * Added detailed UCX pre-requisite information to README.md ([#689](#689)). * Added filters to run only cloud specific task ([#681](#681)). * Added interactive wizard for `databricks labs uninstall ucx` command ([#657](#657)). * Added more granular error retry logic ([#704](#704)). * Added parallel fetching of registered model identifiers to speed-up assessment workflow ([#691](#691)). * Added retry on workspace listing ([#659](#659)). * Added support for mapping workspace group to account group by prefix/suffix/regex/external id ([#650](#650)). * Changed cluster security mode from NONE to LEGACY_SINGLE_USER, as `crawl_tables` was failing when run on non-UC Workspace in No Isolation mode with unable to access the config file ([#661](#661)). * Changed the fields of the table "Tables" to lower case ([#684](#684)). * Cleanup README.md ([#695](#695)). * Cleanup installer framework and speed up test execution ([#711](#711)). * Decouple group configuration from `install.py` ([#714](#714)). * Enabled integration tests for `EXTERNAL` table migrations ([#677](#677)). * Enforced `mypy` validation ([#713](#713)). * Filtering out inventory database from loading into tables and filtering out the same from grant detail view ([#705](#705)). * Fixed documentation for `reflect_account_groups_on_workspace` task and updated `CONTRIBUTING.md` guide ([#654](#654)). * Fixed for secret scope apply task always raises ValueError ([#683](#683)). * Fixed some flaky integration tests ([#700](#700)). * More `mypy` chores ([#697](#697)). * Moved `ExternalLocations` and `Mounts` to `locations` module ([#692](#692)). * New CLI command for workspace mapping ([#678](#678)). * No more `mypy` chores after this ([#699](#699)). * Reduce server load for getting workspace groups and their members ([#666](#666)). * Some `mypy` chores ([#696](#696)). * Throwing ManyError on migrate-groups tasks ([#710](#710)). * Updated databricks-sdk requirement from ~=0.13.0 to ~=0.14.0 ([#651](#651)). * Updated databricks-sdk requirement from ~=0.14.0 to ~=0.15.0 ([#687](#687)). * Updated databricks-sdk requirement from ~=0.15.0 to ~=0.16.0 ([#712](#712)). * Updated installation documentation to use Databricks CLI ([#686](#686)).

* Added `databricks labs ucx create-table-mapping` and `databricks labs ucx manual-workspace-info` commands for CLI ([#682](#682)). * Added `databricks labs ucx installations` command ([#679](#679)). * Added `databricks labs ucx skip --schema ... --table ...` command to mark table/schema for skipping in the table migration process ([#680](#680)). * Added `workspace_group_name` and `account_group_name` to `make_ucx_group` fixture ([#664](#664)). * Added capturing `ANY FILE` and `ANONYMOUS FUNCTION` grants ([#653](#653)). * Added cluster override and handle case of write protected DBFS ([#610](#610)). * Added cluster policy selector in the installer ([#655](#655)). * Added detailed UCX pre-requisite information to README.md ([#689](#689)). * Added filters to run only cloud specific task ([#681](#681)). * Added interactive wizard for `databricks labs uninstall ucx` command ([#657](#657)). * Added more granular error retry logic ([#704](#704)). * Added parallel fetching of registered model identifiers to speed-up assessment workflow ([#691](#691)). * Added retry on workspace listing ([#659](#659)). * Added support for mapping workspace group to account group by prefix/suffix/regex/external id ([#650](#650)). * Changed cluster security mode from NONE to LEGACY_SINGLE_USER, as `crawl_tables` was failing when run on non-UC Workspace in No Isolation mode with unable to access the config file ([#661](#661)). * Changed the fields of the table "Tables" to lower case ([#684](#684)). * Cleanup README.md ([#695](#695)). * Cleanup installer framework and speed up test execution ([#711](#711)). * Decouple group configuration from `install.py` ([#714](#714)). * Enabled integration tests for `EXTERNAL` table migrations ([#677](#677)). * Enforced `mypy` validation ([#713](#713)). * Filtering out inventory database from loading into tables and filtering out the same from grant detail view ([#705](#705)). * Fixed documentation for `reflect_account_groups_on_workspace` task and updated `CONTRIBUTING.md` guide ([#654](#654)). * Fixed for secret scope apply task always raises ValueError ([#683](#683)). * Fixed some flaky integration tests ([#700](#700)). * More `mypy` chores ([#697](#697)). * Moved `ExternalLocations` and `Mounts` to `locations` module ([#692](#692)). * New CLI command for workspace mapping ([#678](#678)). * No more `mypy` chores after this ([#699](#699)). * Reduce server load for getting workspace groups and their members ([#666](#666)). * Some `mypy` chores ([#696](#696)). * Throwing ManyError on migrate-groups tasks ([#710](#710)). * Updated installation documentation to use Databricks CLI ([#686](#686)). Dependency updates: * Updated databricks-sdk requirement from ~=0.13.0 to ~=0.14.0 ([#651](#651)). * Updated databricks-sdk requirement from ~=0.14.0 to ~=0.15.0 ([#687](#687)). * Updated databricks-sdk requirement from ~=0.15.0 to ~=0.16.0 ([#712](#712)).

* Added `databricks labs ucx create-table-mapping` and `databricks labs ucx manual-workspace-info` commands for CLI ([#682](#682)). * Added `databricks labs ucx installations` command ([#679](#679)). * Added `databricks labs ucx skip --schema ... --table ...` command to mark table/schema for skipping in the table migration process ([#680](#680)). * Added `databricks labs ucx validate-external-locations` command for cli ([#715](#715)). * Added `workspace_group_name` and `account_group_name` to `make_ucx_group` fixture ([#664](#664)). * Added capturing `ANY FILE` and `ANONYMOUS FUNCTION` grants ([#653](#653)). * Added cluster override and handle case of write protected DBFS ([#610](#610)). * Added cluster policy selector in the installer ([#655](#655)). * Added detailed UCX pre-requisite information to README.md ([#689](#689)). * Added filters to run only cloud specific task ([#681](#681)). * Added interactive wizard for `databricks labs uninstall ucx` command ([#657](#657)). * Added more granular error retry logic ([#704](#704)). * Added parallel fetching of registered model identifiers to speed-up assessment workflow ([#691](#691)). * Added retry on workspace listing ([#659](#659)). * Added support for mapping workspace group to account group by prefix/suffix/regex/external id ([#650](#650)). * Changed cluster security mode from NONE to LEGACY_SINGLE_USER, as `crawl_tables` was failing when run on non-UC Workspace in No Isolation mode with unable to access the config file ([#661](#661)). * Changed the fields of the table "Tables" to lower case ([#684](#684)). * Cleanup README.md ([#695](#695)). * Cleanup installer framework and speed up test execution ([#711](#711)). * Decouple group configuration from `install.py` ([#714](#714)). * Enabled integration tests for `EXTERNAL` table migrations ([#677](#677)). * Enforced `mypy` validation ([#713](#713)). * Filtering out inventory database from loading into tables and filtering out the same from grant detail view ([#705](#705)). * Fixed documentation for `reflect_account_groups_on_workspace` task and updated `CONTRIBUTING.md` guide ([#654](#654)). * Fixed for secret scope apply task always raises ValueError ([#683](#683)). * Fixed some flaky integration tests ([#700](#700)). * More `mypy` chores ([#697](#697)). * Moved `ExternalLocations` and `Mounts` to `locations` module ([#692](#692)). * New CLI command for workspace mapping ([#678](#678)). * No more `mypy` chores after this ([#699](#699)). * Reduce server load for getting workspace groups and their members ([#666](#666)). * Some `mypy` chores ([#696](#696)). * Throwing ManyError on migrate-groups tasks ([#710](#710)). * Updated installation documentation to use Databricks CLI ([#686](#686)). Dependency updates: * Updated databricks-sdk requirement from ~=0.13.0 to ~=0.14.0 ([#651](#651)). * Updated databricks-sdk requirement from ~=0.14.0 to ~=0.15.0 ([#687](#687)). * Updated databricks-sdk requirement from ~=0.15.0 to ~=0.16.0 ([#712](#712)).

* Added `databricks labs ucx create-table-mapping` and `databricks labs ucx manual-workspace-info` commands for CLI ([#682](#682)). * Added `databricks labs ucx ensure-assessment-run` to CLI commands ([#708](#708)). * Added `databricks labs ucx installations` command ([#679](#679)). * Added `databricks labs ucx skip --schema ... --table ...` command to mark table/schema for skipping in the table migration process ([#680](#680)). * Added `databricks labs ucx validate-external-locations` command for cli ([#715](#715)). * Added capturing `ANY FILE` and `ANONYMOUS FUNCTION` grants ([#653](#653)). * Added cluster override and handle case of write protected DBFS ([#610](#610)). * Added cluster policy selector in the installer ([#655](#655)). * Added detailed UCX pre-requisite information to README.md ([#689](#689)). * Added interactive wizard for `databricks labs uninstall ucx` command ([#657](#657)). * Added more granular error retry logic ([#704](#704)). * Added parallel fetching of registered model identifiers to speed-up assessment workflow ([#691](#691)). * Added retry on workspace listing ([#659](#659)). * Added support for mapping workspace group to account group by prefix/suffix/regex/external id ([#650](#650)). * Changed cluster security mode from NONE to LEGACY_SINGLE_USER, as `crawl_tables` was failing when run on non-UC Workspace in No Isolation mode with unable to access the config file ([#661](#661)). * Changed the fields of the table "Tables" to lower case ([#684](#684)). * Enabled integration tests for `EXTERNAL` table migrations ([#677](#677)). * Enforced `mypy` validation ([#713](#713)). * Filtering out inventory database from loading into tables and filtering out the same from grant detail view ([#705](#705)). * Fixed documentation for `reflect_account_groups_on_workspace` task and updated `CONTRIBUTING.md` guide ([#654](#654)). * Fixed secret scope apply task to raise ValueError ([#683](#683)). * Fixed legacy table ACL ownership migration and other integration testing issues ([#722](#722)). * Fixed some flaky integration tests ([#700](#700)). * New CLI command for workspace mapping ([#678](#678)). * Reduce server load for getting workspace groups and their members ([#666](#666)). * Throwing ManyError on migrate-groups tasks ([#710](#710)). * Updated installation documentation to use Databricks CLI ([#686](#686)). Dependency updates: * Updated databricks-sdk requirement from ~=0.13.0 to ~=0.14.0 ([#651](#651)). * Updated databricks-sdk requirement from ~=0.14.0 to ~=0.15.0 ([#687](#687)). * Updated databricks-sdk requirement from ~=0.15.0 to ~=0.16.0 ([#712](#712)).

andrascsillag-db added 3 commits December 7, 2023 14:41

checking permission after applying

e4a5cd8

add integration test for secrets

219d0b7

add logic to fix the incosistent list_acls api

01c6aeb

andrascsillag-db requested review from a team and larsgeorge-db December 7, 2023 13:51

andrascsillag-db had a problem deploying to account-admin December 7, 2023 13:51 — with GitHub Actions Failure

andrascsillag-db requested a review from william-conti December 7, 2023 13:51

nfx requested changes Dec 7, 2023

View reviewed changes

Comment thread src/databricks/labs/ucx/workspace_access/secrets.py

Comment thread tests/integration/workspace_access/test_secrets.py Outdated

Comment thread tests/integration/workspace_access/test_secrets.py Outdated

renaming and adding unit tests

cbbd34a

andrascsillag-db had a problem deploying to account-admin December 7, 2023 15:34 — with GitHub Actions Failure

remove iteration from integration test

58e6121

andrascsillag-db had a problem deploying to account-admin December 7, 2023 15:58 — with GitHub Actions Failure

dmoore247 suggested changes Dec 7, 2023

View reviewed changes

Comment thread tests/unit/workspace_access/test_secrets.py

added more unit tests

a50d916

andrascsillag-db had a problem deploying to account-admin December 8, 2023 11:00 — with GitHub Actions Failure

andrascsillag-db requested review from dmoore247 and nfx December 8, 2023 16:06

nfx requested changes Dec 9, 2023

View reviewed changes

reducing scope for secrets integration test

eb8e79c

andrascsillag-db had a problem deploying to account-admin December 11, 2023 14:14 — with GitHub Actions Failure

fix formatting

8eac083

andrascsillag-db had a problem deploying to account-admin December 11, 2023 14:16 — with GitHub Actions Failure

andrascsillag-db requested a review from nfx December 11, 2023 14:21

nfx approved these changes Dec 11, 2023

View reviewed changes

nfx merged commit 3e19372 into main Dec 11, 2023

nfx deleted the fix/issue_627 branch December 11, 2023 23:33

nfx mentioned this pull request Dec 21, 2023

Release v0.7.0 #719

Closed

nfx mentioned this pull request Dec 21, 2023

Release v0.7.0 #720

Closed

nfx mentioned this pull request Dec 21, 2023

Release v0.7.0 #723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for secret scope apply task always raises ValueError#683

Fix for secret scope apply task always raises ValueError#683
nfx merged 8 commits intomainfrom
fix/issue_627

andrascsillag-db commented Dec 7, 2023

Uh oh!

codecov bot commented Dec 7, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nfx left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

andrascsillag-db commented Dec 7, 2023

Summary:

Changes:

Tests:

Uh oh!

codecov bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 7, 2023 •

edited

Loading