Skip to content

OCPBUGS-84251: fix(azure): detect and replace stale role assignments on cluster re-creation #8322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 2 commits into from
Apr 28, 2026
Merged

OCPBUGS-84251: fix(azure): detect and replace stale role assignments on cluster re-creation #8322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
717dbef
fix(cli): detect and replace stale Azure role assignments on cluster …
bryan-cox Apr 24, 2026
7443553
docs: add --dns-zone-rg-name to Azure destroy command examples
bryan-cox Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cmd/cluster/azure/destroy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ func NewDestroyCommand(opts *core.DestroyOptions) *cobra.Command {
cmd.Flags().StringVar(&opts.AzurePlatform.Location, "location", opts.AzurePlatform.Location, "Location for the cluster")
cmd.Flags().StringVar(&opts.AzurePlatform.ResourceGroupName, "resource-group-name", opts.AzurePlatform.ResourceGroupName, "The name of the resource group containing the HostedCluster infrastructure resources that need to be destroyed.")
cmd.Flags().BoolVar(&opts.AzurePlatform.PreserveResourceGroup, "preserve-resource-group", opts.AzurePlatform.PreserveResourceGroup, "When true, the managed/main resource group will not be deleted during cluster destroy. Only cluster-specific resources within the resource group will be cleaned up.")
cmd.Flags().StringVar(&opts.AzurePlatform.DNSZoneRGName, "dns-zone-rg-name", opts.AzurePlatform.DNSZoneRGName, util.DNSZoneRGNameDestroyDescription)

@Nirshal Nirshal Apr 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this required instead than optional:

  • there is not such an ecosystem of early adopters that justifies the risk to pollute resource groups if users are unaware/forgetful of the flag, just for retro-compatibility concerns;
  • it is a symmetric behavior with respect to the create, so it make sense having to specify the flag for the destroy, if it is required for the create.

Additionally, we should properly document the flag in the existing howto Azure self-managed documentation in docs/content/how-to/azure/

@bryan-cox bryan-cox Apr 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Made --dns-zone-rg-name required in all 3 destroy commands (cmd/cluster/azure/destroy.go, cmd/infra/azure/destroy_iam.go, product-cli/cmd/cluster/azure/destroy.go). Updated docs in create-self-managed-azure-cluster.md, create-iam-separately.md, and deploy-azure-private-clusters.md to include the flag in all destroy command examples.

AI-assisted response via Claude Code


_ = cmd.MarkFlagRequired("azure-creds")
_ = cmd.MarkFlagRequired("dns-zone-rg-name")

logger := log.Log
cmd.Run = func(cmd *cobra.Command, args []string) {
Expand Down Expand Up @@ -148,6 +150,24 @@ func DestroyCluster(ctx context.Context, o *core.DestroyOptions) error {
}

func destroyPlatformSpecifics(ctx context.Context, o *core.DestroyOptions) error {
// Clean up role assignments before destroying infrastructure to avoid orphans.
// Match the create path resource-group names: {name}-nsg and {name}-vnet.
subscriptionID, azureCreds, err := util.SetupAzureCredentials(o.Log, nil, o.AzurePlatform.CredentialsFile)
if err != nil {
return fmt.Errorf("failed to setup Azure credentials: %w", err)
}

nsgRG := o.Name + "-nsg"
vnetRG := o.Name + "-vnet"

rbacManager := azureinfra.NewRBACManager(subscriptionID, azureCreds)
// assignCustomHCPRoles=false is safe: GetServicePrincipalScopes only uses the flag to select
// the role definition ID, not to modify the scopes list. Cleanup derives role assignment names
// from infraID + component + scope, so the role ID is irrelevant.
if err := rbacManager.CleanupRoleAssignments(ctx, o.Log, o.InfraID, o.AzurePlatform.ResourceGroupName, nsgRG, vnetRG, o.AzurePlatform.DNSZoneRGName, false); err != nil {
o.Log.Error(err, "Failed to clean up some role assignments, continuing with infrastructure deletion")
Comment thread
bryan-cox marked this conversation as resolved.
}

destroyInfraOptions := &azureinfra.DestroyInfraOptions{
Name: o.Name,
Location: o.AzurePlatform.Location,
Expand Down
1 change: 1 addition & 0 deletions cmd/cluster/core/destroy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type AzurePlatformDestroyOptions struct {
ResourceGroupName string
PreserveResourceGroup bool
Cloud string
DNSZoneRGName string
}

type PowerVSPlatformDestroyOptions struct {
Expand Down
19 changes: 19 additions & 0 deletions cmd/infra/azure/destroy_iam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ func NewDestroyIAMCommand() *cobra.Command {
cmd.Flags().StringVar(&opts.Cloud, "cloud", opts.Cloud, util.CloudDescription)
cmd.Flags().StringVar(&opts.Name, "name", opts.Name, util.NameDescription)
cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, util.InfraIDDescription)
cmd.Flags().StringVar(&opts.DNSZoneRG, "dns-zone-rg-name", opts.DNSZoneRG, util.DNSZoneRGNameDestroyDescription)

_ = cmd.MarkFlagRequired("workload-identities-file")
_ = cmd.MarkFlagRequired("azure-creds")
_ = cmd.MarkFlagRequired("resource-group-name")
_ = cmd.MarkFlagRequired("name")
_ = cmd.MarkFlagRequired("infra-id")
_ = cmd.MarkFlagRequired("dns-zone-rg-name")

l := log.Log
cmd.RunE = func(cmd *cobra.Command, args []string) error {
Expand Down Expand Up @@ -70,6 +72,7 @@ func BindDestroyIAMProductFlags(opts *DestroyIAMOptions, flags *pflag.FlagSet) {
flags.StringVar(&opts.Cloud, "cloud", opts.Cloud, util.CloudDescription)
flags.StringVar(&opts.Name, "name", opts.Name, util.NameDescription)
flags.StringVar(&opts.InfraID, "infra-id", opts.InfraID, util.InfraIDDescription)
flags.StringVar(&opts.DNSZoneRG, "dns-zone-rg-name", opts.DNSZoneRG, "The resource group name where the DNS zone resides (used to clean up role assignments)")

@Nirshal Nirshal Apr 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment

@bryan-cox bryan-cox Apr 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Applied the same change here — --dns-zone-rg-name is now required in destroy_iam.go (both MarkFlagRequired and Validate()). Docs updated as well.

AI-assisted response via Claude Code

}

// Validate validates the DestroyIAMOptions
Expand All @@ -89,6 +92,9 @@ func (o *DestroyIAMOptions) Validate() error {
if o.InfraID == "" {
return fmt.Errorf("infra-id is required")
}
if o.DNSZoneRG == "" {
return fmt.Errorf("dns-zone-rg-name is required")
}
return nil
}

Expand All @@ -110,6 +116,19 @@ func (o *DestroyIAMOptions) Run(ctx context.Context, l logr.Logger) error {
"infraID", o.InfraID,
"resourceGroup", o.ResourceGroupName)

// Clean up role assignments before destroying identities to avoid orphans.
// Match the create path resource-group names: {name}-nsg and {name}-vnet.
nsgRG := o.Name + "-nsg"
vnetRG := o.Name + "-vnet"

rbacManager := NewRBACManager(subscriptionID, azureCreds)
// assignCustomHCPRoles=false is safe: GetServicePrincipalScopes only uses the flag to select
// the role definition ID, not to modify the scopes list. Cleanup derives role assignment names
// from infraID + component + scope, so the role ID is irrelevant.
if err := rbacManager.CleanupRoleAssignments(ctx, l, o.InfraID, o.ResourceGroupName, nsgRG, vnetRG, o.DNSZoneRG, false); err != nil {
l.Error(err, "Failed to clean up some role assignments, continuing with identity deletion")
}
Comment thread
bryan-cox marked this conversation as resolved.

// Create the identity manager
identityManager := NewIdentityManager(subscriptionID, azureCreds, o.Cloud)

Expand Down
13 changes: 13 additions & 0 deletions cmd/infra/azure/destroy_iam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func TestDestroyIAMOptionsValidate(t *testing.T) {
WorkloadIdentitiesFile: "/path/to/identities.json",
CredentialsFile: "/path/to/creds.json",
ResourceGroupName: "test-rg",
DNSZoneRG: "dns-zone-rg",
},
expectedError: false,
description: "Should pass when all required fields are provided",
Expand Down Expand Up @@ -81,6 +82,18 @@ func TestDestroyIAMOptionsValidate(t *testing.T) {
errorContains: "infra-id is required",
description: "Should require infra-id",
},
"When dns-zone-rg-name is empty it should return an error": {
opts: DestroyIAMOptions{
Name: "test-cluster",
InfraID: "test-infra-id",
WorkloadIdentitiesFile: "/path/to/identities.json",
CredentialsFile: "/path/to/creds.json",
ResourceGroupName: "test-rg",
},
expectedError: true,
errorContains: "dns-zone-rg-name is required",
description: "Should require dns-zone-rg-name",
},
}

for name, test := range tests {
Expand Down
166 changes: 141 additions & 25 deletions cmd/infra/azure/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,26 @@ import (

"github.com/Azure/azure-sdk-for-go/sdk/azcore"
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
azureauth "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"

"k8s.io/utils/ptr"

"github.com/go-logr/logr"
)

const (
graphAPIEndpoint = "https://graph.microsoft.com/v1.0/servicePrincipals"
)

// roleAssignmentClient abstracts the Azure role assignment operations for testability.
type roleAssignmentClient interface {
Get(ctx context.Context, scope string, roleAssignmentName string, options *azureauth.RoleAssignmentsClientGetOptions) (azureauth.RoleAssignmentsClientGetResponse, error)
Delete(ctx context.Context, scope string, roleAssignmentName string, options *azureauth.RoleAssignmentsClientDeleteOptions) (azureauth.RoleAssignmentsClientDeleteResponse, error)
Create(ctx context.Context, scope string, roleAssignmentName string, parameters azureauth.RoleAssignmentCreateParameters, options *azureauth.RoleAssignmentsClientCreateOptions) (azureauth.RoleAssignmentsClientCreateResponse, error)
NewListForScopePager(scope string, options *azureauth.RoleAssignmentsClientListForScopeOptions) *runtime.Pager[azureauth.RoleAssignmentsClientListForScopeResponse]
}

// RBACManager handles Azure RBAC operations
type RBACManager struct {
subscriptionID string
Expand Down Expand Up @@ -100,6 +111,11 @@ func (r *RBACManager) assignRolesForComponents(ctx context.Context, opts *Create
return err
}

raClient, err := azureauth.NewRoleAssignmentsClient(r.subscriptionID, r.creds, nil)
if err != nil {
return fmt.Errorf("failed to create role assignments client: %w", err)
}

for component, clientID := range components {
objectID, err := r.getObjectIDFromClientID(string(clientID), token)
if err != nil {
Expand All @@ -109,7 +125,7 @@ func (r *RBACManager) assignRolesForComponents(ctx context.Context, opts *Create
role, scopes := azureutil.GetServicePrincipalScopes(r.subscriptionID, resourceGroupName, nsgResourceGroupName, vnetResourceGroupName, opts.DNSZoneRG, component, opts.AssignCustomHCPRoles)

for _, scope := range scopes {
if err := r.assignRole(ctx, opts.InfraID, component, objectID, role, scope); err != nil {
if err := r.assignRole(ctx, raClient, opts.InfraID, component, objectID, role, scope); err != nil {
return fmt.Errorf("failed to perform role assignment: %w", err)
}
}
Expand All @@ -128,12 +144,17 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr
return err
}

raClient, err := azureauth.NewRoleAssignmentsClient(r.subscriptionID, r.creds, nil)
if err != nil {
return fmt.Errorf("failed to create role assignments client: %w", err)
}

// Setup Data Plane MI role assignments
objectID, err := r.getObjectIDFromClientID(dataPlaneIdentities.ImageRegistryMSIClientID, token)
if err != nil {
return err
}
err = r.assignRole(ctx, opts.InfraID, config.CIRO+"WI", objectID, config.ImageRegistryRoleDefinitionID, managedRG)
err = r.assignRole(ctx, raClient, opts.InfraID, config.CIRO+"WI", objectID, config.ImageRegistryRoleDefinitionID, managedRG)
if err != nil {
return err
}
Expand All @@ -142,7 +163,7 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr
if err != nil {
return err
}
err = r.assignRole(ctx, opts.InfraID, config.AzureDisk+"WI", objectID, config.AzureDiskRoleDefinitionID, managedRG)
err = r.assignRole(ctx, raClient, opts.InfraID, config.AzureDisk+"WI", objectID, config.AzureDiskRoleDefinitionID, managedRG)
if err != nil {
return err
}
Expand All @@ -151,7 +172,7 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr
if err != nil {
return err
}
err = r.assignRole(ctx, opts.InfraID, config.AzureFile+"WI", objectID, config.AzureFileRoleDefinitionID, managedRG)
err = r.assignRole(ctx, raClient, opts.InfraID, config.AzureFile+"WI", objectID, config.AzureFileRoleDefinitionID, managedRG)
if err != nil {
return err
}
Expand All @@ -160,12 +181,7 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr
}

// assignRole assigns a scoped role to the service principal assignee
func (r *RBACManager) assignRole(ctx context.Context, infraID, component, assigneeID, role, scope string) error {
roleAssignmentClient, err := azureauth.NewRoleAssignmentsClient(r.subscriptionID, r.creds, nil)
if err != nil {
return fmt.Errorf("failed to create new role assignments client: %w", err)
}

func (r *RBACManager) assignRole(ctx context.Context, client roleAssignmentClient, infraID, component, assigneeID, role, scope string) error {
// Generate the role assignment name
roleAssignmentName := util.GenerateRoleAssignmentName(infraID, component, scope)

Expand All @@ -184,7 +200,7 @@ func (r *RBACManager) assignRole(ctx context.Context, infraID, component, assign
// Robust existence check:
// 1) List assignments for this principalId at or around this scope and
// verify one matches both the exact scope and role definition ID.
pager := roleAssignmentClient.NewListForScopePager(scope, &azureauth.RoleAssignmentsClientListForScopeOptions{
pager := client.NewListForScopePager(scope, &azureauth.RoleAssignmentsClientListForScopeOptions{
// Use atScope() to reliably list assignments at this scope, then match in code
Filter: ptr.To("atScope()"),
})
Expand All @@ -208,25 +224,47 @@ func (r *RBACManager) assignRole(ctx context.Context, infraID, component, assign
}

// 2) Fallback to a direct GET by our deterministic name; create only if 404.
_, err = roleAssignmentClient.Get(ctx, scope, roleAssignmentName, nil)
// If the assignment exists but points to a different principal (stale/orphaned from a
// previous cluster with the same infraID), delete it and fall through to create a new one.
existing, err := client.Get(ctx, scope, roleAssignmentName, nil)
if err == nil {
log.Log.Info("Skipping role assignment creation, role assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope)
return nil
}
var respErr *azcore.ResponseError
if errors.As(err, &respErr) {
if respErr.StatusCode == http.StatusNotFound {
// proceed to create
} else if respErr.StatusCode == http.StatusForbidden || strings.EqualFold(respErr.ErrorCode, "AuthorizationFailed") {
log.Log.Info("Get not permitted; will attempt create and rely on 409 for idempotency.", "role", role, "assigneeID", assigneeID, "scope", scope)
} else {
return fmt.Errorf("failed checking role assignment existence: %w", err)
if existing.Properties != nil &&
existing.Properties.PrincipalID != nil &&
existing.Properties.RoleDefinitionID != nil &&
strings.EqualFold(*existing.Properties.PrincipalID, assigneeID) &&
strings.EqualFold(*existing.Properties.RoleDefinitionID, roleDefinitionID) {
log.Log.Info("Skipping role assignment creation, role assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope)
return nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
// Stale assignment — different principal owns this deterministic name.
stalePrincipal := "<nil>"
if existing.Properties != nil && existing.Properties.PrincipalID != nil {
stalePrincipal = *existing.Properties.PrincipalID
}
log.Log.Info("Deleting stale role assignment with mismatched principal",
"role", role, "expectedPrincipal", assigneeID,
"stalePrincipal", stalePrincipal,
"scope", scope)
if _, err := client.Delete(ctx, scope, roleAssignmentName, nil); err != nil {
return fmt.Errorf("failed to delete stale role assignment: %w", err)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Fall through to create a fresh assignment below.
} else {
return fmt.Errorf("failed to check role assignment existence: %w", err)
var respErr *azcore.ResponseError
if errors.As(err, &respErr) {
if respErr.StatusCode == http.StatusNotFound {
// proceed to create
} else if respErr.StatusCode == http.StatusForbidden || strings.EqualFold(respErr.ErrorCode, "AuthorizationFailed") {
log.Log.Info("Get not permitted; will attempt create and rely on 409 for idempotency.", "role", role, "assigneeID", assigneeID, "scope", scope)
} else {
return fmt.Errorf("failed checking role assignment existence: %w", err)
}
} else {
return fmt.Errorf("failed to check role assignment existence: %w", err)
}
}

_, err = roleAssignmentClient.Create(ctx, scope, roleAssignmentName, roleAssignmentProperties, nil)
_, err = client.Create(ctx, scope, roleAssignmentName, roleAssignmentProperties, nil)
if err != nil {
var respErr *azcore.ResponseError
if errors.As(err, &respErr) && (respErr.StatusCode == http.StatusConflict || strings.EqualFold(respErr.ErrorCode, "RoleAssignmentExists")) {
Expand All @@ -239,6 +277,84 @@ func (r *RBACManager) assignRole(ctx context.Context, infraID, component, assign
return nil
}

// CleanupRoleAssignments deletes all role assignments created for a cluster's workload identities.
// It regenerates the deterministic role assignment names from the infraID, component names, and scopes,
// then deletes each one. This must be called before destroying managed identities to avoid orphaned
// role assignments that cause naming collisions on re-creation.
func (r *RBACManager) CleanupRoleAssignments(ctx context.Context, l logr.Logger, infraID string, resourceGroupName, nsgResourceGroupName, vnetResourceGroupName, dnsZoneRG string, assignCustomHCPRoles bool) error {
raClient, err := azureauth.NewRoleAssignmentsClient(r.subscriptionID, r.creds, nil)
if err != nil {
return fmt.Errorf("failed to create role assignments client: %w", err)
}
return r.cleanupRoleAssignments(ctx, l, raClient, infraID, resourceGroupName, nsgResourceGroupName, vnetResourceGroupName, dnsZoneRG, assignCustomHCPRoles)
}

// cleanupRoleAssignments is the testable inner method that performs the actual cleanup.
func (r *RBACManager) cleanupRoleAssignments(ctx context.Context, l logr.Logger, client roleAssignmentClient, infraID string, resourceGroupName, nsgResourceGroupName, vnetResourceGroupName, dnsZoneRG string, assignCustomHCPRoles bool) error {
// All components that may have role assignments
components := []string{
config.CPO,
config.NodePoolMgmt,
config.CloudProvider,
config.AzureFile,
config.AzureDisk,
config.Ingress,
config.CNCC,
config.CIRO,
}

// Data plane components use a "WI" suffix
dataPlaneComponents := []string{
config.CIRO + "WI",
config.AzureDisk + "WI",
config.AzureFile + "WI",
}

var deleteErrors []error

// Cleanup control plane and workload identity role assignments
for _, component := range components {
_, scopes := azureutil.GetServicePrincipalScopes(r.subscriptionID, resourceGroupName, nsgResourceGroupName, vnetResourceGroupName, dnsZoneRG, component, assignCustomHCPRoles)
for _, scope := range scopes {
name := util.GenerateRoleAssignmentName(infraID, component, scope)
if err := r.deleteRoleAssignmentByName(ctx, l, client, scope, name, component); err != nil {
deleteErrors = append(deleteErrors, err)
}
}
}

// Cleanup data plane role assignments (only scoped to managed RG)
managedRG := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", r.subscriptionID, resourceGroupName)
for _, component := range dataPlaneComponents {
name := util.GenerateRoleAssignmentName(infraID, component, managedRG)
if err := r.deleteRoleAssignmentByName(ctx, l, client, managedRG, name, component); err != nil {
deleteErrors = append(deleteErrors, err)
}
}

if len(deleteErrors) > 0 {
return fmt.Errorf("failed to delete %d role assignments during cleanup: %w", len(deleteErrors), errors.Join(deleteErrors...))
}

l.Info("Successfully cleaned up all role assignments", "infraID", infraID)
return nil
}

func (r *RBACManager) deleteRoleAssignmentByName(ctx context.Context, l logr.Logger, client roleAssignmentClient, scope, name, component string) error {
_, err := client.Delete(ctx, scope, name, nil)
if err != nil {
var respErr *azcore.ResponseError
if errors.As(err, &respErr) && respErr.StatusCode == http.StatusNotFound {
l.Info("Role assignment not found, skipping", "component", component, "scope", scope)
return nil
}
l.Error(err, "Failed to delete role assignment", "component", component, "scope", scope)
return err
}
l.Info("Deleted role assignment", "component", component, "scope", scope)
return nil
}

func (r *RBACManager) getAzureToken() (azcore.AccessToken, error) {
token, err := r.creds.GetToken(context.Background(), policy.TokenRequestOptions{
Scopes: []string{"https://graph.microsoft.com/.default"},
Expand Down
Loading