feat: add ClusterDenyAssignment controller for Azure deny assignments - #6269
feat: add ClusterDenyAssignment controller for Azure deny assignments#6269David Eads (deads2k) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new backend reconciliation loop to enforce least-privilege by creating Azure Microsoft.Authorization/denyAssignments on the managed resource group during cluster creation and removing them during cluster deletion, with progress/state tracked on ServiceProviderCluster.Status.
Changes:
- Introduces
ClusterDenyAssignmentcontroller and deny-assignment definition/permissions catalog to create/delete deterministic deny assignments. - Extends
ServiceProviderClusterStatusto persistPendingDenyAssignmentsandDenyAssignments, plus helper functions to build deny-assignment resource IDs. - Adds lifecycle gates: cluster-service create waits for deny assignments, and ServiceProviderCluster deletion is blocked until deny assignments are removed; wires new Azure clients and controller into backend startup.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/zz_generated.deepcopy.go | Adds deepcopy support for the new deny-assignment reference type and new ServiceProviderClusterStatus fields. |
| internal/api/types_serviceprovider_cluster.go | Adds PendingDenyAssignments/DenyAssignments to SPC status and defines DenyAssignmentReference. |
| internal/api/types_cosmosdata.go | Adds helpers to construct/parse deny assignment ARM resource IDs. |
| backend/pkg/controllers/denyassignments/deny_assignment_permissions.go | Defines the action/notAction/dataAction catalogs used in deny assignment permissions. |
| backend/pkg/controllers/denyassignments/deny_assignment_definitions.go | Defines deny assignment “types” and which operator identities they exclude, plus deterministic ID generation inputs. |
| backend/pkg/controllers/denyassignments/deny_assignment_controller.go | Implements the create/delete reconciliation logic and persistence of pending/created deny assignments. |
| backend/pkg/controllers/clusterdeletion/cluster_child_resources_cleanup_controller.go | Blocks ServiceProviderCluster deletion while deny assignments are still tracked as pending/created. |
| backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go | Adds a precondition gate requiring deny assignments to be created before cluster-service create dispatch. |
| backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go | Updates existing tests to satisfy the new deny-assignment precondition gate. |
| backend/pkg/azure/client/generic_resources_client.go | Adds a minimal interface wrapper for armresources.Client generic CRUD by ID (used for deny assignments). |
| backend/pkg/azure/client/fpa_client_builder.go | Extends FPA client builder to construct generic resources + deny assignments clients. |
| backend/pkg/azure/client/deny_assignments_client.go | Adds a minimal interface wrapper for armauthorization.DenyAssignmentsClient (used for GET). |
| backend/pkg/app/backend.go | Registers and runs the new deny assignment controller under backend leader election. |
Files not reviewed (1)
- internal/api/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (2)
backend/pkg/controllers/denyassignments/deny_assignment_controller.go:304
- ensureDenyAssignment dereferences resourceID and scope without nil checks (resourceID.Name / scope.String()). If a stored ServiceProviderCluster contains a DenyAssignmentReference with a nil DenyAssignmentResourceID, this will panic the controller.
Add explicit nil checks and return a tracked error instead of panicking.
if notActions == nil {
notActions = []string{}
}
if dataActions == nil {
dataActions = []string{}
}
excludedPrincipalIDs, err := resolvePrincipalIDs(cluster, excludedIdentityResourceIDs)
if err != nil {
return utils.TrackError(fmt.Errorf("failed to resolve principal IDs: %w", err))
}
existing, err := denyAssignmentsClient.Get(ctx, scope.String(), resourceID.Name, nil)
backend/pkg/controllers/denyassignments/deny_assignment_controller.go:439
- deleteDenyAssignment calls resourceID.String() without checking for nil. A nil DenyAssignmentResourceID in persisted state will panic during cluster deletion cleanup.
func (c *clusterDenyAssignmentSyncer) deleteDenyAssignment(
ctx context.Context,
client azureclient.GenericResourcesClient,
resourceID *azcorearm.ResourceID,
) error {
poller, err := client.BeginDeleteByID(ctx, resourceID.String(), denyAssignmentAzureAPIVersion, nil)
if isResourceNotFoundError(err) {
| definition, ok := defByType[denyAssignmentReference.DenyAssignmentType] | ||
| if !ok { | ||
| logger.Error(nil, "Skipping unknown deny assignment type", "denyAssignmentType", denyAssignmentReference.DenyAssignmentType) | ||
| continue | ||
| } |
| ready, err = c.createPreconditionDenyAssignmentsCreated(ctx, existingServiceProviderCluster) | ||
| if err != nil { | ||
| return utils.TrackError(err) | ||
| } | ||
| if !ready { | ||
| return nil | ||
| } |
| pendingTypes = append(pendingTypes, denyAssignmentReference.DenyAssignmentType) | ||
| } | ||
| logger.Info("Deny assignments not yet created, waiting for ClusterDenyAssignment controller", | ||
| "pendingDenyAssignmentTypes", pendingTypes) |
There was a problem hiding this comment.
How useful is it to log the types we're waiting on? Could we also just give a number?
| dbCluster: newTestCluster(), | ||
| existingServiceProviderCluster: newTestSPC(func(spc *api.ServiceProviderCluster) { | ||
| spc.Spec.ControlPlaneVersion.DesiredVersion = desiredVersion | ||
| spc.Status.DenyAssignments = []api.DenyAssignmentReference{{DenyAssignmentType: "resources-deny-assignment", DenyAssignmentResourceID: api.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testManagedResourceGroup/providers/Microsoft.Authorization/denyAssignments/00000000-0000-0000-0000-000000000001"))}} |
There was a problem hiding this comment.
I suggest adding a new test where desiredVersion is set but deny assignments are still pending
| "github.com/Azure/ARO-HCP/internal/api" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
we're moving to a common single definition now that we have support for more excluded principals (Massimo can confirm if it's rolled out completely or not).
We should update this too only leverage a single deny assignment instead of multiple and hold the PR on massimo's confirmation it's rolled out to all regions we're deployed in.
PR should just lay down a single deny assignment which looks like: https://github.com/openshift-online/aro-hcp-clusters-service/blob/master/pkg/azure/denyassignmentcreator/deny_assignment_creator.go#L245-L303. We're missing a few permissions from there, so we'll see the following additional permissions shortly:
"Microsoft.Network/dnszones/CAA/write",
"Microsoft.Network/dnszones/TXT/write",
"Microsoft.Network/dnszones/TXT/delete",
"Microsoft.Compute/virtualMachines/retrieveBootDiagnosticsData/action",
and the removal of Microsoft.Authorization/roleAssignments/read since read permissions are already excluded.
There was a problem hiding this comment.
If instead we want 1:1 parity between CS and what's here, we need to introduce a deny assignment similar to what is mentioned above ^ since that one doesn't appear in this PR.
Today CS lays down all the ones mentioned + the one above if the excluded principal limit is large enough.
There was a problem hiding this comment.
the one above if the excluded principal limit is large enough.
+1; we should create the complete one as well to align with CS
There was a problem hiding this comment.
we're moving to a common single definition now that we have support for more excluded principals (Massimo can confirm if it's rolled out completely or not).
Just checked. The ExcludedPrincipal Limit has been increased across all regions (see this kusto query )
| return utils.TrackError(fmt.Errorf("BeginDeleteByID failed: %w", err)) | ||
| } | ||
|
|
||
| _, err = poller.PollUntilDone(ctx, nil) |
There was a problem hiding this comment.
Do we want to block here or somehow signal "reenqueue and retry after a while" ?
There was a problem hiding this comment.
This exceeds my azure knowledge. Are these fast or so slow?
There was a problem hiding this comment.
It depends on the resource you are dealing with.
I haven't played much with DenyAssignments so I don't know how fast/slow they become ready (Massimo can have the answer to that if needed), but some resources can take a considerable amount of time to get in the desired state. Think for example loadbalancers, virtual machines, ... . And we will have some of those.
| return utils.TrackError(fmt.Errorf("BeginCreateOrUpdateByID failed: %w", err)) | ||
| } | ||
|
|
||
| _, err = poller.PollUntilDone(ctx, nil) |
There was a problem hiding this comment.
as a note, if this ends up in a failed terminal state we would be erroring and retrying indefinitely
There was a problem hiding this comment.
as a note, if this ends up in a failed terminal state we would be erroring and retrying indefinitely
I bet it's an azure client gap on my part again. What is supposed to happen?
There was a problem hiding this comment.
When you create a resource in azure and it ends up in a terminal state it never moves from that state unless something modifies it. Think for example creating a VM. The VM can end up being in terminal state "ready" or in terminal state "failed". Once it's failed it will remain in failed state as it's a terminal state. Unless you for example delete it (which will change its state) or do some update on it that makes it change state, however I am not sure that all resources support recovering from "failed" 🤔
| serviceProviderCluster.Status.DenyAssignments = append(serviceProviderCluster.Status.DenyAssignments, denyAssignmentReference) | ||
| serviceProviderCluster.Status.PendingDenyAssignments = removeDenyAssignmentRef(serviceProviderCluster.Status.PendingDenyAssignments, denyAssignmentReference.DenyAssignmentType) | ||
| } | ||
| _, err = serviceProviderClusterCRUD.Replace(ctx, serviceProviderCluster, nil) |
There was a problem hiding this comment.
an alternative is to deepcopy and do semantic deepequal like in other controllers, to only replace when they differ
| return utils.TrackError(err) | ||
| } | ||
|
|
||
| if cluster.ServiceProviderProperties.DeletionTimestamp != nil { |
There was a problem hiding this comment.
There is no need to issue an arm request to delete a deny assingement when the cluster is being deleted.
That's because the MRG is going to be deleted and the corresponding deny assignment will be deleted in cascade.
| return len(serviceProviderCluster.Status.DenyAssignments) > 0 || len(serviceProviderCluster.Status.PendingDenyAssignments) > 0 | ||
| } | ||
|
|
||
| func (c *clusterDenyAssignmentSyncer) creationNeedsWork(cluster *api.HCPOpenShiftCluster, serviceProviderCluster *api.ServiceProviderCluster) bool { |
There was a problem hiding this comment.
There is no point in doing the work if:
- The MRG doesn't exist
- or the principals ids collected here https://github.com/deads2k/ARO-HCP/blob/4ddb416b6d93b38a61890b264e90492d3db75551/backend/pkg/controllers/denyassignments/deny_assignment_controller.go#L467 are not there
Let's add at least the second part in the creationNeedsWork check.
There was a problem hiding this comment.
- or the principals ids collected here https://github.com/deads2k/ARO-HCP/blob/4ddb416b6d93b38a61890b264e90492d3db75551/backend/pkg/controllers/denyassignments/deny_assignment_controller.go#L467 are not there
Let's add at least the second part in the
creationNeedsWorkcheck.
It's a little weird to fanout and I bet it ages poorly as the list of definitions changes. You sure or shall we not add to pending when there isn't an ID for it?
There was a problem hiding this comment.
During early cluster creation phases, it is expected for the principalID to not be there up until when they are reconciled;
If we don't gate the controller for their existence, when collecting them we'll return an error https://github.com/deads2k/ARO-HCP/blob/4ddb416b6d93b38a61890b264e90492d3db75551/backend/pkg/controllers/denyassignments/deny_assignment_controller.go#L467 causing undesired retries due to https://github.com/deads2k/ARO-HCP/blob/4ddb416b6d93b38a61890b264e90492d3db75551/backend/pkg/controllers/denyassignments/deny_assignment_controller.go#L189
or shall we not add to pending when there isn't an ID for it?
Is the idea that we sync but only add the deny assignment to the pending list and not return an error? If so, that can work too.
There was a problem hiding this comment.
Ok, you've phrased it as, "don't have principleIDs for deny assignment", but do you actually mean:
cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators and ``cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperatorsandcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity` have values?
There was a problem hiding this comment.
Sorry for the confusion in the phrasing.
Concretely, I meant that we should only trigger the creation of the deny assignment when we know that the principal IDs of the operator + smi have been retrieved so that we are able to resolve the principalIDs of each operator identity (data plane and control plane) + the service manage identity in this piece of code https://github.com/deads2k/ARO-HCP/blob/41353414c6de01f1704ed83529dee8bb40c5caec/backend/pkg/controllers/denyassignments/deny_assignment_controller.go#L549
This line https://github.com/deads2k/ARO-HCP/blob/41353414c6de01f1704ed83529dee8bb40c5caec/backend/pkg/controllers/denyassignments/deny_assignment_controller.go#L565 will return an error each all the time even early on during provisioning phases where the principalIDs have not been retrieved from Azure yet.
In practice it is okay, since eventually they'll be reconciled, but the error is likely to trigger the BackendControllerRetryHotLoop when a cluster is being created in Stage/Prod envs where we can create deny assignments
| return len(serviceProviderCluster.Status.DenyAssignments) > 0 || len(serviceProviderCluster.Status.PendingDenyAssignments) > 0 | ||
| } | ||
|
|
||
| func (c *clusterDenyAssignmentSyncer) creationNeedsWork(cluster *api.HCPOpenShiftCluster, serviceProviderCluster *api.ServiceProviderCluster) bool { |
There was a problem hiding this comment.
The controller can only be run in environment where deny assignment can be created i.e stage & prod. Lower envs, this controller needs to be disabled
There was a problem hiding this comment.
I give detail about that here: #6269 (comment)
| go dispatchRequestCredentialController.Run(ctx, 20) | ||
| go dispatchRevokeCredentialsController.Run(ctx, 20) | ||
| go clusterClusterServiceCreateController.Run(ctx, 20) | ||
| go clusterDenyAssignmentController.Run(ctx, 20) |
| @@ -0,0 +1,529 @@ | |||
| // Copyright 2026 Microsoft Corporation | |||
There was a problem hiding this comment.
My understanding is that deny assignments can only be created in environments where a real FPA exists. On environments where it doesn't exist (like dev, integration) they can't be created because only a real FPA can create them. This would impact the execution of this controller (as well as preconditions . What signals to backend whether a real FPA is being used or not is the CLI flag --insecure-ignore-user-azure-managed-identities-that-need-managed-identities-dataplane-available-and-use-mock (backend/cmd/root.go).
I don't recall the details on whether attempting to create a denyassignment fails with an error, fails silently or simply creates the resource with no effect. If we are interested on this detail we should find it out.
| ) | ||
| } | ||
|
|
||
| func (c *clusterDenyAssignmentSyncer) deletionNeedsWork(serviceProviderCluster *api.ServiceProviderCluster) bool { |
There was a problem hiding this comment.
Here I think we should not proceed with deletion if CSID is still set:
if cluster.ServiceProviderProperties.ClusterServiceID != nil && len(cluster.ServiceProviderProperties.ClusterServiceID.String()) > 0 {
return false
}
CS has an ordered and coordinated delete process. For example, we don't delete the managed resource group until the deny assignments within it have been deleted. Or another example being we don't delete the managed resource group until the cluster has been fully deleted from the management cluster side, because there are resources within the managed resourcegroup that were created as part of the creation of the cluster in the management cluster side
4ddb416 to
3768270
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 47 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
internal/api/types_serviceprovider_cluster.go:266
- New Cosmos-persisted fields in ServiceProviderClusterStatus should include per-leaf "Written by:" annotations (per the repo's Cosmos writer-annotation convention). DenyAssignmentReferences currently has none, which makes it hard to audit which controller mutates these fields and will drift from docs/cosmos-data-flow.md.
internal/api/types_serviceprovider_cluster.go:277 - DenyAssignmentReference is stored in Cosmos but its leaf fields are missing "Written by:" annotations, which the rest of this file uses to keep Cosmos writer ownership clear.
config/config.yaml:177 - This PR includes multiple image digest bumps and rendered config updates (e.g. fluent-bit/mdsd/hypershift/velero) that are not described in the PR summary and are unrelated to adding the ClusterDenyAssignment controller. CONTRIBUTING.md requires keeping PRs focused (one task per PR); consider splitting these bumps into a separate PR so review/rollback is safer.
arobit:
forwarder:
image:
registry: mcr.microsoft.com
repository: oss/v2/fluent/fluent-bit
digest: sha256:c81f949228a0d446aea4966d8da39cfcd63f17c69a5eae143ce8264ddddc5777 # v5.0.4 (2026-07-28 19:40)
resources:
requests:
cpu: 100m
memory: 256Mi
limits:
memory: 1248Mi
kusto:
enabled: false
buffering: true
environmentName: "{{ .ctx.environment }}"
mdsd:
enabled: true
image:
registry: mcr.microsoft.com
repository: geneva/distroless/mdsd
digest: sha256:5c0159feac3312c447b10c840558a5db6591f78c498cf9150e010be9fa860a20 # 1.43.0-20260728-3 (2026-07-28 17:38)
| // Deny assignments are scoped to the managed resource group and are | ||
| // cleaned up automatically when the managed resource group is deleted. | ||
| if cluster.ServiceProviderProperties.DeletionTimestamp != nil { | ||
| return nil | ||
| } | ||
|
|
||
| return c.syncDenyAssignmentUpsert(ctx, key, cluster) |
| if serviceProviderCluster.Status.AzureResources.ManagedResourceGroup.AzureResource == nil { | ||
| return false | ||
| } |
| managedResourceGroupID := serviceProviderCluster.Status.AzureResources.ManagedResourceGroup.AzureResource | ||
|
|
| } | ||
|
|
||
| lookup := make(map[string]string, len(cluster.Identity.UserAssignedIdentities)) | ||
| for resourceID, identity := range cluster.Identity.UserAssignedIdentities { |
There was a problem hiding this comment.
The cluster.Identity.UserAssignedIdentities field only contain the control plane identities + the service managed identities; the data plane identities will be missing;
We need a way to track their principal ids somewhere in the RP so that we can use them;
or alternatively, retrieve them via a call to ARM using the credentials of the ServiceManagedIdentity (that's how CS does it https://github.com/openshift-online/aro-hcp-clusters-service/blob/master/pkg/clusterprovisioner/acm/aro/fetch_and_persist_data_plane_managed_identities_client_ids_and_principal_ids_step.go)
| @@ -0,0 +1,118 @@ | |||
| // Copyright 2026 Microsoft Corporation | |||
There was a problem hiding this comment.
We can use mockgen like it's done for other azure clients
There was a problem hiding this comment.
We can use mockgen like it's done for other azure clients
came out with ugly test results for pagers.
4135341 to
a2bdaad
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- internal/api/zz_generated.deepcopy.go: Generated file
Suppressed comments (6)
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:147
syncDenyAssignmentNeedsWorkrequiresserviceProviderCluster.Status.AzureResources.ManagedResourceGroup.AzureResourceto be set, but nothing in the repo populates that field (the only references are in this controller/tests). As a result, this controller will never create deny assignments in real runs, andClusterClusterServiceCreatewill wait forever on the new deny-assignment precondition.
if !c.syncDenyAssignmentNeedsWork(cluster, serviceProviderCluster) {
return nil
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:97
- The controller skips all work when
cluster.ServiceProviderProperties.DeletionTimestamp != nil, but this PR also adds a deletion gate that blocksServiceProviderClusterdeletion until deny assignments are gone. With the current early-return, the deny-assignment references will never be cleared and cluster deletion can stall indefinitely (also contradicts the PR description that deny assignments are deleted during cluster deletion).
// Deny assignments are scoped to the managed resource group and are
// cleaned up automatically when the managed resource group is deleted.
if cluster.ServiceProviderProperties.DeletionTimestamp != nil {
return nil
}
internal/api/types_serviceprovider_cluster.go:256
DenyAssignmentReferencesis stored in Cosmos but its leaf fields are missing the required "Written by:" annotations (consistent provenance is important for status/debuggability).
type DenyAssignmentReferences struct {
// PendingAzureResources contains resource IDs that have been requested but
// not yet confirmed to exist in Azure.
PendingAzureResources []DenyAssignmentReference `json:"pendingDenyAssignments,omitempty"`
// AzureResources contains resource IDs that have been confirmed to exist in Azure.
internal/api/types_serviceprovider_cluster.go:276
DenyAssignmentReferenceis stored in Cosmos but its leaf fields are missing the required "Written by:" annotations used elsewhere in this file.
// DenyAssignmentType identifies the category of deny assignment (e.g. "resources-deny-assignment").
// Used as a suffix when generating the deterministic deny assignment UUID.
DenyAssignmentType string `json:"denyAssignmentType"`
// DenyAssignmentResourceID is the full Azure resource ID of the deny assignment,
// e.g. "/subscriptions/{sub}/resourceGroups/{rg}/providers/Microsoft.Authorization/denyAssignments/{uuid}".
DenyAssignmentResourceID *azcorearm.ResourceID `json:"denyAssignmentResourceID"`
internal/api/types_serviceprovider_cluster.go:212
AzureResources.DenyAssignmentsis a Cosmos-persisted status field, but it lacks the required per-field "Written by:" annotation that is used throughout this file for Cosmos writer provenance.
This issue also appears on line 252 of the same file.
// AzureResources groups the Azure resource references associated with a cluster.
type AzureResources struct {
// DenyAssignments tracks the deny assignments applied to the cluster's resources.
DenyAssignments DenyAssignmentReferences `json:"denyAssignments,omitempty"`
// ManagedResourceGroup tracks the managed resource group for the cluster.
ManagedResourceGroup AzureReference `json:"managedResourceGroup,omitempty"`
}
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:210
- This controller introduces new Cosmos writes/updates to
ServiceProviderCluster.Status.AzureResources.DenyAssignments, butdocs/cosmos-data-flow.mdhas no entries for these deny-assignment fields (no "deny assignments"/"DenyAssignments" references found). The Cosmos data flow doc should be updated to reflect the new writers/readers.
// Ensure all existing deny assignments have correct content.
// Succeeded stay in AzureResources; failed move to pending for retry.
ensureExistingSucceeded, ensureExistingFailed, ensureExistingErr := c.ensureDenyAssignmentReferences(ctx, cluster, denyAssignmentsClient, genericResourcesClient,
managedResourceGroupID, denyAssignmentDefinitionsByType, replacement.Status.AzureResources.DenyAssignments.AzureResources)
replacement.Status.AzureResources.DenyAssignments.AzureResources = ensureExistingSucceeded
replacement.Status.AzureResources.DenyAssignments.PendingAzureResources = appendDenyAssignmentReference(replacement.Status.AzureResources.DenyAssignments.PendingAzureResources, ensureExistingFailed...)
serviceProviderCluster, replacement, err = replaceServiceProviderClusterIfChanged(ctx, serviceProviderClusterCRUD, serviceProviderCluster, replacement, []error{ensureExistingErr})
| if len(spc.Status.AzureResources.DenyAssignments.AzureResources) > 0 || len(spc.Status.AzureResources.DenyAssignments.PendingAzureResources) > 0 { | ||
| remainingTypes := make([]string, 0, len(spc.Status.AzureResources.DenyAssignments.AzureResources)+len(spc.Status.AzureResources.DenyAssignments.PendingAzureResources)) | ||
| for _, denyAssignmentReference := range spc.Status.AzureResources.DenyAssignments.AzureResources { | ||
| remainingTypes = append(remainingTypes, denyAssignmentReference.DenyAssignmentType) | ||
| } | ||
| for _, denyAssignmentReference := range spc.Status.AzureResources.DenyAssignments.PendingAzureResources { | ||
| remainingTypes = append(remainingTypes, denyAssignmentReference.DenyAssignmentType) | ||
| } | ||
| logger.Info("waiting for deny assignments to be deleted before removing ServiceProviderCluster", | ||
| "serviceProviderClusterResourceID", spc.ResourceID.String(), | ||
| "remainingDenyAssignmentTypes", remainingTypes) | ||
| return false, nil | ||
| } |
| // we're unlikely to reach here with PendingAzureResource, but if we do, running again is probably a good idea. | ||
| replacement.Status.AzureResources.DenyAssignments.EarliestRecheckTime = nil | ||
| } | ||
| _, _, err = replaceServiceProviderClusterIfChanged(ctx, serviceProviderClusterCRUD, serviceProviderCluster, replacement, nil) |
There was a problem hiding this comment.
Here if we end up returning an error we will have set the earliestretry already: https://github.com/Azure/ARO-HCP/pull/6269/changes#diff-f060ec6fb0ba2b018e4b0f10978336bed126fe6ffaf9285e1461ba52930110a4R244
shouldn't be earliestretry unset in that case?
a2bdaad to
0c4df62
Compare
|
David Eads (@deads2k): The following test failed, say
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. |
Adds a controller that manages Azure deny assignments on the managed resource group for each HCP cluster. The controller ensures all required deny assignments exist with correct content, deletes stale ones, and periodically rechecks consistency with jittered recheck intervals. Deny assignments are scoped to the managed resource group and are cleaned up automatically when the resource group is deleted during cluster teardown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 5 comments.
Files not reviewed (2)
- backend/pkg/azure/client/mock_fpa_client_builder.go: Generated file
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:100
- The PR description says deny assignments are deleted during cluster deletion and that ServiceProviderCluster deletion is blocked while deny assignments remain, but this controller explicitly no-ops when DeletionTimestamp is set. The deletion cleanup controller also comments that deny assignment references are never cleared. Please reconcile the behavior with the PR description (either implement delete+status-clearing semantics, or adjust the description/requirements so reviewers/operators aren’t expecting deletion gating).
// Nothing to do while the cluster is being deleted. The deny assignments are scoped to the
// managed resource group, so Azure deletes them in cascade when that resource group is removed
// during cluster teardown; there is no need to issue ARM deletions or otherwise reconcile them
// here. (Per Manyanda Karombi's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.)
if cluster.ServiceProviderProperties.DeletionTimestamp != nil {
return nil
}
| // DenyAssignmentResourceID is the full Azure resource ID of the deny assignment, | ||
| // e.g. "/subscriptions/{sub}/resourceGroups/{rg}/providers/Microsoft.Authorization/denyAssignments/{uuid}". | ||
| // Written by: ClusterDenyAssignment | ||
| DenyAssignmentResourceID *azcorearm.ResourceID `json:"denyAssignmentResourceID"` | ||
| } |
| type DenyAssignmentReferences struct { | ||
| // PendingAzureResources contains resource IDs that have been requested but | ||
| // not yet confirmed to exist in Azure. |
| // Written by: ClusterDenyAssignment | ||
| PendingAzureResources []DenyAssignmentReference `json:"pendingDenyAssignments,omitempty"` | ||
| // AzureResources contains resource IDs that have been confirmed to exist in Azure. | ||
| // Written by: ClusterDenyAssignment | ||
| AzureResources []DenyAssignmentReference `json:"denyAssignments,omitempty"` |
| if len(serviceProviderCluster.Status.AzureResources.DenyAssignments.PendingAzureResources) == 0 && len(serviceProviderCluster.Status.AzureResources.DenyAssignments.AzureResources) > 0 { | ||
| return true, nil | ||
| } |
| for _, ref := range refs { | ||
| definition, ok := denyAssignmentDefinitionsByType[ref.DenyAssignmentType] | ||
| if !ok { |
0c4df62 to
d2f32fa
Compare
|
/hold cancel |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 5 comments.
Files not reviewed (2)
- backend/pkg/azure/client/mock_fpa_client_builder.go: Generated file
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:239
- Pending deny assignment references that are no longer required are never pruned. If a stale/unknown denyAssignmentType ends up in PendingAzureResources (e.g., after definitions change), it will keep cluster creation blocked forever because createPreconditionDenyAssignmentsCreated waits for PendingAzureResources to be empty. Consider dropping pending refs whose type is not in requiredDenyAssignmentReferenceByType before adding/ensuring required types.
// Add any required types not already in AzureResources or pending.
for _, ref := range requiredDenyAssignmentReferences {
found := false
for _, existing := range replacement.Status.AzureResources.DenyAssignments.AzureResources {
if existing.DenyAssignmentType == ref.DenyAssignmentType {
found = true
break
}
}
if !found {
replacement.Status.AzureResources.DenyAssignments.PendingAzureResources = appendDenyAssignmentReference(replacement.Status.AzureResources.DenyAssignments.PendingAzureResources, ref)
}
}
| // Nothing to do while the cluster is being deleted. The deny assignments are scoped to the | ||
| // managed resource group, so Azure deletes them in cascade when that resource group is removed | ||
| // during cluster teardown; there is no need to issue ARM deletions or otherwise reconcile them | ||
| // here. (Per Manyanda Karombi's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.) | ||
| if cluster.ServiceProviderProperties.DeletionTimestamp != nil { | ||
| return nil | ||
| } |
| // We intentionally do not gate ServiceProviderCluster cleanup on the tracked deny assignments. | ||
| // Deny assignments are scoped to the managed resource group, so Azure deletes them in cascade | ||
| // when that resource group is removed during cluster teardown; the ClusterDenyAssignment | ||
| // controller therefore does nothing on delete and never clears these references. Gating here | ||
| // would block cleanup forever. (Per Manyanda Karombi's note on | ||
| // https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.) |
| serviceProviderCluster, err := corecosmosstorage.GetOrCreateServiceProviderCluster(ctx, c.resourcesDBClient, cluster.ID) | ||
| if err != nil { | ||
| return utils.TrackError(fmt.Errorf("failed to get or create ServiceProviderCluster: %w", err)) | ||
| } |
| type DenyAssignmentReferences struct { | ||
| // PendingAzureResources contains resource IDs that have been requested but | ||
| // not yet confirmed to exist in Azure. | ||
| // Written by: ClusterDenyAssignment | ||
| PendingAzureResources []DenyAssignmentReference `json:"pendingDenyAssignments,omitempty"` | ||
| // AzureResources contains resource IDs that have been confirmed to exist in Azure. | ||
| // Written by: ClusterDenyAssignment | ||
| AzureResources []DenyAssignmentReference `json:"denyAssignments,omitempty"` | ||
| // EarliestRecheckTime is the earliest time at which the controller should | ||
| // re-check the pending resources. Nil means recheck immediately. | ||
| // This allows for controllers to avoid repeatedly hitting an Azure API to recheck that the desired state is true. | ||
| // Controllers should set this field with substantial jitter: without another concern, jitter of 50% is considered normal | ||
| // so that any storms are quickly dissipated. | ||
| // Additionally, long recheck times are recommended for resources outside of their active phases. Order of at least | ||
| // six hours is, with durations up to 24 hours considered normal. | ||
| // Written by: ClusterDenyAssignment | ||
| EarliestRecheckTime *metav1.Time `json:"earliestRecheckTime,omitempty"` | ||
| } |
| // HasRealFPA indicates the backend runs against a real First Party Application rather than the | ||
| // insecure MI mock. Controllers that create Azure resources only a real FPA can create (e.g. | ||
| // deny assignments) are disabled when this is false (dev/int environments). | ||
| HasRealFPA bool |
|
replaced by #6680 |
Add a backend controller that creates Azure deny assignments on the managed resource group before cluster-service create and deletes them during cluster deletion. The controller tracks state via PendingDenyAssignments and DenyAssignments on ServiceProviderClusterStatus.
The deny assignment permissions catalog is ported from clusters-service, covering 19 resource-provider-specific deny assignments that enforce least-privilege access for OpenShift operator managed identities.
Key behaviors:
/hold
cluster service needs fixes before we can do this.