feat: BED5023 Collect AZContributor role assignments for Mgmt Groups, RGs, and Subscriptions#172
Open
StranDutton wants to merge 5 commits intomainfrom
Open
feat: BED5023 Collect AZContributor role assignments for Mgmt Groups, RGs, and Subscriptions#172StranDutton wants to merge 5 commits intomainfrom
StranDutton wants to merge 5 commits intomainfrom
Conversation
WalkthroughThis PR adds support for listing Azure role assignments specifically for Contributor roles across Management Groups, Resource Groups, and Subscriptions, introducing three new CLI commands, corresponding data models, enum constants, and refactoring the core role-assignment pipeline. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5 tasks
StranDutton
changed the title
Contributor
Author
|
Unit test failures are due to a race condition in an existing test - writing a bug ticket to fix |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cmd/list-subscription-contributors_test.go (1)
67-73: Assert the emitted contributor payload, not just its type.This test still passes if the implementation emits zero contributors or contributors with an empty
SubscriptionId. Please also verifyKind, contributor count, and the propagated subscription scope ID here.Suggested assertion upgrade
if result, ok := <-channel; !ok { t.Fatalf("failed to receive from channel") } else if wrapper, ok := result.(AzureWrapper); !ok { t.Errorf("failed type assertion: got %T, want %T", result, AzureWrapper{}) - } else if _, ok := wrapper.Data.(models.SubscriptionContributors); !ok { + } else if wrapper.Kind != enums.KindAZSubscriptionContributor { + t.Errorf("got kind %v, want %v", wrapper.Kind, enums.KindAZSubscriptionContributor) + } else if data, ok := wrapper.Data.(models.SubscriptionContributors); !ok { t.Errorf("failed type assertion: got %T, want %T", wrapper.Data, models.SubscriptionContributors{}) + } else if len(data.Contributors) != 1 { + t.Errorf("got %d contributors, want 1", len(data.Contributors)) + } else if data.Contributors[0].SubscriptionId != "foo" { + t.Errorf("got subscriptionId %q, want %q", data.Contributors[0].SubscriptionId, "foo") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list-subscription-contributors_test.go` around lines 67 - 73, The test currently only asserts types from the channel result; enhance it to validate the actual SubscriptionContributors payload by casting wrapper.Data to models.SubscriptionContributors and then asserting the payload.Kind equals the expected kind string, the Contributors slice length matches the expected count (>0 or specific number), and the propagated subscription scope ID (e.g., SubscriptionId or Scope field) equals the expected value used in the test setup; update the assertions around channel, AzureWrapper, and models.SubscriptionContributors to check these concrete fields (Kind, Contributors length, and SubscriptionId/Scope) rather than only types.cmd/list-resource-group-contributors.go (1)
20-24: UseSIGTERMhere instead ofos.Kill.
os.Kill/SIGKILL can’t be caught, so this context only reacts to Ctrl-C. If this command should shut down cleanly on process termination,syscall.SIGTERMis the catchable signal to register.♻️ Suggested change
import ( "context" "os" "os/signal" + "syscall" "time" @@ - ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, os.Kill) + ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM)Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list-resource-group-contributors.go` around lines 20 - 24, Replace uses of os.Kill in your signal registration with the catchable syscall.SIGTERM so the process can handle termination (SIGTERM) instead of the uncapturable SIGKILL; update the signal.Notify call(s) that currently reference os.Kill to use syscall.SIGTERM and add the syscall import (alongside context, os, os/signal, time) so the cancellation context reacts to external termination as well as Ctrl-C.cmd/list-management-group-contributors_test.go (1)
69-75: Assert the mapped contributor payload in the happy-path test.Right now this passes as long as something is emitted. Please assert the wrapper kind and contributor data so the test actually guards the new Contributor edge behavior.
✅ Suggested assertion upgrade
- if _, ok := <-channel; !ok { - t.Fatalf("failed to receive from channel") - } + if result, ok := <-channel; !ok { + t.Fatalf("failed to receive from channel") + } else if wrapper, ok := result.(azureWrapper[models.ManagementGroupContributors]); !ok { + t.Fatalf("failed type assertion: got %T, want azureWrapper[models.ManagementGroupContributors]", result) + } else if wrapper.Kind != enums.KindAZManagementGroupContributor { + t.Fatalf("got kind %v, want %v", wrapper.Kind, enums.KindAZManagementGroupContributor) + } else if wrapper.Data.ManagementGroupId != "foo" || len(wrapper.Data.Contributors) != 1 { + t.Fatalf("got %+v, want one contributor for management group foo", wrapper.Data) + } else if wrapper.Data.Contributors[0].Contributor.Name != constants.ContributorRoleID { + t.Fatalf("got contributor %q, want %q", wrapper.Data.Contributors[0].Contributor.Name, constants.ContributorRoleID) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list-management-group-contributors_test.go` around lines 69 - 75, The test currently only verifies that something is sent on channel; change the two receives to capture the emitted value (e.g., v, ok := <-channel), assert ok is true, then inspect v's wrapper kind and the contained Contributor payload to ensure the wrapper.Kind equals the expected kind and Contributor fields (ID, Name, Email or whatever fields your Contributor type uses) match the expected mapped contributor values for the happy path; finally keep the second receive asserting the channel is closed (ok == false) or no further emission. Reference the channel variable and the Contributor wrapper/type when locating where to add these assertions.cmd/list-management-group-contributors.go (1)
20-24: UseSIGTERMhere instead ofos.Kill.
os.Kill/SIGKILL can’t be caught, so this context only reacts to Ctrl-C. If this command should shut down cleanly on process termination,syscall.SIGTERMis the catchable signal to register.♻️ Suggested change
import ( "context" "os" "os/signal" + "syscall" "time" @@ - ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, os.Kill) + ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM)Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list-management-group-contributors.go` around lines 20 - 24, The code currently registers os.Kill (SIGKILL) with signal.Notify which is uncatchable; replace uses of os.Kill with syscall.SIGTERM and import "syscall" so the context cancellation in your signal handling (the signal.Notify call and any checks around it in cmd/list-management-group-contributors.go) will also respond to process termination; update both occurrences where os.Kill is passed to signal.Notify (and any related comparisons) to use syscall.SIGTERM and ensure the syscall import is added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/list-subscription-contributors.go`:
- Around line 87-90: The SubscriptionContributor being constructed sets
SubscriptionId from item.SubscriptionId which is empty in fixtures; change it to
use the batch-level subscription ID instead (e.g., set SubscriptionId:
batch.SubscriptionId) so each SubscriptionContributor
(models.SubscriptionContributor created in the loop that assigns Contributor:
item.RoleAssignment) inherits the stream/grouping subscription scope.
- Around line 72-75: The loop currently does an inline chained assertion
result.(AzureWrapper).Data.(models.SubscriptionRoleAssignments) which will panic
if result is not an AzureWrapper; instead first assert and check the outer type:
cast result to AzureWrapper with an ok check (e.g., aw, ok :=
result.(AzureWrapper)); if not ok, log the type error and continue; then assert
aw.Data to models.SubscriptionRoleAssignments with its own ok check, log and
continue on failure; update the loop around pipeline.OrDone(ctx.Done(),
roleAssignments) to use these two guarded checks so non-AzureWrapper items never
cause a panic.
- Around line 48-50: In listSubscriptionContributorsCmdImpl, the
signal.NotifyContext call registers os.Kill which cannot be caught; replace
os.Kill with syscall.SIGTERM (and add the syscall import) so the context will be
cancelled on a catchable termination signal; update the signal.NotifyContext
invocation accordingly and ensure gracefulShutdown(stop) remains deferred.
---
Nitpick comments:
In `@cmd/list-management-group-contributors_test.go`:
- Around line 69-75: The test currently only verifies that something is sent on
channel; change the two receives to capture the emitted value (e.g., v, ok :=
<-channel), assert ok is true, then inspect v's wrapper kind and the contained
Contributor payload to ensure the wrapper.Kind equals the expected kind and
Contributor fields (ID, Name, Email or whatever fields your Contributor type
uses) match the expected mapped contributor values for the happy path; finally
keep the second receive asserting the channel is closed (ok == false) or no
further emission. Reference the channel variable and the Contributor
wrapper/type when locating where to add these assertions.
In `@cmd/list-management-group-contributors.go`:
- Around line 20-24: The code currently registers os.Kill (SIGKILL) with
signal.Notify which is uncatchable; replace uses of os.Kill with syscall.SIGTERM
and import "syscall" so the context cancellation in your signal handling (the
signal.Notify call and any checks around it in
cmd/list-management-group-contributors.go) will also respond to process
termination; update both occurrences where os.Kill is passed to signal.Notify
(and any related comparisons) to use syscall.SIGTERM and ensure the syscall
import is added.
In `@cmd/list-resource-group-contributors.go`:
- Around line 20-24: Replace uses of os.Kill in your signal registration with
the catchable syscall.SIGTERM so the process can handle termination (SIGTERM)
instead of the uncapturable SIGKILL; update the signal.Notify call(s) that
currently reference os.Kill to use syscall.SIGTERM and add the syscall import
(alongside context, os, os/signal, time) so the cancellation context reacts to
external termination as well as Ctrl-C.
In `@cmd/list-subscription-contributors_test.go`:
- Around line 67-73: The test currently only asserts types from the channel
result; enhance it to validate the actual SubscriptionContributors payload by
casting wrapper.Data to models.SubscriptionContributors and then asserting the
payload.Kind equals the expected kind string, the Contributors slice length
matches the expected count (>0 or specific number), and the propagated
subscription scope ID (e.g., SubscriptionId or Scope field) equals the expected
value used in the test setup; update the assertions around channel,
AzureWrapper, and models.SubscriptionContributors to check these concrete fields
(Kind, Contributors length, and SubscriptionId/Scope) rather than only types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 596e4646-6b6c-48d9-a411-7d5715f0a35d
📒 Files selected for processing (12)
.gitignorecmd/list-azure-rm.gocmd/list-management-group-contributors.gocmd/list-management-group-contributors_test.gocmd/list-resource-group-contributors.gocmd/list-resource-group-contributors_test.gocmd/list-subscription-contributors.gocmd/list-subscription-contributors_test.goenums/kind.gomodels/management-group-contributor.gomodels/resource-group-contributor.gomodels/subscription-contributor.go
Comment on lines
+48
to
+50
| func listSubscriptionContributorsCmdImpl(cmd *cobra.Command, args []string) { | ||
| ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, os.Kill) | ||
| defer gracefulShutdown(stop) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Go's signal.NotifyContext receive os.Kill / SIGKILL, or are SIGKILL and SIGSTOP uncatchable?
💡 Result:
No. signal.NotifyContext (like signal.Notify) cannot receive SIGKILL/os.Kill, and it also cannot receive SIGSTOP.
On Unix-like systems, SIGKILL and SIGSTOP are uncatchable (they “may not be caught by a program”), so they cannot be delivered through Go’s os/signal package—meaning they will not cancel a NotifyContext context. [1][2]
Sources:
[1] Go os/signal package docs (pkg.go.dev) (pkg.go.dev)
[2] Go os/signal docs source (doc.go) (go.dev)
Citations:
Don't register os.Kill with signal.NotifyContext.
os.Kill (SIGKILL) cannot be intercepted on Unix-like systems, so it will never trigger graceful shutdown through this context. Use syscall.SIGTERM instead for catchable termination signals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/list-subscription-contributors.go` around lines 48 - 50, In
listSubscriptionContributorsCmdImpl, the signal.NotifyContext call registers
os.Kill which cannot be caught; replace os.Kill with syscall.SIGTERM (and add
the syscall import) so the context will be cancelled on a catchable termination
signal; update the signal.NotifyContext invocation accordingly and ensure
gracefulShutdown(stop) remains deferred.
Comment on lines
+72
to
+75
| for result := range pipeline.OrDone(ctx.Done(), roleAssignments) { | ||
| if roleAssignments, ok := result.(AzureWrapper).Data.(models.SubscriptionRoleAssignments); !ok { | ||
| log.Error(fmt.Errorf("failed type assertion"), "unable to continue enumerating subscription contributors", "result", result) | ||
| return |
There was a problem hiding this comment.
Guard the outer AzureWrapper assertion before reading Data.
Line 73 can panic on any non-AzureWrapper value because result.(AzureWrapper) is evaluated before the checked SubscriptionRoleAssignments assertion. That turns a bad item into a worker crash instead of a normal logged failure.
Suggested fix
for result := range pipeline.OrDone(ctx.Done(), roleAssignments) {
- if roleAssignments, ok := result.(AzureWrapper).Data.(models.SubscriptionRoleAssignments); !ok {
+ wrapper, ok := result.(AzureWrapper)
+ if !ok {
+ log.Error(fmt.Errorf("failed type assertion"), "unable to continue enumerating subscription contributors", "result", result)
+ return
+ }
+
+ if roleAssignments, ok := wrapper.Data.(models.SubscriptionRoleAssignments); !ok {
log.Error(fmt.Errorf("failed type assertion"), "unable to continue enumerating subscription contributors", "result", result)
return
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/list-subscription-contributors.go` around lines 72 - 75, The loop
currently does an inline chained assertion
result.(AzureWrapper).Data.(models.SubscriptionRoleAssignments) which will panic
if result is not an AzureWrapper; instead first assert and check the outer type:
cast result to AzureWrapper with an ok check (e.g., aw, ok :=
result.(AzureWrapper)); if not ok, log the type error and continue; then assert
aw.Data to models.SubscriptionRoleAssignments with its own ok check, log and
continue on failure; update the loop around pipeline.OrDone(ctx.Done(),
roleAssignments) to use these two guarded checks so non-AzureWrapper items never
cause a panic.
Comment on lines
+87
to
+90
| subscriptionContributor := models.SubscriptionContributor{ | ||
| Contributor: item.RoleAssignment, | ||
| SubscriptionId: item.SubscriptionId, | ||
| } |
There was a problem hiding this comment.
Use the batch's subscription ID when building each contributor.
This stream is already grouped by roleAssignments.SubscriptionId, but Line 89 copies item.SubscriptionId. In the current test fixture that field is empty, so emitted contributors lose their scope ID even though the parent payload has it.
Suggested fix
subscriptionContributor := models.SubscriptionContributor{
Contributor: item.RoleAssignment,
- SubscriptionId: item.SubscriptionId,
+ SubscriptionId: roleAssignments.SubscriptionId,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| subscriptionContributor := models.SubscriptionContributor{ | |
| Contributor: item.RoleAssignment, | |
| SubscriptionId: item.SubscriptionId, | |
| } | |
| subscriptionContributor := models.SubscriptionContributor{ | |
| Contributor: item.RoleAssignment, | |
| SubscriptionId: roleAssignments.SubscriptionId, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/list-subscription-contributors.go` around lines 87 - 90, The
SubscriptionContributor being constructed sets SubscriptionId from
item.SubscriptionId which is empty in fixtures; change it to use the batch-level
subscription ID instead (e.g., set SubscriptionId: batch.SubscriptionId) so each
SubscriptionContributor (models.SubscriptionContributor created in the loop that
assigns Contributor: item.RoleAssignment) inherits the stream/grouping
subscription scope.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Ticket
BED-5023
Background
AzureHound already collects
AZContributoredges forKeyVaultsandVirtualMachines, and collectsAZOwnerandAZUserAccessAdministratoredges for Management Groups, Resource Groups, and Subscriptions. However,AZContributorwas never collected for Management Groups, Resource Groups, or Subscriptions, meaning these edges were completely absent from the graph. This was reported as a bug (missing AZContributor edges), but was actually a missing feature across both the collection and ingestion layers.Changes
Kindenums (enums/kind.go):KindAZManagementGroupContributorKindAZResourceGroupContributorKindAZSubscriptionContributorManagementGroupContributorResourceGroupContributorSubscriptionContributorlist-management-group-contributors.golist-resource-group-contributors.golist-subscription-contributors.goContributorRoleIDand outputs the newKindSummary by CodeRabbit
New Features
Tests
Chores