-
Notifications
You must be signed in to change notification settings - Fork 154
CCO-687: Fix nil pointer dereference in Azure role assignment handling #879
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
base: master
Are you sure you want to change the base?
Conversation
This commit addresses CCO-687 by adding comprehensive nil checks throughout the Azure managed identity creation process, particularly in the ensureRolesAssignedToManagedIdentity function. Changes include: - Add nil checks for role assignment properties before dereferencing - Add nil checks for managed identity properties (PrincipalID, ClientID, TenantID) - Add error handling for writeCredReqSecret function calls - Fix incorrect field access patterns (remove redundant struct nesting) - Ensure all pointer dereferences are protected with nil checks This prevents panics when Azure returns role assignments with nil properties during retry scenarios after eventual consistency errors. Fixes: https://issues.redhat.com/browse/CCO-687
@kaovilai: This pull request references CCO-687 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. In 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 NOT APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #879 +/- ##
==========================================
- Coverage 46.97% 46.89% -0.08%
==========================================
Files 97 97
Lines 11910 11932 +22
==========================================
+ Hits 5595 5596 +1
- Misses 5697 5709 +12
- Partials 618 627 +9
🚀 New features to boost your workflow:
|
/retest |
1 similar comment
/retest |
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Summary
This PR fixes a panic with nil pointer dereference in the
ensureRolesAssignedToManagedIdentity
function when processing existing role assignments during Azure STS provisioning retry scenarios.Issue
https://issues.redhat.com/browse/CCO-687
Problem
The ccoctl azure create-all command encounters a panic when iterating through existing role assignments. The code attempts to dereference
existingRoleAssignment.Properties.RoleDefinitionID
without checking if Properties or RoleDefinitionID is nil. This occurs especially during retry scenarios after Azure eventual consistency errors, when role assignment objects may have nil Properties or nil fields.Solution
Added comprehensive nil checks throughout the Azure managed identity creation process:
Test plan
🤖 Generated with Claude Code