Skip to content
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

Automatically add CAN_MANAGE permission on databricks_sql_endpoint for calling user #2168

Merged
merged 3 commits into from
Mar 31, 2023
Merged

Automatically add CAN_MANAGE permission on databricks_sql_endpoint for calling user #2168

merged 3 commits into from
Mar 31, 2023

Conversation

nkvuong
Copy link
Contributor

@nkvuong nkvuong commented Mar 28, 2023 •
edited
Loading

Changes

Tests

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • relevant acceptance tests are passing
  • using Go SDK

@nkvuong nkvuong changed the title Add CAN_MANAGE permission to databricks_sql_endpoint permission Automatically add CAN_MANAGE permission to databricks_sql_endpoint permission Mar 28, 2023
@mkazia-db
Copy link

Why is this handled different from clusters? Why not add "/sql/warehouses/" in function shouldExplicitlyGrantCallingUserManagePermissions

@nfx
Copy link
Contributor

nfx commented Mar 29, 2023

@mkazia-db because our API is not consistent

nfx
nfx requested changes Mar 29, 2023
View reviewed changes
permissions/resource_permissions.go Outdated
return err
}
if strings.HasPrefix(objectID, "/sql/warehouses") {
warehouse, err := w.Warehouses.GetById(a.context, strings.ReplaceAll(objectID, "/sql/warehouses/", ""))
Copy link
Contributor

Choose a reason for hiding this comment

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

for debuggability, could you extract strings.ReplaceAll(objectID, "/sql/warehouses/", "") to a variable?

permissions/resource_permissions.go Outdated
}
// add CAN_MANAGE permission for creator
objectACL.AccessControlList = append(objectACL.AccessControlList, AccessControlChange{
UserName: warehouse.CreatorName,
Copy link
Contributor

Choose a reason for hiding this comment

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

will it work if it's a service principal? check azure-prod env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our API accepts service principal as UserName fine...which is the cause of #2131

permissions/resource_permissions.go Outdated Show resolved Hide resolved
@nkvuong nkvuong force-pushed the fix/sql_warehouse_permission branch from fa81de6 to 1bcb165 Compare March 29, 2023 13:14
@nkvuong
Copy link
Contributor Author

nkvuong commented Mar 29, 2023

@mkazia-db Adding warehouse shouldExplicitlyGrantCallingUserManagePermissions will fix this issue (calling user permission is removed), but not the edge case where the creator is not the same as the calling user

@nfx when I tested this further, automatically adding permission to the warehouse creator results in permanent diff (as the extra permission is returned in the API). For now I'll apply the quick fix, and handle that extra edge case properly

@codecov-commenter
Copy link

Codecov Report

Merging #2168 (a91d07e) into master (9e1eb13) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2168      +/-   ##
==========================================
- Coverage   88.77%   88.74%   -0.03%     
==========================================
  Files         137      137              
  Lines       11211    11217       +6     
==========================================
+ Hits         9953     9955       +2     
- Misses        851      853       +2     
- Partials      407      409       +2
Impacted Files Coverage Δ
permissions/resource_permissions.go 88.84% <50.00%> (-1.42%) ⬇️

@nkvuong nkvuong changed the title Automatically add CAN_MANAGE permission to databricks_sql_endpoint permission Automatically add CAN_MANAGE permission on databricks_sql_endpoint for calling user Mar 29, 2023
@nkvuong
Copy link
Contributor Author

nkvuong commented Mar 29, 2023

acceptance tests passed

@nkvuong nkvuong marked this pull request as ready for review March 29, 2023 14:24
@shreyas-goenka shreyas-goenka requested a review from nfx March 30, 2023 11:18
@nfx nfx merged commit 314c1ad into master Mar 31, 2023
@nfx nfx deleted the fix/sql_warehouse_permission branch March 31, 2023 10:39
@nfx nfx mentioned this pull request Mar 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfx nfx nfx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ISSUE] Issue with databricks_permissions resource on databricks_sql_endpoint
4 participants
@nkvuong @mkazia-db @nfx @codecov-commenter