Return errors for missing identity data in createOrUpdateDenyAssignment during installs, log and continue for admin updates#4769
Merged
kimorris27 merged 5 commits intomasterfrom Apr 16, 2026
Conversation
slawande2
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes createOrUpdateDenyAssignment to fail fast (return errors) when required identity data is missing, rather than logging and skipping deny assignment creation, and updates the unit tests accordingly.
Changes:
- Return errors when Platform Workload Identity ObjectID is empty, ServicePrincipalProfile is nil, or SPObjectID is empty.
- Update
TestCreateOrUpdateDenyAssignmenttable tests to assert expected error strings for the newly-failing cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cluster/denyassignment.go | Converts previously “log-and-skip” missing-identity branches into returned errors before ARM deployment. |
| pkg/cluster/denyassignment_test.go | Adds wantErr expectations and assertions for the new error-return behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
slawande2
requested review from
alcasim,
bennerv,
cadenmarchese,
hawkowl,
hlipsig,
jharrington22,
kevinobriendotca,
kimorris27,
mociarain,
mrWinston,
rogbas,
sankur-codes,
tiguelu,
tsatam,
tuxerrante,
ventifus,
wanghaoran1988 and
yjst2012
as code owners
Collaborator
Author
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kimorris27
requested changes
Apr 14, 2026
Contributor
kimorris27
left a comment
kimorris27
left a comment
There was a problem hiding this comment.
This change makes sense to ensure we catch these errors on installs, but I think we should include some additional changes to preserve the existing behavior during admin updates (log the error and keep going).
Collaborator
Author
|
@kimorris27 that makes sense. I've added a |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
slawande2
changed the title
kimorris27
approved these changes
Apr 16, 2026
shubhadapaithankar
approved these changes
Apr 16, 2026
tsatam
approved these changes
Apr 16, 2026
Member
tsatam
left a comment
tsatam
left a comment
There was a problem hiding this comment.
LGTM - we might want to make the specific error we return more comprehensive over all identities but this change as-is is fine to move forward with
tsatam
approved these changes
Apr 16, 2026
kimorris27
approved these changes
Apr 16, 2026
Contributor
|
MIWI E2E failed with an issue deleting the cluster, but the actual E2E tests passed; see below. I'll go ahead and merge this while I'm here. |
kimorris27
deleted the
slawande/return-error-with-missing-objectid-for-denyassignment
branch
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
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.
Which issue this PR addresses:
https://redhat.atlassian.net/browse/ARO-26127
Currently in createOrUpdateDenyAssignment(), when required identity data is missing (SPObjectID, ServicePrincipalProfile, or PlatformWorkloadIdentity ObjectID), the function logs the issue and returns nil, silently skipping deny assignment creation. This happens regardless of whether the call is from an install or an admin update.
During installs, missing identity data indicates a real problem — silently skipping deny assignment creation masks failures that should block the operation. We want to return errors in this case so the install fails explicitly.
During admin updates, the log-and-continue behavior is intentional. We want to preserve this behavior — log the issue and continue without returning an error. This PR adds logic so that createOrUpdateDenyAssignment distinguishes between these two calling contexts: returning errors during installs, and logging and continuing during admin updates.
What this PR does / why we need it:
This change updates three log messages to Errors when
createOrUpdateDenyAssignmentencounters missing identity data (empty platform workload identity ObjectID, nil ServicePrincipalProfile, or empty SPObjectID). These conditions indicate unexpected state that should not be silently logged at informational level and elevates them to errors.Test plan for issue:
updated unit tests
Is there any documentation that needs to be updated for this PR?
no