Add deny assignment management - #6680
Conversation
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>
|
[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
This PR introduces backend-side management of Azure deny assignments for HCP clusters, including tracking their lifecycle in ServiceProviderCluster and gating Cluster Service creation until deny assignments are in place (when running with a real First Party Application).
Changes:
- Add a new
ClusterDenyAssignmentcontroller to create/update/delete deny assignments in the cluster’s managed resource group and persist state into Cosmos (ServiceProviderCluster.Status.AzureResources.DenyAssignments). - Extend the ServiceProviderCluster API model to track deny assignment references (pending vs confirmed) and add helper functions for deterministic deny assignment resource IDs.
- Gate cluster creation dispatch on deny assignment readiness when the environment supports a real FPA (and explicitly skip that gate when it does not).
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/coreapi/zz_generated.deepcopy.go | Adds deepcopy support for new deny assignment reference types. |
| internal/api/coreapi/types_serviceprovider_cluster.go | Replaces denyAssignments from AzureMultiReference to typed deny assignment reference tracking. |
| internal/api/coreapi/types_cosmosdata.go | Adds helpers to build/parse deny assignment resource IDs. |
| backend/pkg/utils/controllerutils/util.go | Adds helper to consistently extract Cluster Service ID (pending vs assigned). |
| backend/pkg/controllers/cluster/denyassignments/deny_assignment_permissions.go | Defines deny-assignment action/notAction sets per RP area. |
| backend/pkg/controllers/cluster/denyassignments/deny_assignment_definitions.go | Defines deny assignment types and computes required references per cluster. |
| backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go | Implements ClusterDenyAssignment controller reconciliation and Azure upsert/delete logic. |
| backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller_test.go | Adds unit tests for deny assignment controller logic and UUID derivation. |
| backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go | Avoids deletion gating on deny assignment tracking (cascade deletion via MRG). |
| backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller.go | Adds precondition: deny assignments created (only when enabled). |
| backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller_test.go | Updates/extends creation tests to include deny assignment precondition behavior. |
| backend/pkg/azure/client/mock_fpa_client_builder.go | Extends gomock builder to support DenyAssignments/GenericResources clients. |
| backend/pkg/azure/client/generic_resources_client.go | Introduces GenericResourcesClient interface abstraction for ARM generic resources. |
| backend/pkg/azure/client/fpa_client_builder.go | Adds FPA client construction for DenyAssignments and GenericResources. |
| backend/pkg/azure/client/deny_assignments_client.go | Introduces DenyAssignmentsClient interface abstraction. |
| backend/pkg/azure/azuremockclient/mock_clients.go | Adds lightweight mock Azure clients for deny assignments + generic resources. |
| backend/pkg/app/backend.go | Wires the new deny assignment controller conditionally (real FPA only). |
| backend/cmd/root.go | Plumbs HasRealFPA into backend options to enable/disable deny assignments behavior. |
Files not reviewed (2)
- backend/pkg/azure/client/mock_fpa_client_builder.go: Generated file
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // 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 AzureMultiReference `json:"denyAssignments,omitempty"` | ||
| DenyAssignments DenyAssignmentReferences `json:"denyAssignments,omitempty"` | ||
| // ManagedResourceGroup tracks the managed resource group for the cluster. | ||
| ManagedResourceGroup AzureReference `json:"managedResourceGroup,omitempty"` |
| // GenericResourcesClientFunc adapts functions to the GenericResourcesClient interface. | ||
| // Tests set the function fields to control the response. | ||
| // BeginCreateOrUpdateByID and BeginDeleteByID return a nil *Poller and an error — to simulate | ||
| // success, return (nil, nil) and the calling code will call PollUntilDone on nil. | ||
| // To avoid that, the tests should exercise paths that don't reach PollUntilDone (e.g. error paths) | ||
| // or the mock should capture the call without returning a real poller. | ||
| // | ||
| // For paths that call PollUntilDone, set CreateErr/DeleteErr to non-nil to prevent the nil-pointer dereference. | ||
| type GenericResourcesClientFunc struct { |
| var clusterDenyAssignmentController controllerutils.Controller | ||
| if b.options.HasRealFPA { | ||
| clusterDenyAssignmentController = denyassignments.NewClusterDenyAssignmentController( | ||
| utilsclock.RealClock{}, | ||
| b.options.ResourcesDBClient, | ||
| b.options.FPAClientBuilder, | ||
| backendInformers, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Nitpick and it can be a followup;
Always running this controller even in environments where we don't have a real FPA and in those envs, we NOOP the calls but tracks the azure resources ids etc so that we can visualise verify the controller running correctly
| // 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 |
There was a problem hiding this comment.
| // would block cleanup forever. (Per Manyanda Karombi's note on | |
| // would block cleanup forever. (Per Manyanda Chitimbo's note on |
:-)
| // 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.) |
There was a problem hiding this comment.
| // here. (Per Manyanda Karombi's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.) | |
| // here. (Per Manyanda Chitimbo's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.) |
| lookup := make(map[string]string, len(cluster.Identity.UserAssignedIdentities)) | ||
| for resourceID, identity := range cluster.Identity.UserAssignedIdentities { | ||
| if identity != nil && identity.PrincipalID != nil { | ||
| lookup[strings.ToLower(resourceID)] = *identity.PrincipalID | ||
| } | ||
| } |
There was a problem hiding this comment.
The principalIDs of the data plane identities won't be in this list; this list only contains the identities that are resolvable via MSI RP i.e the control plane identities + service managed identity.
For data plane identies, we need to resolve the principal ID from https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L270
While addressing this, you could consider also reading the CP + SMI ones from https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L260
| return c.syncDenyAssignmentUpsert(ctx, key, cluster) | ||
| } | ||
|
|
||
| func (c *clusterDenyAssignmentSyncer) syncDenyAssignmentNeedsWork(cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster) bool { |
There was a problem hiding this comment.
Also check for
https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L260 and https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L270 contain non empty identities
i.e len(.DataPlaneOperatorsManagedIdentities.Identies) > 0 , similarly for CP + SMI so that this controller only runs when the principalIDs of those operators have been resolved to avoid this piece https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go#L360 returning an error for an expected situation during cluster creation becaus the identities are eventually synced
/assign Manyanda Chitimbo (@machi1990)