OCPBUGS-58181: Fix nil pointer dereference in ensureRolesAssignedToManagedIdentity#987
OCPBUGS-58181: Fix nil pointer dereference in ensureRolesAssignedToManagedIdentity#987jstuever wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/severity-low
|
@jstuever: This pull request references Jira Issue OCPBUGS-58181, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds defensive nil checks when iterating and cleaning up Azure role assignments for managed identities; incomplete Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jstuever: This pull request references Jira Issue OCPBUGS-58181, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira backport release-4.21,release-4.22 |
|
@jstuever: Missing required branches for backport chain:
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jstuever: This pull request references Jira Issue OCPBUGS-58181, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstuever The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/provisioning/azure/create_managed_identities.go`:
- Around line 313-316: The cleanup loop may still nil-deref
existingRoleAssignment.Name; move the guard that checks
existingRoleAssignment.Properties, RoleDefinitionID, and Scope to execute before
any dereference of existingRoleAssignment.Name and extend it to also check
existingRoleAssignment.Name != nil; specifically, in the loop that iterates
existingRoleAssignment (the cleanup loop logic where the code compares
*existingRoleAssignment.Name and later logs it), add Name to the nil checks and
run that guard before the name comparison and before logging to avoid panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5879ed38-c555-4581-8eb1-97770b5a6bfb
📒 Files selected for processing (1)
pkg/cmd/provisioning/azure/create_managed_identities.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #987 +/- ##
==========================================
+ Coverage 46.88% 46.91% +0.02%
==========================================
Files 98 98
Lines 12558 12564 +6
==========================================
+ Hits 5888 5894 +6
Misses 6015 6015
Partials 655 655
🚀 New features to boost your workflow:
|
|
/test e2e-azure-manual-oidc |
|
@jstuever e2e-azure-manual-oidc job is blocked by https://issues.redhat.com/browse/OCPBUGS-77845 |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
|
@jstuever: This pull request references Jira Issue OCPBUGS-58181, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/provisioning/azure/create_managed_identities_test.go`:
- Around line 933-1063: Add a sibling table test mirroring the existing
nil-Properties/nil-RoleDefinitionID/nil-Name cases that verifies the cleanup
loop skips assignments with a nil Scope: create a case named like "Extra role
assignment with nil Scope in cleanup loop is skipped without panic" and in its
mockAzureClientWrapper call to mockRoleAssignmentsListForScopePager include a
second armauthorization.RoleAssignment whose Properties exists but has
Properties.Scope == nil (and PrincipalID/RoleDefinitionID set as needed), then
keep the mockRoleDefinitionsListPager entry identical to the others; this will
exercise the cleanup guard in create_managed_identities.go (the nil Scope branch
around lines 306-327) and ensure no panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fb213fa-83ac-406c-a026-5e1303cd867a
📒 Files selected for processing (2)
pkg/cmd/provisioning/azure/create_managed_identities.gopkg/cmd/provisioning/azure/create_managed_identities_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cmd/provisioning/azure/create_managed_identities.go
Add defensive nil checks when iterating over existing role assignments to prevent panics when Azure's eventual consistency results in incomplete data structures with nil Properties, RoleDefinitionID, Scope, or Name fields. Co-Authored-By: Claude Sonnet 4.6
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cmd/provisioning/azure/create_managed_identities.go (1)
271-274:⚠️ Potential issue | 🔴 CriticalRemaining nil-deref paths in role-assignment matching/cleanup
The new guards are incomplete: Line 313 still dereferences
*shouldExistRoleAssignment.Namewithout a nil check, and both loops assume assignment pointers are non-nil. This can still panic on malformed Azure responses.Suggested hardening patch
for _, roleAssignment := range existingRoleAssignments { - if roleAssignment.Properties == nil || roleAssignment.Properties.RoleDefinitionID == nil || roleAssignment.Properties.Scope == nil { + if roleAssignment == nil || roleAssignment.Name == nil || + roleAssignment.Properties == nil || roleAssignment.Properties.RoleDefinitionID == nil || roleAssignment.Properties.Scope == nil { log.Printf("skipping incomplete role assignment (nil Properties, RoleDefinitionID, or Scope)") continue } @@ for _, existingRoleAssignment := range existingRoleAssignments { - if existingRoleAssignment.Name == nil || existingRoleAssignment.Properties == nil || existingRoleAssignment.Properties.RoleDefinitionID == nil || existingRoleAssignment.Properties.Scope == nil { + if existingRoleAssignment == nil || existingRoleAssignment.Name == nil || + existingRoleAssignment.Properties == nil || existingRoleAssignment.Properties.RoleDefinitionID == nil || existingRoleAssignment.Properties.Scope == nil { log.Printf("skipping incomplete existing role assignment during cleanup (nil Name, Properties, RoleDefinitionID, or Scope)") continue } found := false for _, shouldExistRoleAssignment := range shouldExistRoleAssignments { - if shouldExistRoleAssignment != nil && *shouldExistRoleAssignment.Name == *existingRoleAssignment.Name { + if shouldExistRoleAssignment != nil && shouldExistRoleAssignment.Name != nil && + *shouldExistRoleAssignment.Name == *existingRoleAssignment.Name { found = true + break } }Also applies to: 307-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/azure/create_managed_identities.go` around lines 271 - 274, The code still dereferences pointers without full nil checks causing potential panics; update the loops that iterate role assignments to first ensure each roleAssignment and each shouldExistRoleAssignment is non-nil before accessing fields, and add a nil check on shouldExistRoleAssignment.Name (and its pointer) before dereferencing *shouldExistRoleAssignment.Name in the matching/cleanup logic (the loop that compares assignments and the block that uses shouldExistRoleAssignment.Name); change conditions to skip entries where roleAssignment == nil, shouldExistRoleAssignment == nil, shouldExistRoleAssignment.Name == nil, or roleAssignment.Properties/RoleDefinitionID/Scope are nil so no pointer is dereferenced without validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cmd/provisioning/azure/create_managed_identities.go`:
- Around line 271-274: The code still dereferences pointers without full nil
checks causing potential panics; update the loops that iterate role assignments
to first ensure each roleAssignment and each shouldExistRoleAssignment is
non-nil before accessing fields, and add a nil check on
shouldExistRoleAssignment.Name (and its pointer) before dereferencing
*shouldExistRoleAssignment.Name in the matching/cleanup logic (the loop that
compares assignments and the block that uses shouldExistRoleAssignment.Name);
change conditions to skip entries where roleAssignment == nil,
shouldExistRoleAssignment == nil, shouldExistRoleAssignment.Name == nil, or
roleAssignment.Properties/RoleDefinitionID/Scope are nil so no pointer is
dereferenced without validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0383d1dc-1906-4b8d-92a6-8a05a98feba2
📒 Files selected for processing (2)
pkg/cmd/provisioning/azure/create_managed_identities.gopkg/cmd/provisioning/azure/create_managed_identities_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/provisioning/azure/create_managed_identities_test.go
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-azure-manual-oidc |
|
/override ci/prow/security |
|
@jstuever: Overrode contexts on behalf of jstuever: ci/prow/security DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@jstuever: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/assign @dlom |
4 participants
Add nil checks for Properties, RoleDefinitionID, and Scope fields before dereferencing role assignments. This prevents panics during retry scenarios when Azure API returns role assignments with nil properties.
Assisted-by: Claude Sonnet 4.6
Summary by CodeRabbit
Bug Fixes
Tests