Skip to content

Conversation

@btipling
Copy link
Contributor

@btipling btipling commented Mar 17, 2025

The main issue I fixed was that Slug is an empty string and cannot be used for provisioning Grant and Revoke methods.

AI summary

This PR enhances the Databricks connector's group membership and permissions handling, focusing on handling of already-existing conditions and conflicts.

Key Changes

  1. Error Handling Enhancements
    • Added AlreadyExists constant for standardizing conflict handling
    • Improved APIError structure with better error details and status codes
    • Graceful handling of HTTP 409 conflicts
  2. Group Membership Logic
    • Enhanced member existence checks before modifications
    • Improved logging for already-existing conditions
    • Better handling of role permission assignments
  3. Logging Improvements
    • Added informative log messages for common scenarios
    • Better context in error messages

Changes:

  1. Group Membership Logic (in groups.go):
    • The Grant method (lines 265-401) handles adding members to groups and assigning permissions
    • The Revoke method (lines 403-538) handles removing members from groups and revoking permissions

  2. Error Handling for Existing Memberships:

In the Grant method:
• For group memberships, it checks if the member already exists (lines 312-322)
• If the member is already in the group, it logs an informative message and returns success (not an error)
• For role permissions, it checks if the principal already has the role (lines 366-374)
• Returns success if the role is already assigned

In the Revoke method:
• For group memberships, it efficiently removes the member if found
• For role permissions, it checks if the entitlement is already removed (lines 484-492)
• Logs informative messages when attempting to revoke non-existent permissions (lines 516-522)

  1. Conflict Handling:

The APIError structure (in request.go) has been enhanced:
• Defines a constant AlreadyExists (line 18) for conflict scenarios
• The APIError struct (lines 22-27) includes:
◦ StatusCode: For HTTP status codes
◦ Detail: For specific error details
◦ Message: For human-readable messages
◦ Err: The underlying error

Both Grant and Revoke methods handle conflicts (HTTP 409) gracefully:
• They check for APIError with StatusCode == http.StatusConflict
• If the Detail is "AlreadyExists", they return success instead of an error
• This prevents unnecessary error responses for already-applied changes

The improvements ensure:

  1. Idempotent operations - repeated operations don't cause errors
  2. Clear logging of already-existing conditions
  3. Proper handling of API conflicts
  4. User-friendly success responses instead of unnecessary errors

These changes make the connector more robust and user-friendly by:
• Preventing duplicate error messages
• Handling edge cases gracefully
• Providing clear feedback through logs
• Maintaining consistent state management

@btipling btipling marked this pull request as ready for review March 17, 2025 21:39
if slices.Contains(ruleSet.Principals, principalId) {
// if there is only one principal, remove the whole rule set
if len(ruleSet.Principals) == 1 {
ruleSets = slices.Delete(ruleSets, i, i+1)
Copy link

@johnallers johnallers Mar 18, 2025

Choose a reason for hiding this comment

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

@btipling Is it ok in go to delete from and change the value of a slice that is being iterated over?

Copy link
Contributor Author

@btipling btipling Mar 18, 2025

Choose a reason for hiding this comment

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

@johnallers it's not ok, fixed in ca9270d

@btipling btipling merged commit 9cdba51 into main Mar 18, 2025
3 checks passed
@btipling btipling deleted the bt/fix_groups branch March 18, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants