feat(terraform): report operationStatuses/read for long running operation resources - #314
Open
Brian Gordon Davis (bgdnext64) wants to merge 4 commits into
Open
Conversation
- append RESOURCE_TYPE/operationStatuses/read for every discovered RESOURCE_TYPE/write - add --autoAddOperationStatusesReadPermission flag (default true) to the terraform command - drop actions Azure rejects as InvalidActionOrNotAction from the reported result - document the new flag and LRO polling behaviour Closes #62
Add a Terraform sample that provisions an Azure Container Registry, the only sample resource type that exposes a nested operationStatuses action, and an end to end test that enables the new option and asserts that Microsoft.ContainerRegistry/registries/operationStatuses/read is part of the discovered permissions. Also limit the invalid action filtering to the permissions MPF appends itself. Previously every action Azure rejected with InvalidActionOrNotAction was dropped from the result, which silently changed the output for deployments whose error messages reference an invalid action, for example Microsoft.Insights/components/currentbillingfeatures/delete.
…tting them MPF appends a RESOURCE_TYPE/operationStatuses/read candidate for every discovered write permission, but only a few resource providers actually expose that nested action. Azure rejects the rest with InvalidActionOrNotAction. Previously the rejected candidates stayed in the required permissions map, so every later iteration resubmitted them. Azure reports invalid actions one at a time and CreateUpdateCustomRole only retries five times, so a handful of rejected candidates consumed the whole retry budget on every call. The role then stopped being updated at all while the discovery loop kept iterating. Rejected candidates are now remembered and pruned as soon as Azure rejects them, and they are filtered out before new candidates are appended, so each one costs a single retry exactly once. Only permissions MPF appended itself are pruned; actions reported by the deployment errors are still returned unchanged. Also expands the single resource ACR terraform sample into a multi provider sample covering container registry, storage, network, subnet and log analytics, which is what surfaced the resubmission problem. The end to end test asserts the container registry operationStatuses permission is reported and that no other provider's candidate leaks into the result. Measured against a live subscription, the multi resource run went from 28+ iterations and 127 invalid action events to 9 iterations and 5 events, with every custom role update succeeding. Refs #62
Copilot started reviewing on behalf of
Brian Gordon Davis (bgdnext64)
July 27, 2026 20:51
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in Terraform behavior to automatically include RESOURCE_TYPE/operationStatuses/read alongside discovered RESOURCE_TYPE/write permissions to support azurerm resources created via long-running operations (LRO polling), while pruning candidates that Azure rejects as invalid actions.
Changes:
- Add domain helpers to derive and append
.../operationStatuses/readpermissions from.../write. - Add a case-insensitive permission removal helper and update the MPF service to track/prune rejected auto-added permissions across iterations.
- Expose a Terraform CLI flag, add a multi-provider Terraform sample, and introduce unit + e2e coverage for the new behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/usecase/mpfService.go |
Adds opt-in LRO permission auto-append, tracks auto-added vs rejected permissions, and prunes rejected candidates during iterations. |
pkg/usecase/mpfService_test.go |
Unit tests for invalid-action recording/pruning logic and the new service option. |
pkg/domain/appendOperationStatusesReadPermissions.go |
New helper to derive and append operationStatuses/read permissions from write actions. |
pkg/domain/appendOperationStatusesReadPermissions_test.go |
Unit tests for deriving/appending LRO polling permissions. |
pkg/domain/mpfResultFilterSort.go |
Adds case-insensitive FilterOutPermissions helper used for pruning rejected actions. |
pkg/domain/mpfResultFilterSort_test.go |
Unit tests for FilterOutPermissions, including non-mutation expectations. |
cmd/terraformCmd.go |
Adds --autoAddOperationStatusesReadPermission flag and wires it into MPF service options. |
cmd/terraformCmd_test.go |
Verifies the new Terraform flag is registered, defaults to true, and is settable. |
docs/commandline-flags-and-env-variables.md |
Documents the new Terraform flag and explains the LRO polling permission behavior. |
e2eTests/e2eTerraformOperationStatuses_test.go |
Adds an e2e test validating inclusion of valid operationStatuses/read and pruning of rejected candidates. |
samples/terraform/lro-multi-resource/main.tf |
New Terraform sample deploying multiple resources to exercise append-and-prune behavior. |
samples/terraform/lro-multi-resource/variables.tf |
Sample variable definition for location. |
samples/terraform/lro-multi-resource/dev.vars.tfvars |
Sample dev variables file. |
samples/terraform/lro-multi-resource/output.tf |
Sample outputs for deployed resources. |
Comment on lines
+252
to
+264
| // auto add the LRO polling permission for each discovered write permission | ||
| if s.autoAddOperationStatusesReadForWrite { | ||
| scpMp = domain.AppendOperationStatusesReadPermissions(scpMp) | ||
| // candidates Azure already rejected must not be added back | ||
| scpMp = domain.FilterOutPermissions(scpMp, s.rejectedAutoAddedList) | ||
| for _, permissions := range scpMp { | ||
| for _, permission := range permissions { | ||
| if strings.HasSuffix(strings.ToLower(permission), strings.ToLower(domain.OperationStatusesReadSuffix)) { | ||
| s.autoAddedPermissions[strings.ToLower(permission)] = true | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+34
to
+36
| if len(permissionsToRemove) == 0 { | ||
| return scpPerms | ||
| } |
Comment on lines
+73
to
+77
| // AppendOperationStatusesReadPermissions appends the LRO polling read permission for every | ||
| // resource type write permission found in the supplied scope/permission map. | ||
| // | ||
| // Permissions that Azure does not recognise are removed later on, when the custom role update | ||
| // reports them as InvalidActionOrNotAction. |
| @@ -0,0 +1,4 @@ | |||
| variable "location" { | |||
| description = "The supported azure location where the resource exists" | |||
Copilot started reviewing on behalf of
Brian Gordon Davis (bgdnext64)
July 27, 2026 21:06
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
pkg/usecase/mpfService.go:261
- When tracking auto-added operationStatuses permissions, the key is lowercased but not trimmed. Trimming here keeps keying consistent with recordInvalidActions/FilterOutPermissions and avoids missed matches if any permission strings contain whitespace.
if strings.HasSuffix(strings.ToLower(permission), strings.ToLower(domain.OperationStatusesReadSuffix)) {
s.autoAddedPermissions[strings.ToLower(permission)] = true
}
samples/terraform/lro-multi-resource/variables.tf:2
- The variable description reads like the resource already exists and uses lowercase “azure”. Aligning with other samples’ wording makes the sample clearer (this configuration deploys resources).
description = "The supported azure location where the resource exists"
Comment on lines
+47
to
+51
| if !strings.HasSuffix(permission, "/write") { | ||
| return "", false | ||
| } | ||
|
|
||
| resourceType := strings.TrimSuffix(permission, "/write") |
Comment on lines
+111
to
+117
| key := strings.ToLower(invalidAction) | ||
| if !s.autoAddedPermissions[key] || s.rejectedAutoAdded[key] { | ||
| continue | ||
| } | ||
| s.rejectedAutoAdded[key] = true | ||
| s.rejectedAutoAddedList = append(s.rejectedAutoAddedList, invalidAction) | ||
| newlyRejected = append(newlyRejected, invalidAction) |
Comment on lines
+1
to
+5
| terraform {} | ||
|
|
||
| provider "azurerm" { | ||
| features {} | ||
| } |
Brian Gordon Davis (bgdnext64)
requested review from
Mani Bindra (maniSbindra)
and
a lite review from Copilot
August 4, 2026 16:39
Copilot started reviewing on behalf of
Brian Gordon Davis (bgdnext64)
August 4, 2026 16:41
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/usecase/mpfService.go:263
- The
autoAddedPermissionstracking marks any permission ending in/operationStatuses/readas "auto-added". HoweverGetScopePermissionsFromAuthErrorcan already return an/operationStatuses/readaction directly from deployment errors (e.g. LinkedAuthorizationFailed), so this can cause deployment-reported permissions to be treated as auto-added and later dropped byrecordInvalidActions, contradicting the comment that deployment-reported actions are left untouched. Track only the permissions that were actually appended by this feature (i.e., candidates not already present before appending).
// auto add the LRO polling permission for each discovered write permission
if s.autoAddOperationStatusesReadForWrite {
scpMp = domain.AppendOperationStatusesReadPermissions(scpMp)
// candidates Azure already rejected must not be added back
scpMp = domain.FilterOutPermissions(scpMp, s.rejectedAutoAddedList)
for _, permissions := range scpMp {
for _, permission := range permissions {
if strings.HasSuffix(strings.ToLower(permission), strings.ToLower(domain.OperationStatusesReadSuffix)) {
s.autoAddedPermissions[strings.ToLower(permission)] = true
}
}
}
samples/terraform/lro-multi-resource/variables.tf:2
- The variable description says the location is where "the resource exists", but this sample creates resources. Updating the wording avoids confusion for users of the sample.
description = "The supported azure location where the resource exists"
pkg/domain/appendOperationStatusesReadPermissions_test.go:2
- This new test file is missing the standard MIT license header that appears at the top of other Go files in this repository (including other
pkg/domain/*_test.gofiles). Add the header for consistency and license compliance.
package domain
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.
Closes #62
What this does
Adds an opt-in behaviour where MPF reports
RESOURCE_TYPE/operationStatuses/readalongside a discoveredRESOURCE_TYPE/writepermission. Resources created through long running operations return201 Createdwith anAzure-AsyncOperationheader, and the azurerm provider then polls that URL (CreateThenPoll,CreateOrUpdateThenPoll). Without the read permission on the operation status the poll fails even though the create itself succeeded.Changes
pkg/domain/appendOperationStatusesReadPermissions.goderives the candidate permission from a write permission. It skips wildcards, requires a/writesuffix, and will not double-append to a permission that already targetsoperationStatuses.pkg/domain/mpfResultFilterSort.gogainsFilterOutPermissions, a case-insensitive removal helper that returns a new map.pkg/usecase/mpfService.goappends candidates each iteration, tracks which permissions MPF added itself, and drops the ones Azure rejects.cmd/terraformCmd.goexposes--autoAddOperationStatusesReadPermission, documented indocs/commandline-flags-and-env-variables.md.samples/terraform/lro-multi-resource/is a new sample spanning container registry, storage, virtual network, subnet and log analytics.Things worth a reviewer's attention
The permission is only useful for a small number of providers. I checked with
az provider operation showandMicrosoft.ContainerRegistryis the only provider I found that exposes the nestedRESOURCE_TYPE/operationStatusesaction. ContainerInstance, ContainerService, KeyVault, DocumentDB, Compute, Web and Cache do not. OperationalInsights, Network, Storage and App only expose location scoped or specialised variants (locations/operationstatuses,dnsoperationstatuses,locations/*RPOperationStatuses).So in practice MPF appends a candidate for every write permission, Azure rejects nearly all of them, and they are discarded. The feature works, but the mechanism is speculative by design and worth an explicit decision from a maintainer. If you would prefer a curated allowlist of providers over propose-and-discard, that is a reasonable alternative and I am happy to rework it.
Default differs by layer. The CLI flag defaults to
true, matching the unconditional wording in #62. The service level option defaults tofalseso existing programmatic callers are unaffected.Related pre-existing bug: #313. Testing this surfaced a separate problem in
CreateUpdateCustomRole, which retries 5 times and then returnsnileven if every attempt failed. Azure reports invalid actions one at a time, so a handful of rejected candidates exhausts the budget and the role silently stops being updated. This PR avoids triggering it by never resubmitting a rejected candidate, but the underlying trap is untouched and filed separately as #313.Validation
Unit tests,
go build,go vetandgofmtall clean.End to end against a live subscription using a service principal with no role assignments:
TestTerraformMultiResourceWithOperationStatusesReadPermissions— PASS (592s). Result containedMicrosoft.ContainerRegistry/registries/operationStatuses/readand no other provider's candidate, with all four non-registry write permissions intact.TestTerraformWithImport— PASS (405s). This is the regression check for the pruning logic; an earlier iteration of this branch incorrectly filtered actions that came from deployment errors, and this test caught it.The prune fix is measurable on the multi provider sample:
InvalidActionOrNotActioneventsThe full 21 test end to end suite was green earlier on this branch. I did not re-run all of it after the final commit, since the pruning path is inert when the option is off, but I am happy to if you would like that confirmed.