OCPBUGS-58181: Fix nil pointer dereference in ensureRolesAssignedToManagedIdentity#987
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. |
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.99% +0.11%
==========================================
Files 98 98
Lines 12558 12641 +83
==========================================
+ Hits 5888 5941 +53
- Misses 6015 6043 +28
- Partials 655 657 +2
🚀 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
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom, 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 |
|
/verified by e2e-azure-manual-oidc, @jstuever |
openshift-ci-robot
added
the
verified
|
@jstuever: This PR has been marked as verified by 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. |
|
/retest |
|
/retest |
|
/hold Revision b5f8b6a was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
|
The e2e-azure-manual-oidc test appears to be failing openshift-tests. I'm not sure this is directly related to this change. It works for me in manual testing. I'll retry a few more times hoping whatever is broken gets fixed so it can pass. |
|
/retest |
|
/skip e2e-azure-manual-oidc |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/skip ci/prow/e2e-azure-manual-oidc |
|
/skip ci/prow/e2e-azure-manual-oidc |
|
/override ci/prow/e2e-azure-manual-oidc |
|
@jstuever: Overrode contexts on behalf of jstuever: ci/prow/e2e-azure-manual-oidc 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. |
|
/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: Jira Issue OCPBUGS-58181: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-58181 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
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