azuread_app_role_assignment - fix assignments orphaned by the create retry - #1896
Open
charlesbalderson wants to merge 3 commits into
Open
Conversation
…eated by the create retry The create request carries a RetryFunc that replays the request when Graph reports the service principal or app role as not found, or answers "Not a valid reference update", both of which happen while a newly created object replicates. That request is a POST and is not idempotent. When an attempt has actually landed, the replay is answered with 400 Request_BadRequest "Permission being assigned already exists on the object", which the provider surfaced as a hard error before setting an ID. The assignment then exists in Entra but not in state, and because the assignment ID is server generated there is nothing for the practitioner to import without querying Graph by hand. Every subsequent apply plans the same create and fails the same way, so the configuration cannot be applied again. Track whether the retry fired, and on that error look the assignment up. If a retry happened the provider created it, so adopt it. If no retry happened the assignment pre-dates this request, so return the usual "already exists" diagnostic - now with a usable ID to import. Fixes hashicorp#763
…fore assigning it Assignments are commonly created in the same apply as the app role they reference, and a role that Graph has only just accepted is not yet visible on every replica. That is what puts the create request onto its retry path in the first place. Wait for the role to appear in the resource service principal's appRoles before sending the request. The wait is bounded and non-fatal: an app_role_id that is genuinely wrong should be reported by the API, not as a timeout here.
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.
Description
azuread_app_role_assignmentcan create an assignment in Entra and then fail, leaving itunmanaged and the configuration permanently unappliable.
The create request carries a
RetryFuncthat replays the request when Graph reports the serviceprincipal or app role as not found, or answers
Not a valid reference update. Both happen while anewly created object replicates, so the retry is doing useful work.
The problem is that the request is a
POSTand is not idempotent. When an attempt has actuallylanded, the replay is answered with:
That is returned before
d.SetId(), so the assignment exists in Entra but not in state. Becausethe assignment ID is server generated, there is nothing the practitioner can import without
querying Graph by hand. Every subsequent apply plans the same create and fails identically.
This reproduces reliably when the app role and the assignment are created in the same apply, which
is the common case for
azuread_application_app_roleplus afor_eachover a set of groups. We hitit building ephemeral environments: one assignment out of ~25 is orphaned, and that environment can
never be applied again.
Fixes #763
Changes
recover from duplicate assignments created by the create retry— the actual fix.RetryFuncnow records whether it fired. OnPermission being assigned already exists on the objectthe assignment is looked up on the resource service principal, and:tf.ImportAsExistsDiagis returned — nowwith a usable ID, which it previously was not possible to obtain
The lookup pages
appRoleAssignedToand matches locally, since Graph does not support$filteronappRoleIdorprincipalId. It only runs on the error path.Adopting is deliberately limited to the case the provider caused. A blanket adopt-on-conflict would
silently take over assignments made outside Terraform, which is not how this provider behaves
elsewhere.
wait for the app role to replicate before assigning it— prevention, separable.Waits for the app role to appear in the resource service principal's
appRolesbefore sending therequest, using the existing
consistencyhelper, in the spirit of #1844 and #1845. Bounded at oneminute and non-fatal on purpose: an
app_role_idthat is simply wrong should be reported by theAPI, not as a timeout here. Skipped for the default all-zeros role, which never appears in
appRoles.This is the second commit and can be dropped without affecting the fix.
add a requiresImport acceptance testTesting
The new
TestAccAppRoleAssignment_requiresImportcovers the branch where the assignment alreadyexisted and no retry occurred.
I could not construct a deterministic test for the adopt branch — it needs Graph to accept a write
and then report it as missing, which I have no way to force. It is reached by reasoning about the
retry path rather than by a repro, and I would rather flag that than imply coverage that is not
there. Happy to take a suggestion if there is an established way to fake the transport in this
repo.