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.

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

Fix segmentation fault when unassigning role assignments #213

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

vinay-gopalan
Copy link
Contributor

Overview

The error handling case in unassignRoles was misconstrued, and was causing a panic when unassigning role assignments would fail during a WAL Rollback. This PR appropriately parses the received error from Azure and fixes the if cases to remove the panic case.

A unit test with a mock provider was added to confirm that the plugin can correctly

  • return errors when needed
  • ignore errors if the status code is 204 or 404 (role assignment was manually deleted)

Related Issues/Pull Requests

[x] [Issue #190]
[x] [PR #210]

Contributor Checklist

[x] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
My Docs PR Link
Example
[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible

test output without fix:

=== RUN   TestUnassignRoleFailures
=== RUN   TestUnassignRoleFailures/Role_unassign_fail
--- FAIL: TestUnassignRoleFailures (0.00s)
    --- FAIL: TestUnassignRoleFailures/Role_unassign_fail (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x3b4b916]

test output with fix:

=== RUN   TestUnassignRoleFailures
=== RUN   TestUnassignRoleFailures/Role_unassign_fail
=== RUN   TestUnassignRoleFailures/Role_unassign_error_handled
--- PASS: TestUnassignRoleFailures (0.00s)
    --- PASS: TestUnassignRoleFailures/Role_unassign_fail (0.00s)
    --- PASS: TestUnassignRoleFailures/Role_unassign_error_handled (0.00s)
PASS

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looks good. The approach to the evaluate there response error was definitely the right way to go.

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
provider_mock_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM!

@vinay-gopalan vinay-gopalan merged commit 2335312 into main Jul 1, 2024
4 checks passed
@vinay-gopalan vinay-gopalan deleted the VAULT-26583/unassign-roles-panic branch July 1, 2024 17: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